| 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
| ||
|
Date: Wed, 12 Oct 2022 11:59:43 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
David Ahern <dsahern@...nel.org>,
Martin KaFai Lau <martin.lau@...nel.org>,
Craig Gallek <kraig@...gle.com>,
Willem de Bruijn <willemb@...gle.com>,
Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net] udp: Update reuse->has_conns under reuseport_lock.
On Wed, Oct 12, 2022 at 11:53 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> When we call connect() for a UDP socket in a reuseport group, we have
> to update sk->sk_reuseport_cb->has_conns to 1. Otherwise, the kernel
> could select a unconnected socket wrongly for packets sent to the
> connected socket.
>
> However, the current way to set has_conns is illegal and possible to
> trigger that problem. reuseport_has_conns() changes has_conns under
> rcu_read_lock(), which upgrades the RCU reader to the updater. Then,
> it must do the update under the updater's lock, reuseport_lock, but
> it doesn't for now.
>
> For this reason, there is a race below where we fail to set has_conns
> resulting in the wrong socket selection. To avoid the race, let's split
> the reader and updater with proper locking.
>
> cpu1 cpu2
> +----+ +----+
>
> __ip[46]_datagram_connect() reuseport_grow()
> . .
> |- reuseport_has_conns(sk, true) |- more_reuse = __reuseport_alloc(more_socks_size)
> | . |
> | |- rcu_read_lock()
> | |- reuse = rcu_dereference(sk->sk_reuseport_cb)
> | |
> | | | /* reuse->has_conns == 0 here */
> | | |- more_reuse->has_conns = reuse->has_conns
> | |- reuse->has_conns = 1 | /* more_reuse->has_conns SHOULD BE 1 HERE */
> | | |
> | | |- rcu_assign_pointer(reuse->socks[i]->sk_reuseport_cb,
> | | | more_reuse)
> | `- rcu_read_unlock() `- kfree_rcu(reuse, rcu)
> |
> |- sk->sk_state = TCP_ESTABLISHED
>
> Fixes: acdcecc61285 ("udp: correct reuseport selection with connected sockets")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
> v2:
> * Fix build failure for CONFIG_IPV6=m
> * Drop SO_INCOMING_CPU fix, which will be sent for net-next
> after the v6.1 merge window
>
> v1: https://lore.kernel.org/netdev/20221010174351.11024-1-kuniyu@amazon.com/
> ---
> include/net/sock_reuseport.h | 11 +++++------
> net/core/sock_reuseport.c | 15 +++++++++++++++
> net/ipv4/datagram.c | 2 +-
> net/ipv4/udp.c | 2 +-
> net/ipv6/datagram.c | 2 +-
> net/ipv6/udp.c | 2 +-
> 6 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/include/net/sock_reuseport.h b/include/net/sock_reuseport.h
> index 473b0b0fa4ab..efc9085c6892 100644
> --- a/include/net/sock_reuseport.h
> +++ b/include/net/sock_reuseport.h
> @@ -43,21 +43,20 @@ struct sock *reuseport_migrate_sock(struct sock *sk,
> extern int reuseport_attach_prog(struct sock *sk, struct bpf_prog *prog);
> extern int reuseport_detach_prog(struct sock *sk);
>
> -static inline bool reuseport_has_conns(struct sock *sk, bool set)
> +static inline bool reuseport_has_conns(struct sock *sk)
> {
> struct sock_reuseport *reuse;
> bool ret = false;
>
> rcu_read_lock();
> reuse = rcu_dereference(sk->sk_reuseport_cb);
> - if (reuse) {
> - if (set)
> - reuse->has_conns = 1;
> - ret = reuse->has_conns;
> - }
> + if (reuse && reuse->has_conns)
> + ret = true;
> rcu_read_unlock();
>
> return ret;
> }
>
> +void reuseport_has_conns_set(struct sock *sk);
> +
> #endif /* _SOCK_REUSEPORT_H */
> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> index 5daa1fa54249..abb414ed4aa7 100644
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c
> @@ -21,6 +21,21 @@ static DEFINE_IDA(reuseport_ida);
> static int reuseport_resurrect(struct sock *sk, struct sock_reuseport *old_reuse,
> struct sock_reuseport *reuse, bool bind_inany);
>
> +void reuseport_has_conns_set(struct sock *sk)
> +{
> + struct sock_reuseport *reuse;
> +
> + if (!rcu_access_pointer(sk->sk_reuseport_cb))
> + return;
> +
> + spin_lock(&reuseport_lock);
> + reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
> + lockdep_is_held(&reuseport_lock));
Could @reuse be NULL at this point ?
Previous test was performed without reuseport_lock being held.
> + reuse->has_conns = 1;
> + spin_unlock(&reuseport_lock);
> +}
> +EXPORT_SYMBOL(reuseport_has_conns_set);
> +
> static int reuseport_sock_index(struct sock *sk,
> const struct sock_reuseport *reuse,
> bool closed)
> diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
> index 405a8c2aea64..5e66add7befa 100644
> --- a/net/ipv4/datagram.c
> +++ b/net/ipv4/datagram.c
> @@ -70,7 +70,7 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
> }
> inet->inet_daddr = fl4->daddr;
> inet->inet_dport = usin->sin_port;
> - reuseport_has_conns(sk, true);
> + reuseport_has_conns_set(sk);
> sk->sk_state = TCP_ESTABLISHED;
> sk_set_txhash(sk);
> inet->inet_id = prandom_u32();
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index d63118ce5900..29228231b058 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -448,7 +448,7 @@ static struct sock *udp4_lib_lookup2(struct net *net,
> result = lookup_reuseport(net, sk, skb,
> saddr, sport, daddr, hnum);
> /* Fall back to scoring if group has connections */
> - if (result && !reuseport_has_conns(sk, false))
> + if (result && !reuseport_has_conns(sk))
> return result;
>
> result = result ? : sk;
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index df665d4e8f0f..5ecb56522f9d 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -256,7 +256,7 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
> goto out;
> }
>
> - reuseport_has_conns(sk, true);
> + reuseport_has_conns_set(sk);
> sk->sk_state = TCP_ESTABLISHED;
> sk_set_txhash(sk);
> out:
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 91e795bb9ade..56e4523a3004 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -182,7 +182,7 @@ static struct sock *udp6_lib_lookup2(struct net *net,
> result = lookup_reuseport(net, sk, skb,
> saddr, sport, daddr, hnum);
> /* Fall back to scoring if group has connections */
> - if (result && !reuseport_has_conns(sk, false))
> + if (result && !reuseport_has_conns(sk))
> return result;
>
> result = result ? : sk;
> --
> 2.30.2
>
Powered by blists - more mailing lists