Re: [PATCH net] udp6: set dst cache for a connected sk before udp_v6_send_skb

[Alexey Kodanev <alexey.kodanev@oracle.com>]

On 03/23/2018 08:13 PM, Alexey Kodanev wrote:
> On 03/23/2018 06:50 PM, Eric Dumazet wrote:
...
>>> +	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?

And the section "release_dst:" looks redundant after that too.

Thanks,
Alexey