| Message ID | 20210721093832.78081-3-desmondcheongzx@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Bluetooth: fix inconsistent lock states | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | Link |
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(+)