> On Mar 21, 2023, at 2:31 PM, Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On 03/21, Aditi Ghag wrote:
>> When sockets are destroyed in the BPF iterator context, sock
>> lock is already acquired, so skip taking the lock. This allows
>> TCP listening sockets to be destroyed from BPF programs.
>
>> Signed-off-by: Aditi Ghag <aditi.ghag@xxxxxxxxxxxxx>
>> ---
>> net/ipv4/inet_hashtables.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
>> index e41fdc38ce19..5543a3e0d1b4 100644
>> --- a/net/ipv4/inet_hashtables.c
>> +++ b/net/ipv4/inet_hashtables.c
>> @@ -777,9 +777,11 @@ void inet_unhash(struct sock *sk)
>> /* Don't disable bottom halves while acquiring the lock to
>> * avoid circular locking dependency on PREEMPT_RT.
>> */
>> - spin_lock(&ilb2->lock);
>> + if (!has_current_bpf_ctx())
>> + spin_lock(&ilb2->lock);
>> if (sk_unhashed(sk)) {
>> - spin_unlock(&ilb2->lock);
>> + if (!has_current_bpf_ctx())
>> + spin_unlock(&ilb2->lock);
>
> That's bucket lock, why do we have to skip it?
Good catch! We shouldn't skip it. So the issue is that BPF TCP iterator acquires the sock fast lock that disables BH, and then inet_unhash acquires the bucket lock, but this is in reverse order to when the sock lock is acquired with BH enabled.
@Martin: I wonder if you ran into similar issues while working on the BPF TCP iterator batching changes for setsockopt?
Here is the truncated stack trace:
1.494510] test_progs/118 [HC0[0]:SC0[4]:HE1:SE0] is trying to acquire:
[ 1.495123] ffff9ec9c184da10 (&h->lhash2[i].lock){+.+.}-{2:2}, at: inet_unhash+0x96/0xd0
[ 1.495867]
[ 1.495867] and this task is already holding:
[ 1.496401] ffff9ec9c23400b0 (slock-AF_INET6){+.-.}-{2:2}, at: __lock_sock_fast+0x33/0x70
[ 1.497148] which would create a new lock dependency:
[ 1.497619] (slock-AF_INET6){+.-.}-{2:2} -> (&h->lhash2[i].lock){+.+.}-{2:2}
[ 1.498278]
[ 1.498278] but this new dependency connects a SOFTIRQ-irq-safe lock:
[ 1.499011] (slock-AF_INET6){+.-.}-{2:2}
[ 1.499014]
[ 1.499014] ... which became SOFTIRQ-irq-safe at:
[ 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.512311] to a SOFTIRQ-irq-unsafe lock:
[ 1.512773] (&h->lhash2[i].lock){+.+.}-{2:2}
[ 1.512776]
[ 1.512776] ... which became SOFTIRQ-irq-unsafe at:
[ 1.513691] ...
[ 1.513692] lock_acquire+0xcd/0x330
[ 1.514168] _raw_spin_lock+0x33/0x40
[ 1.514492] __inet_hash+0x4b/0x210
[ 1.514802] inet_csk_listen_start+0xe6/0x100
[ 1.515188] inet_listen+0x95/0x1d0
[ 1.515510] __sys_listen+0x69/0xb0
[ 1.515825] __x64_sys_listen+0x14/0x20
[ 1.516163] do_syscall_64+0x3c/0x90
[ 1.516481] entry_SYSCALL_64_after_hwframe+0x72/0xdc
:
:
[ 1.560494] ... acquired at:
[ 1.560634] lock_acquire+0xcd/0x330
[ 1.560804] _raw_spin_lock_bh+0x38/0x50
[ 1.561002] inet_unhash+0x96/0xd0
[ 1.561168] tcp_set_state+0x6a/0x210
[ 1.561352] tcp_abort+0x12b/0x230
[ 1.561518] bpf_prog_f4110fb1100e26b5_iter_tcp6_server+0xa3/0xaa
[ 1.561799] bpf_iter_run_prog+0x1ff/0x340
[ 1.561990] bpf_iter_tcp_seq_show+0xca/0x190
[ 1.562193] bpf_seq_read+0x177/0x450
[ 1.562363] vfs_read+0xc6/0x300
[ 1.562516] ksys_read+0x69/0xf0
[ 1.562665] do_syscall_64+0x3c/0x90
[ 1.562838] entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
>> return;
>> }
>
>> @@ -788,7 +790,8 @@ void inet_unhash(struct sock *sk)
>
>> __sk_nulls_del_node_init_rcu(sk);
>> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>> - spin_unlock(&ilb2->lock);
>> + if (!has_current_bpf_ctx())
>> + spin_unlock(&ilb2->lock);
>> } else {
>> spinlock_t *lock = inet_ehash_lockp(hashinfo, sk->sk_hash);
>
>> --
>> 2.34.1
