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
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: Eric Dumazet <edumazet@google.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Link: https://patch.msgid.link/20250424143549.669426-2-willemdebruijn.kernel@gmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index e3864b74e92a6c1e1e1b4bb82017e593b7c2c5f9..48bb3cf414691255309febcc96d4723e039ed8ed 100644 (file)
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -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);
 

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 5326f1501af020f976d9d4c1e99f45970ea1b2a7..2371f311a1e1428c4ba06f7f9181b508d70a4504 100644 (file)
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -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))
-                               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;
 
-               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);
 }
 #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

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 49cffbe838029567825e7ae664638228dc7f324a..e5e4c71be3afbc64d2051ef2256022e301e3efa7 100644 (file)
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -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