| Message ID | 20231108211325.18938-1-sdf@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 871019b22d1bcc9fab2d1feba1b9a564acbb6e99 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: set SOCK_RCU_FREE before inserting socket into hashtable | expand |
From: Stanislav Fomichev <sdf@google.com> Date: Wed, 8 Nov 2023 13:13:25 -0800 > We've started to see the following kernel traces: > > WARNING: CPU: 83 PID: 0 at net/core/filter.c:6641 sk_lookup+0x1bd/0x1d0 > > Call Trace: > <IRQ> > __bpf_skc_lookup+0x10d/0x120 > bpf_sk_lookup+0x48/0xd0 > bpf_sk_lookup_tcp+0x19/0x20 > bpf_prog_<redacted>+0x37c/0x16a3 > cls_bpf_classify+0x205/0x2e0 > tcf_classify+0x92/0x160 > __netif_receive_skb_core+0xe52/0xf10 > __netif_receive_skb_list_core+0x96/0x2b0 > napi_complete_done+0x7b5/0xb70 > <redacted>_poll+0x94/0xb0 > net_rx_action+0x163/0x1d70 > __do_softirq+0xdc/0x32e > asm_call_irq_on_stack+0x12/0x20 > </IRQ> > do_softirq_own_stack+0x36/0x50 > do_softirq+0x44/0x70 > > __inet_hash can race with lockless (rcu) readers on the other cpus: > > __inet_hash > __sk_nulls_add_node_rcu > <- (bpf triggers here) > sock_set_flag(SOCK_RCU_FREE) > > Let's move the SOCK_RCU_FREE part up a bit, before we are inserting > the socket into hashtables. Note, that the race is really harmless; > the bpf callers are handling this situation (where listener socket > doesn't have SOCK_RCU_FREE set) correctly, so the only > annoyance is a WARN_ONCE. > > More details from Eric regarding SOCK_RCU_FREE timeline: > > Commit 3b24d854cb35 ("tcp/dccp: do not touch listener sk_refcnt under > synflood") added SOCK_RCU_FREE. At that time, the precise location of > sock_set_flag(sk, SOCK_RCU_FREE) did not matter, because the thread calling > __inet_hash() owns a reference on sk. SOCK_RCU_FREE was only tested > at dismantle time. > > Commit 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF") > started checking SOCK_RCU_FREE _after_ the lookup to infer whether > the refcount has been taken care of. > > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF") > Reviewed-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Stanislav Fomichev <sdf@google.com> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > net/ipv4/inet_hashtables.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c > index 598c1b114d2c..a532f749e477 100644 > --- a/net/ipv4/inet_hashtables.c > +++ b/net/ipv4/inet_hashtables.c > @@ -751,12 +751,12 @@ int __inet_hash(struct sock *sk, struct sock *osk) > if (err) > goto unlock; > } > + sock_set_flag(sk, SOCK_RCU_FREE); > if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport && > sk->sk_family == AF_INET6) > __sk_nulls_add_node_tail_rcu(sk, &ilb2->nulls_head); > else > __sk_nulls_add_node_rcu(sk, &ilb2->nulls_head); > - sock_set_flag(sk, SOCK_RCU_FREE); > sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1); > unlock: > spin_unlock(&ilb2->lock); > -- > 2.42.0.869.gea05f2083d-goog
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 8 Nov 2023 13:13:25 -0800 you wrote: > We've started to see the following kernel traces: > > WARNING: CPU: 83 PID: 0 at net/core/filter.c:6641 sk_lookup+0x1bd/0x1d0 > > Call Trace: > <IRQ> > __bpf_skc_lookup+0x10d/0x120 > bpf_sk_lookup+0x48/0xd0 > bpf_sk_lookup_tcp+0x19/0x20 > bpf_prog_<redacted>+0x37c/0x16a3 > cls_bpf_classify+0x205/0x2e0 > tcf_classify+0x92/0x160 > __netif_receive_skb_core+0xe52/0xf10 > __netif_receive_skb_list_core+0x96/0x2b0 > napi_complete_done+0x7b5/0xb70 > <redacted>_poll+0x94/0xb0 > net_rx_action+0x163/0x1d70 > __do_softirq+0xdc/0x32e > asm_call_irq_on_stack+0x12/0x20 > </IRQ> > do_softirq_own_stack+0x36/0x50 > do_softirq+0x44/0x70 > > [...] Here is the summary with links: - [net,v2] net: set SOCK_RCU_FREE before inserting socket into hashtable https://git.kernel.org/netdev/net/c/871019b22d1b You are awesome, thank you!
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c index 598c1b114d2c..a532f749e477 100644 --- a/net/ipv4/inet_hashtables.c +++ b/net/ipv4/inet_hashtables.c @@ -751,12 +751,12 @@ int __inet_hash(struct sock *sk, struct sock *osk) if (err) goto unlock; } + sock_set_flag(sk, SOCK_RCU_FREE); if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport && sk->sk_family == AF_INET6) __sk_nulls_add_node_tail_rcu(sk, &ilb2->nulls_head); else __sk_nulls_add_node_rcu(sk, &ilb2->nulls_head); - sock_set_flag(sk, SOCK_RCU_FREE); sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1); unlock: spin_unlock(&ilb2->lock);
We've started to see the following kernel traces: WARNING: CPU: 83 PID: 0 at net/core/filter.c:6641 sk_lookup+0x1bd/0x1d0 Call Trace: <IRQ> __bpf_skc_lookup+0x10d/0x120 bpf_sk_lookup+0x48/0xd0 bpf_sk_lookup_tcp+0x19/0x20 bpf_prog_<redacted>+0x37c/0x16a3 cls_bpf_classify+0x205/0x2e0 tcf_classify+0x92/0x160 __netif_receive_skb_core+0xe52/0xf10 __netif_receive_skb_list_core+0x96/0x2b0 napi_complete_done+0x7b5/0xb70 <redacted>_poll+0x94/0xb0 net_rx_action+0x163/0x1d70 __do_softirq+0xdc/0x32e asm_call_irq_on_stack+0x12/0x20 </IRQ> do_softirq_own_stack+0x36/0x50 do_softirq+0x44/0x70 __inet_hash can race with lockless (rcu) readers on the other cpus: __inet_hash __sk_nulls_add_node_rcu <- (bpf triggers here) sock_set_flag(SOCK_RCU_FREE) Let's move the SOCK_RCU_FREE part up a bit, before we are inserting the socket into hashtables. Note, that the race is really harmless; the bpf callers are handling this situation (where listener socket doesn't have SOCK_RCU_FREE set) correctly, so the only annoyance is a WARN_ONCE. More details from Eric regarding SOCK_RCU_FREE timeline: Commit 3b24d854cb35 ("tcp/dccp: do not touch listener sk_refcnt under synflood") added SOCK_RCU_FREE. At that time, the precise location of sock_set_flag(sk, SOCK_RCU_FREE) did not matter, because the thread calling __inet_hash() owns a reference on sk. SOCK_RCU_FREE was only tested at dismantle time. Commit 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF") started checking SOCK_RCU_FREE _after_ the lookup to infer whether the refcount has been taken care of. Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF") Reviewed-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Stanislav Fomichev <sdf@google.com> --- net/ipv4/inet_hashtables.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)