netdev - Re: [PATCH net] ipv4: fix fnhe usage by non-cached routes

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

Message-ID: <alpine.LFD.2.20.1805030827460.2188@ja.home.ssi.bg>
Date:   Thu, 3 May 2018 08:32:53 +0300 (EEST)
From:   Julian Anastasov <ja@....bg>
To:     David Ahern <dsahern@...il.com>
cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        Martin KaFai Lau <kafai@...com>, kernel-team@...com,
        Xin Long <lucien.xin@...il.com>
Subject: Re: [PATCH net] ipv4: fix fnhe usage by non-cached routes


	Hello,

On Wed, 2 May 2018, David Ahern wrote:

> On 5/2/18 12:41 AM, Julian Anastasov wrote:
> > Allow some non-cached routes to use non-expired fnhe:
> > 
> > 1. ip_del_fnhe: moved above and now called by find_exception.
> > The 4.5+ commit deed49df7390 expires fnhe only when caching
> > routes. Change that to:
> > 
> > 1.1. use fnhe for non-cached local output routes, with the help
> > from (2)
> > 
> > 1.2. allow __mkroute_input to detect expired fnhe (outdated
> > fnhe_gw, for example) when do_cache is false, eg. when itag!=0
> > for unicast destinations.
> > 
> > 2. __mkroute_output: keep fi to allow local routes with orig_oif != 0
> > to use fnhe info even when the new route will not be cached into fnhe.
> > After commit 839da4d98960 ("net: ipv4: set orig_oif based on fib
> > result for local traffic") it means all local routes will be affected
> > because they are not cached. This change is used to solve a PMTU
> > problem with IPVS (and probably Netfilter DNAT) setups that redirect
> > local clients from target local IP (local route to Virtual IP)
> > to new remote IP target, eg. IPVS TUN real server. Loopback has
> > 64K MTU and we need to create fnhe on the local route that will
> > keep the reduced PMTU for the Virtual IP. Without this change
> > fnhe_pmtu is updated from ICMP but never exposed to non-cached
> > local routes. This includes routes with flowi4_oif!=0 for 4.6+ and
> > with flowi4_oif=any for 4.14+).
> 
> Can you add a test case to tools/testing/selftests/net/pmtu.sh to cover
> this situation?

	Sure, I'll give it a try.

> > @@ -1310,8 +1340,14 @@ static struct fib_nh_exception *find_exception(struct fib_nh *nh, __be32 daddr)
> >  
> >  	for (fnhe = rcu_dereference(hash[hval].chain); fnhe;
> >  	     fnhe = rcu_dereference(fnhe->fnhe_next)) {
> > -		if (fnhe->fnhe_daddr == daddr)
> > +		if (fnhe->fnhe_daddr == daddr) {
> > +			if (fnhe->fnhe_expires &&
> > +			    time_after(jiffies, fnhe->fnhe_expires)) {
> > +				ip_del_fnhe(nh, daddr);
> 
> I'm surprised this is done in the fast path vs gc time. (the existing
> code does as well; your change is only moving the call to make the input
> and output paths the same)
> 
> 
> The change looks correct to me and all of my functional tests passed.
> 
> Acked-by: David Ahern <dsahern@...il.com>

	Thanks for the review!

Regards

--
Julian Anastasov <ja@....bg>