netdev - Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket

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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 7 Mar 2016 09:35:42 -0800
From:	Eric Dumazet <edumazet@...gle.com>
To:	David Miller <davem@...emloft.net>
Cc:	Wei Wang <weiwan@...gle.com>, netdev <netdev@...r.kernel.org>,
	Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket

On Mon, Mar 7, 2016 at 9:19 AM, Eric Dumazet <edumazet@...gle.com> wrote:
> On Sat, Mar 5, 2016 at 9:55 PM, David Miller <davem@...emloft.net> wrote:
>> From: Wei Wang <weiwan@...gle.com>
>> Date: Wed,  2 Mar 2016 11:19:21 -0800
>>
>>> @@ -566,7 +567,16 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>>>       if (type == ICMPV6_PKT_TOOBIG) {
>>>               if (!ip6_sk_accept_pmtu(sk))
>>>                       goto out;
>>> -             ip6_sk_update_pmtu(skb, sk, info);
>>> +             bh_lock_sock(sk);
>>> +             if (sk->sk_state == TCP_ESTABLISHED &&
>>> +                 !sock_owned_by_user(sk) &&
>>> +                 ipv6_addr_equal(saddr, &sk->sk_v6_rcv_saddr) &&
>>> +                 ipv6_addr_equal(daddr, &sk->sk_v6_daddr) &&
>>> +                 uh->dest == sk->sk_dport)
>>> +                     inet6_csk_update_pmtu(sk, ntohl(info));
>>
>> If I apply this patch it will hide a bug.
>>
>> Why isn't ip6_sk_update_pmtu() matching the same route as the
>> one attached to the socket?
>>
>> I'd prefer you figure out what part of the lookup key used is
>> wrong, and fix that instead.
>>
>
> The dst itself is the same than the socket sk_dst_cache, but
> __ip6_rt_update_pmtu() sees rt6_cache_allowed_for_pmtu()
>
> We endup doing :
>
>                 nrt6 = ip6_rt_cache_alloc(rt6, daddr, saddr);
>                 if (nrt6) {
>                         rt6_do_update_pmtu(nrt6, mtu);
>
>                         /* ip6_ins_rt(nrt6) will bump the
>                          * rt6->rt6i_node->fn_sernum
>                          * which will fail the next rt6_check() and
>                          * invalidate the sk->sk_dst_cache.
>                          */
>                         ip6_ins_rt(nrt6);
>                 }
>
>
> But apparently the sk->sk_dst_cache is _not_ invalidated, even if the
> comment loudly claims so.


Wei and Martin, what do you think of :


diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed446639219c..17e5db80be62 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1342,7 +1342,7 @@ static void rt6_do_update_pmtu(struct rt6_info
*rt, u32 mtu)

 static bool rt6_cache_allowed_for_pmtu(const struct rt6_info *rt)
 {
-       return !(rt->rt6i_flags & RTF_CACHE) &&
+       return (rt->rt6i_flags & RTF_CACHE) &&
                (rt->rt6i_flags & RTF_PCPU || rt->rt6i_node);
 }

@@ -1359,7 +1359,7 @@ static void __ip6_rt_update_pmtu(struct
dst_entry *dst, const struct sock *sk,
        if (mtu >= dst_mtu(dst))
                return;

-       if (!rt6_cache_allowed_for_pmtu(rt6)) {
+       if (rt6_cache_allowed_for_pmtu(rt6)) {
                rt6_do_update_pmtu(rt6, mtu);
        } else {
                const struct in6_addr *daddr, *saddr;