From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alexey Kodanev <alexey.kodanev@oracle.com>
Subject: Re: [PATCH net v2] udp6: set dst cache for a connected sk before
 udp_v6_send_skb
Date: Wed, 28 Mar 2018 17:57:22 +0300
Message-ID: <1df03279-565a-8f34-c975-99c1a16711f3@oracle.com>
References: <1522075727-19860-1-git-send-email-alexey.kodanev@oracle.com>
 <20180326170037.36xikwnham3zaiel@kafai-mbp.dhcp.thefacebook.com>
 <7d9ede59-3c29-4b09-eb2e-2650400a7d5c@oracle.com>
 <20180327190002.bbrfik2pucgwepyu@kafai-mbp.dhcp.thefacebook.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        David Miller <davem@davemloft.net>
To: Martin KaFai Lau <kafai@fb.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from userp2130.oracle.com ([156.151.31.86]:56248 "EHLO
        userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751072AbeC1O5f (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 28 Mar 2018 10:57:35 -0400
In-Reply-To: <20180327190002.bbrfik2pucgwepyu@kafai-mbp.dhcp.thefacebook.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 27.03.2018 22:00, Martin KaFai Lau wrote:
> On Tue, Mar 27, 2018 at 04:27:30PM +0300, Alexey Kodanev wrote:
>> On 26.03.2018 20:02, Martin KaFai Lau wrote:
>>> On Mon, Mar 26, 2018 at 05:48:47PM +0300, 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
>>> Instead of ip6_dst_lookup_flow(),
>>> you meant the RTF_CACHE route created in ip6_update_pmtu()
>>>
>>
>> Right, or even earlier... I was using vti tunnel and it invokes
>> skb_dst_update_pmtu() on this error, then sends ICMPv6_PKT_TOOBIG.
>>
>>>>                 ...
>>>>                 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().
>>> After this patch, the above udpv6_err() path could not happen after
>>> ip6_sk_dst_lookup_flow() and before the ip6_dst_store() in udpv6_sendmsg()?
>>>
>>
>> May be we could minimize this if save it in ip6_sk_dst_lookup_flow()
>> for a connected UDP sockets only if we're not getting it from a cache
>> for some reason?
> I am just not clear how moving it earlier can completely stop the
> described issue.  Beside, the next ICMPV6_PKT_TOOBIG will be
> received again and eventually rectify sk->sk_dst_cache?


With a request/response scenario in my environment, it always happens
after udp_v6_send_skb() and before the 'release_dst:' in udpv6_sendmsg().
So the socket there never gets updated cache when resending datagram
again after ICMPV6_PKT_TOOBIG... dst cache is always reset to the old
one in the end of udpv6_sendmsg().


> 
>>
>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>> index a8a9195..0204f52 100644
>> --- a/net/ipv6/ip6_output.c
>> +++ b/net/ipv6/ip6_output.c
>> @@ -1115,13 +1115,30 @@ struct dst_entry *ip6_dst_lookup_flow(const struct sock *sk, struct flowi6 *fl6,
>>   *     error code.
>>   */
>>  struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
>> -                                        const struct in6_addr *final_dst)
>> +                                        const struct in6_addr *final_dst,
>> +                                        bool connected)
>>  {
>>         struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
>>  
>>         dst = ip6_sk_dst_check(sk, dst, fl6);
>> -       if (!dst)
>> -               dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
>> +       if (dst)
>> +               return dst;
>> +
>> +       dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
>> +
>> +       if (connected && !IS_ERR(dst))
>> +               ip6_dst_store(sk, dst_clone(dst), ...);
> Right, I think doing dst_store() only if ip6_sk_dst_check()/
> sk_dst_check() returns NULL is a more sensible thing to
> do here.

OK, will prepare the patch.

Thanks,
Alexey