netdev - Re: [PATCH net] udp6: set dst cache for a connected sk before udp_v6_send

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4e99b198-7e5b-77bd-f46c-42f62897e9eb@oracle.com>
Date:   Fri, 23 Mar 2018 20:43:04 +0300
From:   Alexey Kodanev <alexey.kodanev@...cle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org
Cc:     David Miller <davem@...emloft.net>
Subject: Re: [PATCH net] udp6: set dst cache for a connected sk before
 udp_v6_send_skb

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