On 03/23/2018 06:50 PM, Eric Dumazet wrote:
>
>
> On 03/23/2018 07:39 AM, Alexey Kodanev wrote:
>> After commit 33c162a980fe ("ipv6: datagram: Update dst cache of a
>> connected datagram sk during pmtu update"), when the error occurs on
>> sending datagram in udpv6_sendmsg() due to ICMPV6_PKT_TOOBIG type,
>> error handler can trigger the following path and call ip6_dst_store():
>>
>> udpv6_err()
>> ip6_sk_update_pmtu()
>> ip6_datagram_dst_update()
>> ip6_dst_lookup_flow(), can create a RTF_CACHE clone
>> ...
>> ip6_dst_store()
>>
>> It can happen before a connected UDP socket invokes ip6_dst_store()
>> in the end of udpv6_sendmsg(), on destination release, as a result,
>> the last one changes dst to the old one, preventing getting updated
>> dst cache on the next udpv6_sendmsg() call.
>>
>> This patch moves ip6_dst_store() in udpv6_sendmsg(), so that it is
>> invoked after ip6_sk_dst_lookup_flow() and before udp_v6_send_skb().
>>
>
>
> A Fixes: tag would be nice, for automatic tools (and humans as well)
Hi Eric,
I see, will add the commit 33c162a980fe.
...
>>
>> + if (connected)
>> + ip6_dst_store(sk, dst,
>> + ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
>> + &sk->sk_v6_daddr : NULL,
>> +#ifdef CONFIG_IPV6_SUBTREES
>> + ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
>> + &np->saddr :
>> +#endif
>> + NULL);
>> +
>
> What about the MSG_CONFIRM stuff ?
>
>> if (msg->msg_flags&MSG_CONFIRM)
>> goto do_confirm;
>> back_from_confirm:
>
> Should not you move the above code here instead ?
Ah, you are right, it can release that dst if it go to "do_confirm".
> > Also ip6_dst_store() does not increment dst refcount.
>
> I fear that as soon as dst is visible to other cpus, it might be stolen.
>
So we should pass dst_clone(dst) to ip6_dst_store() instead of dst,
because udpv6_err() can release it if it's set the new one.
Then, I guess, we could left udpv6_sendmsg()/ip6_dst_store() where it
is now in the patch and remove the check for "connected" before dst_relase(),
similar to udp_sendmsg(), right?
Thanks,
Alexey
