Message-ID: <680fac4d8cc75_23f881294be@willemb.c.googlers.com.notmuch>
Date: Mon, 28 Apr 2025 12:26:53 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Ido Schimmel <idosch@...dia.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 horms@...nel.org, 
 kuniyu@...zon.com, 
 Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next v2 1/3] ipv4: prefer multipath nexthop that
 matches source address

Ido Schimmel wrote:
> On Sat, Apr 26, 2025 at 11:01:31AM -0400, Willem de Bruijn wrote:
> > Ido Schimmel wrote:
> > > On Thu, Apr 24, 2025 at 10:35:18AM -0400, Willem de Bruijn wrote:
> > > > From: Willem de Bruijn <willemb@...gle.com>
> > > > 
> > > > With multipath routes, try to ensure that packets leave on the device
> > > > that is associated with the source address.
> > > > 
> > > > Avoid the following tcpdump example:
> > > > 
> > > >     veth0 Out IP 10.1.0.2.38640 > 10.2.0.3.8000: Flags [S]
> > > >     veth1 Out IP 10.1.0.2.38648 > 10.2.0.3.8000: Flags [S]
> > > > 
> > > > Which can happen easily with the most straightforward setup:
> > > > 
> > > >     ip addr add 10.0.0.1/24 dev veth0
> > > >     ip addr add 10.1.0.1/24 dev veth1
> > > > 
> > > >     ip route add 10.2.0.3 nexthop via 10.0.0.2 dev veth0 \
> > > >     			  nexthop via 10.1.0.2 dev veth1
> > > > 
> > > > This is apparently considered WAI, based on the comment in
> > > > ip_route_output_key_hash_rcu:
> > > > 
> > > >     * 2. Moreover, we are allowed to send packets with saddr
> > > >     *    of another iface. --ANK
> > > > 
> > > > It may be ok for some uses of multipath, but not all. For instance,
> > > > when using two ISPs, a router may drop packets with unknown source.
> > > > 
> > > > The behavior occurs because tcp_v4_connect makes three route
> > > > lookups when establishing a connection:
> > > > 
> > > > 1. ip_route_connect calls to select a source address, with saddr zero.
> > > > 2. ip_route_connect calls again now that saddr and daddr are known.
> > > > 3. ip_route_newports calls again after a source port is also chosen.
> > > > 
> > > > With a route with multiple nexthops, each lookup may make a different
> > > > choice depending on available entropy to fib_select_multipath. So it
> > > > is possible for 1 to select the saddr from the first entry, but 3 to
> > > > select the second entry. Leading to the above situation.
> > > > 
> > > > Address this by preferring a match that matches the flowi4 saddr. This
> > > > will make 2 and 3 make the same choice as 1. Continue to update the
> > > > backup choice until a choice that matches saddr is found.
> > > > 
> > > > Do this in fib_select_multipath itself, rather than passing an fl4_oif
> > > > constraint, to avoid changing non-multipath route selection. Commit
> > > > e6b45241c57a ("ipv4: reset flowi parameters on route connect") shows
> > > > how that may cause regressions.
> > > > 
> > > > Also read ipv4.sysctl_fib_multipath_use_neigh only once. No need to
> > > > refresh in the loop.
> > > > 
> > > > This does not happen in IPv6, which performs only one lookup.
> > > > 
> > > > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > > > Reviewed-by: David Ahern <dsahern@...nel.org>
> > > 
> > > Reviewed-by: Ido Schimmel <idosch@...dia.com>
> > > 
> > > One note below
> > > 
> > > [...]
> > > 
> > > > -void fib_select_multipath(struct fib_result *res, int hash)
> > > > +void fib_select_multipath(struct fib_result *res, int hash,
> > > > +			  const struct flowi4 *fl4)
> > > >  {
> > > >  	struct fib_info *fi = res->fi;
> > > >  	struct net *net = fi->fib_net;
> > > > -	bool first = false;
> > > > +	bool found = false;
> > > > +	bool use_neigh;
> > > > +	__be32 saddr;
> > > >  
> > > >  	if (unlikely(res->fi->nh)) {
> > > >  		nexthop_path_fib_result(res, hash);
> > > >  		return;
> > > >  	}
> > > >  
> > > > +	use_neigh = READ_ONCE(net->ipv4.sysctl_fib_multipath_use_neigh);
> > > > +	saddr = fl4 ? fl4->saddr : 0;
> > > > +
> > > >  	change_nexthops(fi) {
> > > > -		if (READ_ONCE(net->ipv4.sysctl_fib_multipath_use_neigh)) {
> > > > -			if (!fib_good_nh(nexthop_nh))
> > > > -				continue;
> > > > -			if (!first) {
> > > > -				res->nh_sel = nhsel;
> > > > -				res->nhc = &nexthop_nh->nh_common;
> > > > -				first = true;
> > > > -			}
> > > > +		if (use_neigh && !fib_good_nh(nexthop_nh))
> > > > +			continue;
> > > > +
> > > > +		if (!found) {
> > > > +			res->nh_sel = nhsel;
> > > > +			res->nhc = &nexthop_nh->nh_common;
> > > > +			found = !saddr || nexthop_nh->nh_saddr == saddr;
> > > >  		}
> > > >  
> > > >  		if (hash > atomic_read(&nexthop_nh->fib_nh_upper_bound))
> > > >  			continue;
> > > 
> > > Note that because 'res' is set before comparing the hash with the hash
> > > threshold, it's possible to choose a nexthop that does not have a
> > > carrier (they are assigned a hash threshold of -1), whereas this did
> > > not happen before. Tested with [1].
> > 
> > This is different from the previous pre-threshold choice if !first,
> > because that choice was always tested with fib_good_nh(), while now
> > that is optional?
> 
> I'm not sure I understood the question, but my point is that we can make
> the code a bit clearer and more "correct" with something like this [1]
> as a follow-up. It honors the "ignore_routes_with_linkdown" sysctl and
> skips over nexthops that do not have a carrier.
>
> I tested with [2] which fails without the patch. fib_tests.sh is also OK
> [3] (including the new tests).
> 
> In practice, the patch shouldn't make a big difference. For the case of
> saddr==0 (e.g., forwarding), it shouldn't make any difference because
> you are guaranteed to find a nexthop whose upper bound covers the
> calculated hash.
> 
> For the case of saddr!=0 (e.g., locally generated traffic) this patch
> will not choose a nexthop if it has the correct address but no carrier.
> Like I said before, it probably doesn't matter in practice because the
> route lookup for the source address wouldn't choose this nexthop /
> address in the first place.
> 
> [1]
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 2371f311a1e1..ce56fe39b185 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -2188,7 +2188,14 @@ void fib_select_multipath(struct fib_result *res, int hash,
>  	saddr = fl4 ? fl4->saddr : 0;
>  
>  	change_nexthops(fi) {
> -		if (use_neigh && !fib_good_nh(nexthop_nh))
> +		int nh_upper_bound;
> +
> +		/* Nexthops without a carrier are assigned an upper bound of
> +		 * minus one when "ignore_routes_with_linkdown" is set.
> +		 */
> +		nh_upper_bound = atomic_read(&nexthop_nh->fib_nh_upper_bound);
> +		if (nh_upper_bound == -1 ||
> +		    (use_neigh && !fib_good_nh(nexthop_nh)))
>  			continue;
>  
>  		if (!found) {
> @@ -2197,7 +2204,7 @@ void fib_select_multipath(struct fib_result *res, int hash,
>  			found = !saddr || nexthop_nh->nh_saddr == saddr;
>  		}
>  
> -		if (hash > atomic_read(&nexthop_nh->fib_nh_upper_bound))
> +		if (hash > nh_upper_bound)
>  			continue;
>  
>  		if (!saddr || nexthop_nh->nh_saddr == saddr) {

Makes sense, thanks.

Do you want to send the follow up to net-next once the series
lands there?
 
> [2]
> #!/bin/bash
> 
> trap cleanup EXIT
> 
> cleanup() {
> 	ip netns del ns1
> }
> 
> ip netns add ns1
> ip -n ns1 link set dev lo up
> 
> ip -n ns1 link add name dummy1 up type dummy
> ip -n ns1 link add name dummy2 up type dummy
> 
> ip -n ns1 address add 192.0.2.1/28 dev dummy1
> ip -n ns1 address add 192.0.2.17/28 dev dummy2
> 
> ip -n ns1 route add 198.51.100.0/24 \
> 	nexthop via 192.0.2.2 dev dummy1 \
> 	nexthop via 192.0.2.18 dev dummy2
> 
> ip netns exec ns1 sysctl -wq net.ipv4.fib_multipath_hash_policy=1
> ip netns exec ns1 sysctl -wq net.ipv4.conf.all.ignore_routes_with_linkdown=1
> 
> ip -n ns1 link set dev dummy2 carrier off
> 
> for i in {1..128}; do
> 	ip -n ns1 route get to 198.51.100.1 from 192.0.2.17 \
> 		ipproto tcp sport $i dport $i | grep -q dummy2
> 	[[ $? -eq 0 ]] && echo "FAIL" && exit
> done
> 
> echo "SUCCESS"
> 
> [3]
> # ./fib_tests.sh
> [...]
> Tests passed: 230
> Tests failed:   0

Powered by blists - more mailing lists