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
