| 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
| ||
|
Message-ID: <680cf54b983d5_193a06294ab@willemb.c.googlers.com.notmuch>
Date: Sat, 26 Apr 2025 11:01:31 -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 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 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.
Agreed. Thanks for the thorough review.
> >
> > - 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
Powered by blists - more mailing lists