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: <CAM_iQpXT2Q1UKpyjmf3hu=EqL=HesE268tK5Y0xksPPMHgZv0g@mail.gmail.com>
Date:	Tue, 22 Mar 2016 11:19:27 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Martin KaFai Lau <kafai@...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 10:39 AM, Martin KaFai Lau <kafai@...com> wrote:
> 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 ;)

Sure, I will take a look at this once net-next is re-open.


>
>>
>>
>> > 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?


I thought sk->sk_dst_cache is generic to all sockets, but it is up to
each kind of
socket to decide to use it or not, and you are right, raw socket doesn't seem to
care about it even though it calls *_sk_update_pmtu().