Re: [PATCH net-next v2 1/3] ipv4: prefer multipath nexthop that matches source address

[Ido Schimmel <idosch@nvidia.com>]

On Thu, Apr 24, 2025 at 10:35:18AM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.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@google.com>
> Reviewed-by: David Ahern <dsahern@kernel.org>

Reviewed-by: Ido Schimmel <idosch@nvidia.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].

I guess it's not a problem in practice because the initial route lookup
for the source address wouldn't have chosen the linkdown nexthop to
begin with.

>  
> -		res->nh_sel = nhsel;
> -		res->nhc = &nexthop_nh->nh_common;
> -		return;
> +		if (!saddr || nexthop_nh->nh_saddr == saddr) {
> +			res->nh_sel = nhsel;
> +			res->nhc = &nexthop_nh->nh_common;
> +			return;
> +		}
> +
> +		if (found)
> +			return;
> +
>  	} endfor_nexthops(fi);
>  }

[1]
#!/bin/bash

ip link del dev dummy1 &> /dev/null
ip link del dev dummy2 &> /dev/null

ip link add name dummy1 up type dummy
ip link add name dummy2 up type dummy
ip address add 192.0.2.1/28 dev dummy1
ip address add 192.0.2.17/28 dev dummy2
ip route add 192.0.2.32/28 \
	nexthop via 192.0.2.2 dev dummy1 \
	nexthop via 192.0.2.18 dev dummy2

ip link set dev dummy2 carrier off
sysctl -wq net.ipv4.fib_multipath_hash_policy=1
sysctl -wq net.ipv4.conf.all.ignore_routes_with_linkdown=1

sleep 1

ip route show 192.0.2.32/28
for i in {1..100}; do
	ip route get to 192.0.2.33 from 192.0.2.17 ipproto tcp sport $i dport $i | grep dummy2
done