From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from aserp2130.oracle.com ([141.146.126.79]:50904 "EHLO
        aserp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751400AbeCWRT0 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 23 Mar 2018 13:19:26 -0400
Subject: Re: [PATCH net] udp6: set dst cache for a connected sk before
 udp_v6_send_skb
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>
From: Alexey Kodanev <alexey.kodanev@oracle.com>
Message-ID: <6e3bf7fc-572c-9d6f-3d15-85a3b7699f3b@oracle.com>
Date: Fri, 23 Mar 2018 20:13:54 +0300
MIME-Version: 1.0
In-Reply-To: <538ef522-619b-becd-7391-0770d19fa33d@gmail.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 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