| Message ID | 20210721093832.78081-3-desmondcheongzx@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: fix inconsistent lock states | expand |
Hi Desmond, > Commit fad003b6c8e3d ("Bluetooth: Fix inconsistent lock state with > RFCOMM") fixed a lockdep warning due to sk->sk_lock.slock being > acquired without disabling softirq while the lock is also used in > softirq context. This was done by disabling interrupts before calling > bh_lock_sock in rfcomm_sk_state_change. > > Later, this was changed in commit e6da0edc24ee ("Bluetooth: Acquire > sk_lock.slock without disabling interrupts") to disable softirqs > only. > > However, there is another instance of sk->sk_lock.slock being acquired > without disabling softirq in rfcomm_connect_ind. This patch fixes this > by disabling local bh before the call to bh_lock_sock. back in the days, the packet processing was done in a tasklet, but these days it is done in a workqueue. So shouldn’t this be just converted into a lock_sock(). Am I missing something? Regards Marcel
Hi Marcel, On 30/7/21 3:53 am, Marcel Holtmann wrote: > Hi Desmond, > >> Commit fad003b6c8e3d ("Bluetooth: Fix inconsistent lock state with >> RFCOMM") fixed a lockdep warning due to sk->sk_lock.slock being >> acquired without disabling softirq while the lock is also used in >> softirq context. This was done by disabling interrupts before calling >> bh_lock_sock in rfcomm_sk_state_change. >> >> Later, this was changed in commit e6da0edc24ee ("Bluetooth: Acquire >> sk_lock.slock without disabling interrupts") to disable softirqs >> only. >> >> However, there is another instance of sk->sk_lock.slock being acquired >> without disabling softirq in rfcomm_connect_ind. This patch fixes this >> by disabling local bh before the call to bh_lock_sock. > > back in the days, the packet processing was done in a tasklet, but these days it is done in a workqueue. So shouldn’t this be just converted into a lock_sock(). Am I missing something? > Thanks for the info. I think you're right, I just didn't understand very much when I wrote this patch. If I'm understanding correctly, it seems that both the bh_lock_sock in rfcomm_connect_ind, and spin_lock_bh in rfcomm_sk_state_change need to be changed to lock_sock, otherwise they don't provide any synchronization with other functions in RFCOMM that use lock_sock. If that sounds correct I can prepare the patch for that. Best wishes, Desmond
Hi Desmond, >>> Commit fad003b6c8e3d ("Bluetooth: Fix inconsistent lock state with >>> RFCOMM") fixed a lockdep warning due to sk->sk_lock.slock being >>> acquired without disabling softirq while the lock is also used in >>> softirq context. This was done by disabling interrupts before calling >>> bh_lock_sock in rfcomm_sk_state_change. >>> >>> Later, this was changed in commit e6da0edc24ee ("Bluetooth: Acquire >>> sk_lock.slock without disabling interrupts") to disable softirqs >>> only. >>> >>> However, there is another instance of sk->sk_lock.slock being acquired >>> without disabling softirq in rfcomm_connect_ind. This patch fixes this >>> by disabling local bh before the call to bh_lock_sock. >> back in the days, the packet processing was done in a tasklet, but these days it is done in a workqueue. So shouldn’t this be just converted into a lock_sock(). Am I missing something? > > Thanks for the info. I think you're right, I just didn't understand very much when I wrote this patch. > > If I'm understanding correctly, it seems that both the bh_lock_sock in rfcomm_connect_ind, and spin_lock_bh in rfcomm_sk_state_change need to be changed to lock_sock, otherwise they don't provide any synchronization with other functions in RFCOMM that use lock_sock. > > If that sounds correct I can prepare the patch for that. please do so and re-run the tests. Thanks. Regards Marcel
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c index ae6f80730561..d8734abb2df4 100644 --- a/net/bluetooth/rfcomm/sock.c +++ b/net/bluetooth/rfcomm/sock.c @@ -974,6 +974,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc * if (!parent) return 0; + local_bh_disable(); bh_lock_sock(parent); /* Check for backlog size */ @@ -1002,6 +1003,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc * done: bh_unlock_sock(parent); + local_bh_enable(); if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags)) parent->sk_state_change(parent);
Commit fad003b6c8e3d ("Bluetooth: Fix inconsistent lock state with RFCOMM") fixed a lockdep warning due to sk->sk_lock.slock being acquired without disabling softirq while the lock is also used in softirq context. This was done by disabling interrupts before calling bh_lock_sock in rfcomm_sk_state_change. Later, this was changed in commit e6da0edc24ee ("Bluetooth: Acquire sk_lock.slock without disabling interrupts") to disable softirqs only. However, there is another instance of sk->sk_lock.slock being acquired without disabling softirq in rfcomm_connect_ind. This patch fixes this by disabling local bh before the call to bh_lock_sock. Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- net/bluetooth/rfcomm/sock.c | 2 ++ 1 file changed, 2 insertions(+)