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: <20160322173939.GA53936@kafai-mba.local>
Date:	Tue, 22 Mar 2016 10:39:39 -0700
From:	Martin KaFai Lau <kafai@...com>
To:	Cong Wang <xiyou.wangcong@...il.com>
CC:	Wei Wang <tracywwnj@...il.com>, Wei Wang <weiwan@...gle.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	David Miller <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket

On Tue, Mar 22, 2016 at 09:53:35AM -0700, Cong Wang wrote:
> On Mon, Mar 21, 2016 at 11:02 PM, Martin KaFai Lau <kafai@...com> wrote:
> > In term of difference, AFAICT, the current patch is an optimization in the
> > sense that the update_pmtu() code path does not have to do a dst_check to
> > discover its sk->sk_dst_cache is invalid, and then do a relookup to find out
> > that the just created RTF_CACHE clone should be used.  To get this, it may
> > make more sense to remove all the relookup code together during update_pmtu().
> > Even if this slow path was to be optimized, should it be put in a
> > separate patch where net-next is a better candidate?
> >
>
> Speaking of RTF_CACHE, I am curious why you didn't use FIB next hop exception
> as what ipv4 does to cache exceptions? This makes IPv6 has more gap with IPv4.
> This is (almost) irrelevant to this patch.
There are a few differences between IPv6 and IPv4.  Both in terms of
data structure and functionality.  The last 'RTF_CACHE on exception' patchset is one
step toward this direction. More patches are needed and are welcomed ;)

>
>
> > I think fixing it in __udp6_lib_err() or what Cong Wang is suggesting makes
> > more sense for a net branch fix.  If there is logic specific to connected-udp,
> > I would do it in the __udp6_lib_err() instead.  After looking at
> > udpv6_sendmsg() and how it calls ip6_dst_store(), may also need to be careful
> > what daddr and saddr should be passed to ip6_dst_store(), or at least a commit
> > message.  The first patch is essentially passing NULL to daddr and saddr
> > while the second patch seems passing something else.
>
> Raw socket needs to fix too, we can't just fix __udp6_lib_err(), this is also
> why fixing ip6_sk_update_pmtu() is better, its call path is better.
I don't see rawv6 socket is storing the dst.  I probably have overlooked it.  Can
you point it out?

Having said that, I don't feel strongly on any of the two places.  I think only
implementation can tell.