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:	Tue, 22 Mar 2016 09:53:35 -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 Mon, Mar 21, 2016 at 11:02 PM, Martin KaFai Lau <kafai@...com> wrote:
> I think Cong Wang is suggesting, in ip6_sk_update_pmtu():
> 1. call ip6_upate_pmtu() as it is
> 2. do a dst_check()
> 3. re-lookup() if it is invalid
> 4. and then do a ip6_dst_store()/dst_set
>
> The above is exactly what inet6_csk_update_pmtu(), which was also used in the
> first patch, is doing.

Well, the reasons why I suggest to fix ip6_sk_update_pmtu() instead of anything
else (no matter __udp6_lib_err() in v1 or ip6_update_pmtu() in v2) are that:

1) Catch up with ipv4 part, which is ipv4_sk_update_pmtu()
2) Straight-forward, (ideally) no need to bother other irrelevant
callers like xfrm.

Functionally all the solutions should work to fix Wei's problem, we just need to
find which one is more elegant.


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


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