Commit 32607a33 authored by Willem de Bruijn's avatar Willem de Bruijn Committed by Paolo Abeni
Browse files

ipv4: prefer multipath nexthop that matches source address 



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
e6b45241 ("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: default avatarWillem de Bruijn <willemb@google.com>
Reviewed-by: default avatarDavid Ahern <dsahern@kernel.org>
Reviewed-by: default avatarEric Dumazet <edumazet@google.com>
Reviewed-by: default avatarIdo Schimmel <idosch@nvidia.com>
Link: https://patch.msgid.link/20250424143549.669426-2-willemdebruijn.kernel@gmail.com


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parent 0d15a26b

include/net/ip_fib.h

+2 −1
Original line number Diff line number Diff line
@@ -574,7 +574,8 @@ static inline u32 fib_multipath_hash_from_keys(const struct net *net, 


int fib_check_nh(struct net *net, struct fib_nh *nh, u32 table, u8 scope,

		 struct netlink_ext_ack *extack);

void fib_select_multipath(struct fib_result *res, int hash);

void fib_select_multipath(struct fib_result *res, int hash,

			  const struct flowi4 *fl4);

void fib_select_path(struct net *net, struct fib_result *res,

		     struct flowi4 *fl4, const struct sk_buff *skb);

net/ipv4/fib_semantics.c

+25 −14
Original line number Diff line number Diff line
@@ -2170,34 +2170,45 @@ static bool fib_good_nh(const struct fib_nh *nh) 
	return !!(state & NUD_VALID);

}



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

		if (use_neigh && !fib_good_nh(nexthop_nh))

			continue;

			if (!first) {



		if (!found) {

			res->nh_sel = nhsel;

			res->nhc = &nexthop_nh->nh_common;

				first = true;

			}

			found = !saddr || nexthop_nh->nh_saddr == saddr;

		}



		if (hash > atomic_read(&nexthop_nh->fib_nh_upper_bound))

			continue;



		if (!saddr || nexthop_nh->nh_saddr == saddr) {

			res->nh_sel = nhsel;

			res->nhc = &nexthop_nh->nh_common;

			return;

		}



		if (found)

			return;



	} endfor_nexthops(fi);

}

#endif
@@ -2212,7 +2223,7 @@ void fib_select_path(struct net *net, struct fib_result *res, 
	if (fib_info_num_path(res->fi) > 1) {

		int h = fib_multipath_hash(net, fl4, skb, NULL);



		fib_select_multipath(res, h);

		fib_select_multipath(res, h, fl4);

	}

	else

#endif

net/ipv4/route.c

+1 −1
Original line number Diff line number Diff line
@@ -2154,7 +2154,7 @@ ip_mkroute_input(struct sk_buff *skb, struct fib_result *res, 
	if (res->fi && fib_info_num_path(res->fi) > 1) {

		int h = fib_multipath_hash(res->fi->fib_net, NULL, skb, hkeys);



		fib_select_multipath(res, h);

		fib_select_multipath(res, h, NULL);

		IPCB(skb)->flags |= IPSKB_MULTIPATH;

	}

#endif