| 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: <68064dfd68775_32bae8294e1@willemb.c.googlers.com.notmuch>
Date: Mon, 21 Apr 2025 09:54:05 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
netdev@...r.kernel.org
Cc: davem@...emloft.net,
kuba@...nel.org,
edumazet@...gle.com,
pabeni@...hat.com,
dsahern@...nel.org,
horms@...nel.org,
idosch@...dia.com,
kuniyu@...zon.com,
Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next 2/3] ip: load balance tcp connections to single
dst addr and port
Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@...gle.com>
>
> Load balance new TCP connections across nexthops also when they
> connect to the same service at a single remote address and port.
>
> This affects only port-based multipath hashing:
> fib_multipath_hash_policy 1 or 3.
>
> Local connections must choose both a source address and port when
> connecting to a remote service, in ip_route_connect. This
> "chicken-and-egg problem" (commit 2d7192d6cbab ("ipv4: Sanitize and
> simplify ip_route_{connect,newports}()")) is resolved by first
> selecting a source address, by looking up a route using the zero
> wildcard source port and address.
>
> As a result multiple connections to the same destination address and
> port have no entropy in fib_multipath_hash.
>
> This is not a problem when forwarding, as skb-based hashing has a
> 4-tuple. Nor when establishing UDP connections, as autobind there
> selects a port before reaching ip_route_connect.
>
> Load balance also TCP, by using a random port in fib_multipath_hash.
> Port assignment in inet_hash_connect is not atomic with
> ip_route_connect. Thus ports are unpredictable, effectively random.
>
> Implementation details:
>
> Do not actually pass a random fl4_sport, as that affects not only
> hashing, but routing more broadly, and can match a source port based
> policy route, which existing wildcard port 0 will not. Instead,
> define a new wildcard flowi flag that is used only for hashing.
>
> Selecting a random source is equivalent to just selecting a random
> hash entirely. But for code clarity, follow the normal 4-tuple hash
> process and only update this field.
>
> fib_multipath_hash can be reached with zero sport from other code
> paths, so explicitly pass this flowi flag, rather than trying to infer
> this case in the function itself.
>
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ---
> include/net/flow.h | 1 +
> include/net/route.h | 3 +++
> net/ipv4/route.c | 13 ++++++++++---
> net/ipv6/route.c | 13 ++++++++++---
> net/ipv6/tcp_ipv6.c | 2 ++
> 5 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/flow.h b/include/net/flow.h
> index 2a3f0c42f092..a1839c278d87 100644
> --- a/include/net/flow.h
> +++ b/include/net/flow.h
> @@ -39,6 +39,7 @@ struct flowi_common {
> #define FLOWI_FLAG_ANYSRC 0x01
> #define FLOWI_FLAG_KNOWN_NH 0x02
> #define FLOWI_FLAG_L3MDEV_OIF 0x04
> +#define FLOWI_FLAG_ANY_SPORT 0x08
> __u32 flowic_secid;
> kuid_t flowic_uid;
> __u32 flowic_multipath_hash;
> diff --git a/include/net/route.h b/include/net/route.h
> index c605fd5ec0c0..8e39aa822cf9 100644
> --- a/include/net/route.h
> +++ b/include/net/route.h
> @@ -326,6 +326,9 @@ static inline void ip_route_connect_init(struct flowi4 *fl4, __be32 dst,
> if (inet_test_bit(TRANSPARENT, sk))
> flow_flags |= FLOWI_FLAG_ANYSRC;
>
> + if (IS_ENABLED(CONFIG_IP_ROUTE_MULTIPATH) && !sport)
> + flow_flags |= FLOWI_FLAG_ANY_SPORT;
> +
> flowi4_init_output(fl4, oif, READ_ONCE(sk->sk_mark), ip_sock_rt_tos(sk),
> ip_sock_rt_scope(sk), protocol, flow_flags, dst,
> src, dport, sport, sk->sk_uid);
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index e5e4c71be3af..685e8d3b4f5d 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -2037,8 +2037,12 @@ static u32 fib_multipath_custom_hash_fl4(const struct net *net,
> hash_keys.addrs.v4addrs.dst = fl4->daddr;
> if (hash_fields & FIB_MULTIPATH_HASH_FIELD_IP_PROTO)
> hash_keys.basic.ip_proto = fl4->flowi4_proto;
> - if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_PORT)
> - hash_keys.ports.src = fl4->fl4_sport;
> + if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_PORT) {
> + if (fl4->flowi4_flags & FLOWI_FLAG_ANY_SPORT)
> + hash_keys.ports.src = get_random_u16();
> + else
> + hash_keys.ports.src = fl4->fl4_sport;
> + }
> if (hash_fields & FIB_MULTIPATH_HASH_FIELD_DST_PORT)
> hash_keys.ports.dst = fl4->fl4_dport;
>
> @@ -2093,7 +2097,10 @@ int fib_multipath_hash(const struct net *net, const struct flowi4 *fl4,
> hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> hash_keys.addrs.v4addrs.src = fl4->saddr;
> hash_keys.addrs.v4addrs.dst = fl4->daddr;
> - hash_keys.ports.src = fl4->fl4_sport;
> + if (fl4->flowi4_flags & FLOWI_FLAG_ANY_SPORT)
> + hash_keys.ports.src = get_random_u16();
> + else
> + hash_keys.ports.src = fl4->fl4_sport;
> hash_keys.ports.dst = fl4->fl4_dport;
> hash_keys.basic.ip_proto = fl4->flowi4_proto;
> }
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 945857a8bfe3..39f07cdbbc64 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2492,8 +2492,12 @@ static u32 rt6_multipath_custom_hash_fl6(const struct net *net,
> hash_keys.basic.ip_proto = fl6->flowi6_proto;
> if (hash_fields & FIB_MULTIPATH_HASH_FIELD_FLOWLABEL)
> hash_keys.tags.flow_label = (__force u32)flowi6_get_flowlabel(fl6);
> - if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_PORT)
> - hash_keys.ports.src = fl6->fl6_sport;
> + if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_PORT) {
> + if (fl6->flowi6_flags & FLOWI_FLAG_ANY_SPORT)
> + hash_keys.ports.src = get_random_u16();
> + else
> + hash_keys.ports.src = fl6->fl6_sport;
> + }
> if (hash_fields & FIB_MULTIPATH_HASH_FIELD_DST_PORT)
> hash_keys.ports.dst = fl6->fl6_dport;
>
> @@ -2547,7 +2551,10 @@ u32 rt6_multipath_hash(const struct net *net, const struct flowi6 *fl6,
> hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> hash_keys.addrs.v6addrs.src = fl6->saddr;
> hash_keys.addrs.v6addrs.dst = fl6->daddr;
> - hash_keys.ports.src = fl6->fl6_sport;
> + if (fl6->flowi6_flags & FLOWI_FLAG_ANY_SPORT)
> + hash_keys.ports.src = get_random_u16();
I missed the __be16 endianness here and in the related cases
That'll teach me to forget running
make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/route.o [...]
> + else
> + hash_keys.ports.src = fl6->fl6_sport;
Powered by blists - more mailing lists