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]

Message-ID: <1458174127.7353.11.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Wed, 16 Mar 2016 17:22:07 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	weiwan@...gle.com, edumazet@...gle.com, netdev@...r.kernel.org
Subject: Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket

On Wed, 2016-03-16 at 19:53 -0400, David Miller wrote:
> From: Wei Wang <weiwan@...gle.com>
> Date: Mon, 14 Mar 2016 13:59:47 -0700
> 
> > From: Wei Wang <weiwan@...gle.com>
> > 
> > When ICMPV6_PKT_TOOBIG message is received by a connected UDP socket,
> > the new mtu value is not properly updated in the dst_entry associated
> > with the socket.
> > This leads to the issue that the mtu value returned by getsockopt(sockfd,
> > IPPROTO_IPV6, IPV6_MTU, ...) is wrong.
> > The fix is to update sk->sk_dst_cache and other corresponding fields
> > when a new routing cache is allocated for the new pmtu in UDP connected
> > socket case.
> > 
> > Signed-off-by: Wei Wang <weiwan@...gle.com>
> 
> Wait a second:
> 
> >  		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.
> > -			 */
> > +			if (sk)
> > +				ip6_dst_store(sk, &nrt6->dst, daddr, saddr);
> >  			ip6_ins_rt(nrt6);
> >  		}
> >  	}
> 
> I still haven't seen a satisfactory answer as to why the as-designed
> invalidation mechanism using fn_sernum is not working.
> 
> If that's broken, then a lot of other things won't work properly
> either.
> 
> I've read the ip6_ins_rt() code path several times, and it always
> increments the serial number, and therefore the next dst->check() call
> (which every cached route usage should invoke) should invalide
> this socket's route and lookup the new one.
> 
> Why does this not work?


One of the issue is that IPV6_MTU getsockopt() will not check the dst,
but simply use __sk_dst_get() : It will then report old mtu.

Not sure we want to use the full check and then if dst appears to be
obsolete, do another route lookup ?