From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from userp2130.oracle.com ([156.151.31.86]:51338 "EHLO
        userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751715AbeCWRdb (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 23 Mar 2018 13:33:31 -0400
Subject: Re: [PATCH net] udp6: set dst cache for a connected sk before
 udp_v6_send_skb
From: Alexey Kodanev <alexey.kodanev@oracle.com>
To: Eric Dumazet <eric.dumazet@gmail.com>, netdev@vger.kernel.org
Cc: David Miller <davem@davemloft.net>
References: <1521815989-26412-1-git-send-email-alexey.kodanev@oracle.com>
 <538ef522-619b-becd-7391-0770d19fa33d@gmail.com>
 <6e3bf7fc-572c-9d6f-3d15-85a3b7699f3b@oracle.com>
Message-ID: <4e99b198-7e5b-77bd-f46c-42f62897e9eb@oracle.com>
Date: Fri, 23 Mar 2018 20:43:04 +0300
MIME-Version: 1.0
In-Reply-To: <6e3bf7fc-572c-9d6f-3d15-85a3b7699f3b@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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