On 4/18/23 8:31 AM, Aditi Ghag wrote:
Previously, BPF TCP iterator was acquiring fast version of sock lock that
disables the BH. This introduced a circular dependency with code paths that
later acquire sockets hash table bucket lock.
Replace the fast version of sock lock with slow that faciliates BPF
programs executed from the iterator to destroy TCP listening sockets
using the bpf_sock_destroy kfunc (implemened in follow-up commits).
Here is a stack trace that motivated this change:
```
1) sock_lock with BH disabled + bucket lock
lock_acquire+0xcd/0x330
_raw_spin_lock_bh+0x38/0x50
inet_unhash+0x96/0xd0
tcp_set_state+0x6a/0x210
tcp_abort+0x12b/0x230
bpf_prog_f4110fb1100e26b5_iter_tcp6_server+0xa3/0xaa
bpf_iter_run_prog+0x1ff/0x340
bpf_iter_tcp_seq_show+0xca/0x190
bpf_seq_read+0x177/0x450
vfs_read+0xc6/0x300
ksys_read+0x69/0xf0
do_syscall_64+0x3c/0x90
entry_SYSCALL_64_after_hwframe+0x72/0xdc
IIUC, the above deadlock is due to
> lock_acquire+0xcd/0x330
> _raw_spin_lock_bh+0x38/0x50
> inet_unhash+0x96/0xd0
> tcp_set_state+0x6a/0x210
> tcp_abort+0x12b/0x230
> bpf_prog_f4110fb1100e26b5_iter_tcp6_server+0xa3/0xaa
> bpf_iter_run_prog+0x1ff/0x340
> ... lock_acquire for sock lock ...
> bpf_iter_tcp_seq_show+0xca/0x190
> bpf_seq_read+0x177/0x450
> vfs_read+0xc6/0x300
> ksys_read+0x69/0xf0
> do_syscall_64+0x3c/0x90
> entry_SYSCALL_64_after_hwframe+0x72/0xdc
I could be great to make it explicit with the stack trace so
it is clear where the circular dependency is.
2) sock lock with BH enable
[ 1.499968] lock_acquire+0xcd/0x330
[ 1.500316] _raw_spin_lock+0x33/0x40
[ 1.500670] sk_clone_lock+0x146/0x520
[ 1.501030] inet_csk_clone_lock+0x1b/0x110
[ 1.501433] tcp_create_openreq_child+0x22/0x3f0
[ 1.501873] tcp_v6_syn_recv_sock+0x96/0x940
[ 1.502284] tcp_check_req+0x137/0x660
[ 1.502646] tcp_v6_rcv+0xa63/0xe80
[ 1.502994] ip6_protocol_deliver_rcu+0x78/0x590
[ 1.503434] ip6_input_finish+0x72/0x140
[ 1.503818] __netif_receive_skb_one_core+0x63/0xa0
[ 1.504281] process_backlog+0x79/0x260
[ 1.504668] __napi_poll.constprop.0+0x27/0x170
[ 1.505104] net_rx_action+0x14a/0x2a0
[ 1.505469] __do_softirq+0x165/0x510
[ 1.505842] do_softirq+0xcd/0x100
[ 1.506172] __local_bh_enable_ip+0xcc/0xf0
[ 1.506588] ip6_finish_output2+0x2a8/0xb00
[ 1.506988] ip6_finish_output+0x274/0x510
[ 1.507377] ip6_xmit+0x319/0x9b0
[ 1.507726] inet6_csk_xmit+0x12b/0x2b0
[ 1.508096] __tcp_transmit_skb+0x549/0xc40
[ 1.508498] tcp_rcv_state_process+0x362/0x1180
Similarly, it would be good to illustrate where is the
deadlock in this case.
```
Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
Signed-off-by: Aditi Ghag <aditi.ghag@xxxxxxxxxxxxx>
---
net/ipv4/tcp_ipv4.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index ea370afa70ed..f2d370a9450f 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2962,7 +2962,6 @@ static int bpf_iter_tcp_seq_show(struct seq_file *seq, void *v)
struct bpf_iter_meta meta;
struct bpf_prog *prog;
struct sock *sk = v;
- bool slow;
uid_t uid;
int ret;
@@ -2970,7 +2969,7 @@ static int bpf_iter_tcp_seq_show(struct seq_file *seq, void *v)
return 0;
if (sk_fullsock(sk))
- slow = lock_sock_fast(sk);
+ lock_sock(sk);
if (unlikely(sk_unhashed(sk))) {
ret = SEQ_SKIP;
@@ -2994,7 +2993,7 @@ static int bpf_iter_tcp_seq_show(struct seq_file *seq, void *v)
unlock:
if (sk_fullsock(sk))
- unlock_sock_fast(sk, slow);
+ release_sock(sk);
return ret;
}
