| Message ID | 45455ee45ccb3313618a48c01be714e14d372257.1687525956.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hci_conn and ISO concurrency fixes | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | fail | TestRunner_iso-tester: Total: 80, Passed: 76 (95.0%), Failed: 4, Not Run: 0 |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=759866
---Test result---
Test Summary:
CheckPatch FAIL 4.21 seconds
GitLint FAIL 2.10 seconds
SubjectPrefix PASS 0.68 seconds
BuildKernel PASS 33.21 seconds
CheckAllWarning PASS 35.89 seconds
CheckSparse PASS 40.89 seconds
CheckSmatch PASS 113.98 seconds
BuildKernel32 PASS 31.83 seconds
TestRunnerSetup PASS 450.74 seconds
TestRunner_l2cap-tester PASS 16.89 seconds
TestRunner_iso-tester FAIL 22.89 seconds
TestRunner_bnep-tester PASS 5.40 seconds
TestRunner_mgmt-tester PASS 128.09 seconds
TestRunner_rfcomm-tester PASS 8.59 seconds
TestRunner_sco-tester PASS 7.93 seconds
TestRunner_ioctl-tester PASS 9.29 seconds
TestRunner_mesh-tester PASS 6.77 seconds
TestRunner_smp-tester PASS 7.89 seconds
TestRunner_userchan-tester PASS 5.66 seconds
IncrementalBuild PASS 51.76 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[RFC,2/5] Bluetooth: hci_sync: iterate conn_hash safely in hci_disconnect_all_sync
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#102:
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
total: 0 errors, 1 warnings, 0 checks, 180 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13290840.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[RFC,3/5] Bluetooth: hci_conn: hold ref while hci_connect_le_sync is pending
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#101:
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
total: 0 errors, 1 warnings, 0 checks, 41 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13290841.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[RFC,4/5] Bluetooth: ISO: fix use-after-free in __iso_sock_close
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#106:
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
total: 0 errors, 1 warnings, 0 checks, 90 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13290842.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[RFC,2/5] Bluetooth: hci_sync: iterate conn_hash safely in hci_disconnect_all_sync
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (82>80): "[RFC,2/5] Bluetooth: hci_sync: iterate conn_hash safely in hci_disconnect_all_sync"
16: B1 Line exceeds max length (157>80): "BUG: KASAN: slab-use-after-free in hci_set_powered_sync (net/bluetooth/hci_sync.c:5345 net/bluetooth/hci_sync.c:5385 net/bluetooth/hci_sync.c:5397) bluetooth"
19: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
25: B1 Line exceeds max length (107>80): "? __virt_addr_valid (./include/linux/mmzone.h:1901 ./include/linux/mmzone.h:1997 arch/x86/mm/physaddr.c:65)"
26: B1 Line exceeds max length (124>80): "? hci_set_powered_sync (net/bluetooth/hci_sync.c:5345 net/bluetooth/hci_sync.c:5385 net/bluetooth/hci_sync.c:5397) bluetooth"
28: B1 Line exceeds max length (124>80): "? hci_set_powered_sync (net/bluetooth/hci_sync.c:5345 net/bluetooth/hci_sync.c:5385 net/bluetooth/hci_sync.c:5397) bluetooth"
29: B1 Line exceeds max length (122>80): "hci_set_powered_sync (net/bluetooth/hci_sync.c:5345 net/bluetooth/hci_sync.c:5385 net/bluetooth/hci_sync.c:5397) bluetooth"
49: B1 Line exceeds max length (108>80): "hci_conn_add (./include/linux/slab.h:559 ./include/linux/slab.h:680 net/bluetooth/hci_conn.c:1002) bluetooth"
62: B1 Line exceeds max length (85>80): "__kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244)"
65: B1 Line exceeds max length (93>80): "kobject_put (lib/kobject.c:683 lib/kobject.c:714 ./include/linux/kref.h:65 lib/kobject.c:731)"
72: B1 Line exceeds max length (117>80): "exit_to_user_mode_prepare (./include/linux/resume_user_mode.h:49 kernel/entry/common.c:171 kernel/entry/common.c:204)"
[RFC,3/5] Bluetooth: hci_conn: hold ref while hci_connect_le_sync is pending
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
15: B1 Line exceeds max length (100>80): "BUG: KASAN: slab-use-after-free in hci_le_create_conn_sync (net/bluetooth/hci_sync.c:6167) bluetooth"
18: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
24: B1 Line exceeds max length (107>80): "? __virt_addr_valid (./include/linux/mmzone.h:1901 ./include/linux/mmzone.h:1997 arch/x86/mm/physaddr.c:65)"
52: B1 Line exceeds max length (108>80): "hci_conn_add (./include/linux/slab.h:559 ./include/linux/slab.h:680 net/bluetooth/hci_conn.c:1002) bluetooth"
54: B1 Line exceeds max length (104>80): "process_adv_report.constprop.0 (net/bluetooth/hci_event.c:6172 net/bluetooth/hci_event.c:6293) bluetooth"
56: B1 Line exceeds max length (90>80): "hci_event_packet (net/bluetooth/hci_event.c:7515 net/bluetooth/hci_event.c:7570) bluetooth"
67: B1 Line exceeds max length (85>80): "__kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244)"
70: B1 Line exceeds max length (93>80): "kobject_put (lib/kobject.c:683 lib/kobject.c:714 ./include/linux/kref.h:65 lib/kobject.c:731)"
[RFC,4/5] Bluetooth: ISO: fix use-after-free in __iso_sock_close
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
20: B1 Line exceeds max length (83>80): "BUG: KASAN: slab-use-after-free in iso_conn_del (net/bluetooth/iso.c:211) bluetooth"
23: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
29: B1 Line exceeds max length (107>80): "? __virt_addr_valid (./include/linux/mmzone.h:1901 ./include/linux/mmzone.h:1997 arch/x86/mm/physaddr.c:65)"
34: B1 Line exceeds max length (101>80): "hci_conn_hash_flush (./include/net/bluetooth/hci_core.h:1872 net/bluetooth/hci_conn.c:2576) bluetooth"
37: B1 Line exceeds max length (92>80): "hci_set_powered_sync (net/bluetooth/hci_sync.c:5433 net/bluetooth/hci_sync.c:5441) bluetooth"
57: B1 Line exceeds max length (108>80): "hci_conn_add (./include/linux/slab.h:559 ./include/linux/slab.h:680 net/bluetooth/hci_conn.c:1002) bluetooth"
70: B1 Line exceeds max length (85>80): "__kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244)"
73: B1 Line exceeds max length (93>80): "kobject_put (lib/kobject.c:683 lib/kobject.c:714 ./include/linux/kref.h:65 lib/kobject.c:731)"
80: B1 Line exceeds max length (117>80): "exit_to_user_mode_prepare (./include/linux/resume_user_mode.h:49 kernel/entry/common.c:171 kernel/entry/common.c:204)"
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 80, Passed: 76 (95.0%), Failed: 4, Not Run: 0
Failed Test Cases
ISO Receive - Success Failed 0.192 seconds
ISO Receive Timestamped - Success Failed 0.181 seconds
ISO Defer Receive - Success Failed 0.183 seconds
ISO 48_2_1 Defer Receive - Success Failed 0.181 seconds
---
Regards,
Linux Bluetooth
Hi Pauli, On Fri, Jun 23, 2023 at 10:37 AM Pauli Virtanen <pav@iki.fi> wrote: > > A delayed operation such as hci_sync on a given hci_conn needs to take > hci_conn_get, so that the hci_conn doesn't get freed in the meantime. > This does not guarantee the conn is still alive in a valid state, as it > may be cleaned up in the meantime, so one needs to check if it is still > in conn_hash to know if it's still alive. > > Simplify this alive check, using HCI_CONN_DELETED flag. This is also > meaningful with RCU lock only, but with slightly different semantics. > > If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was > in conn_hash from the point of view of the current task when the flag > was read. Then its deletion cannot complete before rcu_read_unlock. > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > --- > > Notes: > This probably can be done with RCU primitives setting list.prev, but > that's maybe more magical... > > include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ > net/bluetooth/hci_conn.c | 10 +--------- > 2 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 05a9b3ab3f56..cab39bdd0592 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -978,6 +978,7 @@ enum { > HCI_CONN_PER_ADV, > HCI_CONN_BIG_CREATED, > HCI_CONN_CREATE_CIS, > + HCI_CONN_DELETED, > }; > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) > static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > { > struct hci_conn_hash *h = &hdev->conn_hash; > + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); > list_add_tail_rcu(&c->list, &h->list); > switch (c->type) { > case ACL_LINK: > @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > { > struct hci_conn_hash *h = &hdev->conn_hash; > + bool deleted; > + > + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); > + WARN_ON(deleted); > > list_del_rcu(&c->list); > synchronize_rcu(); > @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > } > } > > +/* With hdev->lock: whether hci_conn is in conn_hash. > + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and > + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in > + * this critical section it is always valid, but this may return false!) > + */ > +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) > +{ > + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), > + "suspicious locking"); > + return !test_bit(HCI_CONN_DELETED, &c->flags); > +} I think we are better off doing something like hci_conn_hold_unless_zero like we do in l2cap_chan_hold_unless_zero, that said we need to check if the hci_conn_drop can still set the ref below zero, anyway that is probably a bug in itself and we should probably WARN_ON if that happens. > static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) > { > struct hci_conn_hash *h = &hdev->conn_hash; > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index 62a7ccfdfe63..d489a4829be7 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) > struct hci_conn *conn = container_of(work, struct hci_conn, > le_scan_cleanup); > struct hci_dev *hdev = conn->hdev; > - struct hci_conn *c = NULL; > > BT_DBG("%s hcon %p", hdev->name, conn); > > hci_dev_lock(hdev); > > /* Check that the hci_conn is still around */ > - rcu_read_lock(); > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { > - if (c == conn) > - break; > - } > - rcu_read_unlock(); > - > - if (c == conn) { > + if (hci_conn_is_alive(hdev, conn)) { Hmm, I don't think this is safe, except if we are doing hci_conn_get we can't really access the conn pointer since it may be freed already, anyway this is sort of broken already given that we do access conn->hdev already. > hci_connect_le_scan_cleanup(conn, 0x00); > hci_conn_cleanup(conn); > } > -- > 2.41.0 >
Hi Luiz, pe, 2023-06-23 kello 12:39 -0700, Luiz Augusto von Dentz kirjoitti: > On Fri, Jun 23, 2023 at 10:37 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > A delayed operation such as hci_sync on a given hci_conn needs to take > > hci_conn_get, so that the hci_conn doesn't get freed in the meantime. > > This does not guarantee the conn is still alive in a valid state, as it > > may be cleaned up in the meantime, so one needs to check if it is still > > in conn_hash to know if it's still alive. > > > > Simplify this alive check, using HCI_CONN_DELETED flag. This is also > > meaningful with RCU lock only, but with slightly different semantics. > > > > If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was > > in conn_hash from the point of view of the current task when the flag > > was read. Then its deletion cannot complete before rcu_read_unlock. > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > --- > > > > Notes: > > This probably can be done with RCU primitives setting list.prev, but > > that's maybe more magical... > > > > include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ > > net/bluetooth/hci_conn.c | 10 +--------- > > 2 files changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index 05a9b3ab3f56..cab39bdd0592 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -978,6 +978,7 @@ enum { > > HCI_CONN_PER_ADV, > > HCI_CONN_BIG_CREATED, > > HCI_CONN_CREATE_CIS, > > + HCI_CONN_DELETED, > > }; > > > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > > @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) > > static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > { > > struct hci_conn_hash *h = &hdev->conn_hash; > > + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); > > list_add_tail_rcu(&c->list, &h->list); > > switch (c->type) { > > case ACL_LINK: > > @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > { > > struct hci_conn_hash *h = &hdev->conn_hash; > > + bool deleted; > > + > > + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); > > + WARN_ON(deleted); > > > > list_del_rcu(&c->list); > > synchronize_rcu(); > > @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > } > > } > > > > +/* With hdev->lock: whether hci_conn is in conn_hash. > > + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and > > + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in > > + * this critical section it is always valid, but this may return false!) > > + */ > > +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) > > +{ > > + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), > > + "suspicious locking"); > > + return !test_bit(HCI_CONN_DELETED, &c->flags); > > +} > > I think we are better off doing something like > hci_conn_hold_unless_zero like we do in l2cap_chan_hold_unless_zero, > that said we need to check if the hci_conn_drop can still set the ref > below zero, anyway that is probably a bug in itself and we should > probably WARN_ON if that happens. The problem here is that we'd like to have both (1) to have hci_conn_del/cleanup delete the item from conn_hash immediately (2) be able to continue iteration from the conn we held, after releasing and reacquiring RCU or hdev->lock If conn is removed from the list, conn->list.next won't be updated any more, so it is not safe to access after we have left the critical section. So it seems we'd need some marker on whether it is still in the list. Maybe (1) could be given up instead, something like: hci_conn_cleanup sets HCI_CONN_DELETED instead of deleting from the list if refcount is positive, and lookup functions skip items with this flag. Something along these lines could work, need to think a bit. > > static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) > > { > > struct hci_conn_hash *h = &hdev->conn_hash; > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > index 62a7ccfdfe63..d489a4829be7 100644 > > --- a/net/bluetooth/hci_conn.c > > +++ b/net/bluetooth/hci_conn.c > > @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) > > struct hci_conn *conn = container_of(work, struct hci_conn, > > le_scan_cleanup); > > struct hci_dev *hdev = conn->hdev; > > - struct hci_conn *c = NULL; > > > > BT_DBG("%s hcon %p", hdev->name, conn); > > > > hci_dev_lock(hdev); > > > > /* Check that the hci_conn is still around */ > > - rcu_read_lock(); > > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { > > - if (c == conn) > > - break; > > - } > > - rcu_read_unlock(); > > - > > - if (c == conn) { > > + if (hci_conn_is_alive(hdev, conn)) { > > Hmm, I don't think this is safe, except if we are doing hci_conn_get > we can't really access the conn pointer since it may be freed already, > anyway this is sort of broken already given that we do access > conn->hdev already. hci_conn_get is held here, there's a hci_conn_put at the end of this function. > > > hci_connect_le_scan_cleanup(conn, 0x00); > > hci_conn_cleanup(conn); > > } > > -- > > 2.41.0 > > > >
Hi Pauli, On Fri, Jun 23, 2023 at 3:21 PM Pauli Virtanen <pav@iki.fi> wrote: > > Hi Luiz, > > pe, 2023-06-23 kello 12:39 -0700, Luiz Augusto von Dentz kirjoitti: > > On Fri, Jun 23, 2023 at 10:37 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > A delayed operation such as hci_sync on a given hci_conn needs to take > > > hci_conn_get, so that the hci_conn doesn't get freed in the meantime. > > > This does not guarantee the conn is still alive in a valid state, as it > > > may be cleaned up in the meantime, so one needs to check if it is still > > > in conn_hash to know if it's still alive. > > > > > > Simplify this alive check, using HCI_CONN_DELETED flag. This is also > > > meaningful with RCU lock only, but with slightly different semantics. > > > > > > If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was > > > in conn_hash from the point of view of the current task when the flag > > > was read. Then its deletion cannot complete before rcu_read_unlock. > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > --- > > > > > > Notes: > > > This probably can be done with RCU primitives setting list.prev, but > > > that's maybe more magical... > > > > > > include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ > > > net/bluetooth/hci_conn.c | 10 +--------- > > > 2 files changed, 19 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > index 05a9b3ab3f56..cab39bdd0592 100644 > > > --- a/include/net/bluetooth/hci_core.h > > > +++ b/include/net/bluetooth/hci_core.h > > > @@ -978,6 +978,7 @@ enum { > > > HCI_CONN_PER_ADV, > > > HCI_CONN_BIG_CREATED, > > > HCI_CONN_CREATE_CIS, > > > + HCI_CONN_DELETED, > > > }; > > > > > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > > > @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) > > > static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > { > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); > > > list_add_tail_rcu(&c->list, &h->list); > > > switch (c->type) { > > > case ACL_LINK: > > > @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > { > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > + bool deleted; > > > + > > > + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); > > > + WARN_ON(deleted); > > > > > > list_del_rcu(&c->list); > > > synchronize_rcu(); > > > @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > } > > > } > > > > > > +/* With hdev->lock: whether hci_conn is in conn_hash. > > > + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and > > > + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in > > > + * this critical section it is always valid, but this may return false!) > > > + */ > > > +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) > > > +{ > > > + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), > > > + "suspicious locking"); > > > + return !test_bit(HCI_CONN_DELETED, &c->flags); > > > +} > > > > I think we are better off doing something like > > hci_conn_hold_unless_zero like we do in l2cap_chan_hold_unless_zero, > > that said we need to check if the hci_conn_drop can still set the ref > > below zero, anyway that is probably a bug in itself and we should > > probably WARN_ON if that happens. > > The problem here is that we'd like to have both > > (1) to have hci_conn_del/cleanup delete the item from conn_hash > immediately > > (2) be able to continue iteration from the conn we held, after > releasing and reacquiring RCU or hdev->lock > > If conn is removed from the list, conn->list.next won't be updated any > more, so it is not safe to access after we have left the critical > section. So it seems we'd need some marker on whether it is still in > the list. > > Maybe (1) could be given up instead, something like: hci_conn_cleanup > sets HCI_CONN_DELETED instead of deleting from the list if refcount is > positive, and lookup functions skip items with this flag. > > Something along these lines could work, need to think a bit. Ive end up reworking this logic to use something similar to what mgmt.c was doing: https://patchwork.kernel.org/project/bluetooth/patch/20230627225915.2674812-1-luiz.dentz@gmail.com/ That way we just cancel by handle and don't have to make reference left and right, we just lookup by handle if the connection is still there when the work is scheduled we abort otherwise we don't have to do anything. > > > static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) > > > { > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > > index 62a7ccfdfe63..d489a4829be7 100644 > > > --- a/net/bluetooth/hci_conn.c > > > +++ b/net/bluetooth/hci_conn.c > > > @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) > > > struct hci_conn *conn = container_of(work, struct hci_conn, > > > le_scan_cleanup); > > > struct hci_dev *hdev = conn->hdev; > > > - struct hci_conn *c = NULL; > > > > > > BT_DBG("%s hcon %p", hdev->name, conn); > > > > > > hci_dev_lock(hdev); > > > > > > /* Check that the hci_conn is still around */ > > > - rcu_read_lock(); > > > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { > > > - if (c == conn) > > > - break; > > > - } > > > - rcu_read_unlock(); > > > - > > > - if (c == conn) { > > > + if (hci_conn_is_alive(hdev, conn)) { > > > > Hmm, I don't think this is safe, except if we are doing hci_conn_get > > we can't really access the conn pointer since it may be freed already, > > anyway this is sort of broken already given that we do access > > conn->hdev already. > > hci_conn_get is held here, there's a hci_conn_put at the end of this > function. > > > > > > hci_connect_le_scan_cleanup(conn, 0x00); > > > hci_conn_cleanup(conn); > > > } > > > -- > > > 2.41.0 > > > > > > > >
Hi Luiz, ti, 2023-06-27 kello 16:05 -0700, Luiz Augusto von Dentz kirjoitti: > Hi Pauli, > > On Fri, Jun 23, 2023 at 3:21 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > Hi Luiz, > > > > pe, 2023-06-23 kello 12:39 -0700, Luiz Augusto von Dentz kirjoitti: > > > On Fri, Jun 23, 2023 at 10:37 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > > > A delayed operation such as hci_sync on a given hci_conn needs to take > > > > hci_conn_get, so that the hci_conn doesn't get freed in the meantime. > > > > This does not guarantee the conn is still alive in a valid state, as it > > > > may be cleaned up in the meantime, so one needs to check if it is still > > > > in conn_hash to know if it's still alive. > > > > > > > > Simplify this alive check, using HCI_CONN_DELETED flag. This is also > > > > meaningful with RCU lock only, but with slightly different semantics. > > > > > > > > If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was > > > > in conn_hash from the point of view of the current task when the flag > > > > was read. Then its deletion cannot complete before rcu_read_unlock. > > > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > > --- > > > > > > > > Notes: > > > > This probably can be done with RCU primitives setting list.prev, but > > > > that's maybe more magical... > > > > > > > > include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ > > > > net/bluetooth/hci_conn.c | 10 +--------- > > > > 2 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > > index 05a9b3ab3f56..cab39bdd0592 100644 > > > > --- a/include/net/bluetooth/hci_core.h > > > > +++ b/include/net/bluetooth/hci_core.h > > > > @@ -978,6 +978,7 @@ enum { > > > > HCI_CONN_PER_ADV, > > > > HCI_CONN_BIG_CREATED, > > > > HCI_CONN_CREATE_CIS, > > > > + HCI_CONN_DELETED, > > > > }; > > > > > > > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > > > > @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) > > > > static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > { > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); > > > > list_add_tail_rcu(&c->list, &h->list); > > > > switch (c->type) { > > > > case ACL_LINK: > > > > @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > { > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > + bool deleted; > > > > + > > > > + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); > > > > + WARN_ON(deleted); > > > > > > > > list_del_rcu(&c->list); > > > > synchronize_rcu(); > > > > @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > } > > > > } > > > > > > > > +/* With hdev->lock: whether hci_conn is in conn_hash. > > > > + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and > > > > + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in > > > > + * this critical section it is always valid, but this may return false!) > > > > + */ > > > > +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) > > > > +{ > > > > + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), > > > > + "suspicious locking"); > > > > + return !test_bit(HCI_CONN_DELETED, &c->flags); > > > > +} > > > > > > I think we are better off doing something like > > > hci_conn_hold_unless_zero like we do in l2cap_chan_hold_unless_zero, > > > that said we need to check if the hci_conn_drop can still set the ref > > > below zero, anyway that is probably a bug in itself and we should > > > probably WARN_ON if that happens. > > > > The problem here is that we'd like to have both > > > > (1) to have hci_conn_del/cleanup delete the item from conn_hash > > immediately > > > > (2) be able to continue iteration from the conn we held, after > > releasing and reacquiring RCU or hdev->lock > > > > If conn is removed from the list, conn->list.next won't be updated any > > more, so it is not safe to access after we have left the critical > > section. So it seems we'd need some marker on whether it is still in > > the list. > > > > Maybe (1) could be given up instead, something like: hci_conn_cleanup > > sets HCI_CONN_DELETED instead of deleting from the list if refcount is > > positive, and lookup functions skip items with this flag. > > > > Something along these lines could work, need to think a bit. > > Ive end up reworking this logic to use something similar to what > mgmt.c was doing: > > https://patchwork.kernel.org/project/bluetooth/patch/20230627225915.2674812-1-luiz.dentz@gmail.com/ > > That way we just cancel by handle and don't have to make reference > left and right, we just lookup by handle if the connection is still > there when the work is scheduled we abort otherwise we don't have to > do anything. Does this still rely on the conn not being freed concurrently, maybe to be totally sure holding rcu_read_lock/hdev->lock or having refcount would be needed around the lookup and *conn access? Unless there is something else guaranteeing the call sites of hci_conn_del cannot occur at the same time? IIUC, hci_conn_del is called also from several other places that may run concurrently if you don't lock, eg. in hci_event.c (seems to run in different workqueue than hci_sync), and I guess controller could trigger eg. HCI_Disconnection_Complete spontaneously. I'm not sure if these can be serialized behind hci_sync, if a handle is disconnected it probably needs to do some teardown immediately like unregistering it from sysfs, so that the same handle value can be reused. This is also problem for using conn->refcnt for keeping it alive: it seems we want to do partial cleanup anyway even if someone is holding the conn, so it would in the end to boil down to same as hci_conn_get. (Having hci_conn_get keep items in list would simplify iteration, but other parts seem to become more complex so what was in this RFC is maybe simplest in that direction.) > > > > > static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) > > > > { > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > > > index 62a7ccfdfe63..d489a4829be7 100644 > > > > --- a/net/bluetooth/hci_conn.c > > > > +++ b/net/bluetooth/hci_conn.c > > > > @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) > > > > struct hci_conn *conn = container_of(work, struct hci_conn, > > > > le_scan_cleanup); > > > > struct hci_dev *hdev = conn->hdev; > > > > - struct hci_conn *c = NULL; > > > > > > > > BT_DBG("%s hcon %p", hdev->name, conn); > > > > > > > > hci_dev_lock(hdev); > > > > > > > > /* Check that the hci_conn is still around */ > > > > - rcu_read_lock(); > > > > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { > > > > - if (c == conn) > > > > - break; > > > > - } > > > > - rcu_read_unlock(); > > > > - > > > > - if (c == conn) { > > > > + if (hci_conn_is_alive(hdev, conn)) { > > > > > > Hmm, I don't think this is safe, except if we are doing hci_conn_get > > > we can't really access the conn pointer since it may be freed already, > > > anyway this is sort of broken already given that we do access > > > conn->hdev already. > > > > hci_conn_get is held here, there's a hci_conn_put at the end of this > > function. > > > > > > > > > hci_connect_le_scan_cleanup(conn, 0x00); > > > > hci_conn_cleanup(conn); > > > > } > > > > -- > > > > 2.41.0 > > > > > > > > > > > > > >
Hi Pauli, On Wed, Jun 28, 2023 at 9:24 AM Pauli Virtanen <pav@iki.fi> wrote: > > Hi Luiz, > > ti, 2023-06-27 kello 16:05 -0700, Luiz Augusto von Dentz kirjoitti: > > Hi Pauli, > > > > On Fri, Jun 23, 2023 at 3:21 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > Hi Luiz, > > > > > > pe, 2023-06-23 kello 12:39 -0700, Luiz Augusto von Dentz kirjoitti: > > > > On Fri, Jun 23, 2023 at 10:37 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > > > > > A delayed operation such as hci_sync on a given hci_conn needs to take > > > > > hci_conn_get, so that the hci_conn doesn't get freed in the meantime. > > > > > This does not guarantee the conn is still alive in a valid state, as it > > > > > may be cleaned up in the meantime, so one needs to check if it is still > > > > > in conn_hash to know if it's still alive. > > > > > > > > > > Simplify this alive check, using HCI_CONN_DELETED flag. This is also > > > > > meaningful with RCU lock only, but with slightly different semantics. > > > > > > > > > > If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was > > > > > in conn_hash from the point of view of the current task when the flag > > > > > was read. Then its deletion cannot complete before rcu_read_unlock. > > > > > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > > > --- > > > > > > > > > > Notes: > > > > > This probably can be done with RCU primitives setting list.prev, but > > > > > that's maybe more magical... > > > > > > > > > > include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ > > > > > net/bluetooth/hci_conn.c | 10 +--------- > > > > > 2 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > > > index 05a9b3ab3f56..cab39bdd0592 100644 > > > > > --- a/include/net/bluetooth/hci_core.h > > > > > +++ b/include/net/bluetooth/hci_core.h > > > > > @@ -978,6 +978,7 @@ enum { > > > > > HCI_CONN_PER_ADV, > > > > > HCI_CONN_BIG_CREATED, > > > > > HCI_CONN_CREATE_CIS, > > > > > + HCI_CONN_DELETED, > > > > > }; > > > > > > > > > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > > > > > @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) > > > > > static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > > { > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); > > > > > list_add_tail_rcu(&c->list, &h->list); > > > > > switch (c->type) { > > > > > case ACL_LINK: > > > > > @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > > static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > > { > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > + bool deleted; > > > > > + > > > > > + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); > > > > > + WARN_ON(deleted); > > > > > > > > > > list_del_rcu(&c->list); > > > > > synchronize_rcu(); > > > > > @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > > } > > > > > } > > > > > > > > > > +/* With hdev->lock: whether hci_conn is in conn_hash. > > > > > + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and > > > > > + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in > > > > > + * this critical section it is always valid, but this may return false!) > > > > > + */ > > > > > +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) > > > > > +{ > > > > > + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), > > > > > + "suspicious locking"); > > > > > + return !test_bit(HCI_CONN_DELETED, &c->flags); > > > > > +} > > > > > > > > I think we are better off doing something like > > > > hci_conn_hold_unless_zero like we do in l2cap_chan_hold_unless_zero, > > > > that said we need to check if the hci_conn_drop can still set the ref > > > > below zero, anyway that is probably a bug in itself and we should > > > > probably WARN_ON if that happens. > > > > > > The problem here is that we'd like to have both > > > > > > (1) to have hci_conn_del/cleanup delete the item from conn_hash > > > immediately > > > > > > (2) be able to continue iteration from the conn we held, after > > > releasing and reacquiring RCU or hdev->lock > > > > > > If conn is removed from the list, conn->list.next won't be updated any > > > more, so it is not safe to access after we have left the critical > > > section. So it seems we'd need some marker on whether it is still in > > > the list. > > > > > > Maybe (1) could be given up instead, something like: hci_conn_cleanup > > > sets HCI_CONN_DELETED instead of deleting from the list if refcount is > > > positive, and lookup functions skip items with this flag. > > > > > > Something along these lines could work, need to think a bit. > > > > Ive end up reworking this logic to use something similar to what > > mgmt.c was doing: > > > > https://patchwork.kernel.org/project/bluetooth/patch/20230627225915.2674812-1-luiz.dentz@gmail.com/ > > > > That way we just cancel by handle and don't have to make reference > > left and right, we just lookup by handle if the connection is still > > there when the work is scheduled we abort otherwise we don't have to > > do anything. > > Does this still rely on the conn not being freed concurrently, maybe to > be totally sure holding rcu_read_lock/hdev->lock or having refcount > would be needed around the lookup and *conn access? > > Unless there is something else guaranteeing the call sites of > hci_conn_del cannot occur at the same time? > > IIUC, hci_conn_del is called also from several other places that may > run concurrently if you don't lock, eg. in hci_event.c (seems to run in > different workqueue than hci_sync), and I guess controller could > trigger eg. HCI_Disconnection_Complete spontaneously. > > I'm not sure if these can be serialized behind hci_sync, if a handle is > disconnected it probably needs to do some teardown immediately like > unregistering it from sysfs, so that the same handle value can be > reused. > > This is also problem for using conn->refcnt for keeping it alive: it > seems we want to do partial cleanup anyway even if someone is holding > the conn, so it would in the end to boil down to same as hci_conn_get. > (Having hci_conn_get keep items in list would simplify iteration, but > other parts seem to become more complex so what was in this RFC is > maybe simplest in that direction.) The idea here is that lookup by handle shall always be safe since if hci_conn_del has already been called the conn shall no longer be in the hash, now perhaps what you are afraid is that hci_conn_del is not safe to be called concurrently, which is probably correct, so perhaps we need a flag or something to check that HCI_CONN_DEL is in progress. > > > > > > > static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) > > > > > { > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > > > > index 62a7ccfdfe63..d489a4829be7 100644 > > > > > --- a/net/bluetooth/hci_conn.c > > > > > +++ b/net/bluetooth/hci_conn.c > > > > > @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) > > > > > struct hci_conn *conn = container_of(work, struct hci_conn, > > > > > le_scan_cleanup); > > > > > struct hci_dev *hdev = conn->hdev; > > > > > - struct hci_conn *c = NULL; > > > > > > > > > > BT_DBG("%s hcon %p", hdev->name, conn); > > > > > > > > > > hci_dev_lock(hdev); > > > > > > > > > > /* Check that the hci_conn is still around */ > > > > > - rcu_read_lock(); > > > > > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { > > > > > - if (c == conn) > > > > > - break; > > > > > - } > > > > > - rcu_read_unlock(); > > > > > - > > > > > - if (c == conn) { > > > > > + if (hci_conn_is_alive(hdev, conn)) { > > > > > > > > Hmm, I don't think this is safe, except if we are doing hci_conn_get > > > > we can't really access the conn pointer since it may be freed already, > > > > anyway this is sort of broken already given that we do access > > > > conn->hdev already. > > > > > > hci_conn_get is held here, there's a hci_conn_put at the end of this > > > function. > > > > > > > > > > > > hci_connect_le_scan_cleanup(conn, 0x00); > > > > > hci_conn_cleanup(conn); > > > > > } > > > > > -- > > > > > 2.41.0 > > > > > > > > > > > > > > > > > > > > >
ke, 2023-06-28 kello 12:29 -0700, Luiz Augusto von Dentz kirjoitti: > Hi Pauli, > > On Wed, Jun 28, 2023 at 9:24 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > Hi Luiz, > > > > ti, 2023-06-27 kello 16:05 -0700, Luiz Augusto von Dentz kirjoitti: > > > Hi Pauli, > > > > > > On Fri, Jun 23, 2023 at 3:21 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > > > Hi Luiz, > > > > > > > > pe, 2023-06-23 kello 12:39 -0700, Luiz Augusto von Dentz kirjoitti: > > > > > On Fri, Jun 23, 2023 at 10:37 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > > > > > > > A delayed operation such as hci_sync on a given hci_conn needs to take > > > > > > hci_conn_get, so that the hci_conn doesn't get freed in the meantime. > > > > > > This does not guarantee the conn is still alive in a valid state, as it > > > > > > may be cleaned up in the meantime, so one needs to check if it is still > > > > > > in conn_hash to know if it's still alive. > > > > > > > > > > > > Simplify this alive check, using HCI_CONN_DELETED flag. This is also > > > > > > meaningful with RCU lock only, but with slightly different semantics. > > > > > > > > > > > > If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was > > > > > > in conn_hash from the point of view of the current task when the flag > > > > > > was read. Then its deletion cannot complete before rcu_read_unlock. > > > > > > > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > > > > --- > > > > > > > > > > > > Notes: > > > > > > This probably can be done with RCU primitives setting list.prev, but > > > > > > that's maybe more magical... > > > > > > > > > > > > include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ > > > > > > net/bluetooth/hci_conn.c | 10 +--------- > > > > > > 2 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > > > > index 05a9b3ab3f56..cab39bdd0592 100644 > > > > > > --- a/include/net/bluetooth/hci_core.h > > > > > > +++ b/include/net/bluetooth/hci_core.h > > > > > > @@ -978,6 +978,7 @@ enum { > > > > > > HCI_CONN_PER_ADV, > > > > > > HCI_CONN_BIG_CREATED, > > > > > > HCI_CONN_CREATE_CIS, > > > > > > + HCI_CONN_DELETED, > > > > > > }; > > > > > > > > > > > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > > > > > > @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) > > > > > > static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > > > { > > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > > + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); > > > > > > list_add_tail_rcu(&c->list, &h->list); > > > > > > switch (c->type) { > > > > > > case ACL_LINK: > > > > > > @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > > > static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > > > { > > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > > + bool deleted; > > > > > > + > > > > > > + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); > > > > > > + WARN_ON(deleted); > > > > > > > > > > > > list_del_rcu(&c->list); > > > > > > synchronize_rcu(); > > > > > > @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > > > } > > > > > > } > > > > > > > > > > > > +/* With hdev->lock: whether hci_conn is in conn_hash. > > > > > > + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and > > > > > > + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in > > > > > > + * this critical section it is always valid, but this may return false!) > > > > > > + */ > > > > > > +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) > > > > > > +{ > > > > > > + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), > > > > > > + "suspicious locking"); > > > > > > + return !test_bit(HCI_CONN_DELETED, &c->flags); > > > > > > +} > > > > > > > > > > I think we are better off doing something like > > > > > hci_conn_hold_unless_zero like we do in l2cap_chan_hold_unless_zero, > > > > > that said we need to check if the hci_conn_drop can still set the ref > > > > > below zero, anyway that is probably a bug in itself and we should > > > > > probably WARN_ON if that happens. > > > > > > > > The problem here is that we'd like to have both > > > > > > > > (1) to have hci_conn_del/cleanup delete the item from conn_hash > > > > immediately > > > > > > > > (2) be able to continue iteration from the conn we held, after > > > > releasing and reacquiring RCU or hdev->lock > > > > > > > > If conn is removed from the list, conn->list.next won't be updated any > > > > more, so it is not safe to access after we have left the critical > > > > section. So it seems we'd need some marker on whether it is still in > > > > the list. > > > > > > > > Maybe (1) could be given up instead, something like: hci_conn_cleanup > > > > sets HCI_CONN_DELETED instead of deleting from the list if refcount is > > > > positive, and lookup functions skip items with this flag. > > > > > > > > Something along these lines could work, need to think a bit. > > > > > > Ive end up reworking this logic to use something similar to what > > > mgmt.c was doing: > > > > > > https://patchwork.kernel.org/project/bluetooth/patch/20230627225915.2674812-1-luiz.dentz@gmail.com/ > > > > > > That way we just cancel by handle and don't have to make reference > > > left and right, we just lookup by handle if the connection is still > > > there when the work is scheduled we abort otherwise we don't have to > > > do anything. > > > > Does this still rely on the conn not being freed concurrently, maybe to > > be totally sure holding rcu_read_lock/hdev->lock or having refcount > > would be needed around the lookup and *conn access? > > > > Unless there is something else guaranteeing the call sites of > > hci_conn_del cannot occur at the same time? > > > > IIUC, hci_conn_del is called also from several other places that may > > run concurrently if you don't lock, eg. in hci_event.c (seems to run in > > different workqueue than hci_sync), and I guess controller could > > trigger eg. HCI_Disconnection_Complete spontaneously. > > > > I'm not sure if these can be serialized behind hci_sync, if a handle is > > disconnected it probably needs to do some teardown immediately like > > unregistering it from sysfs, so that the same handle value can be > > reused. > > > > This is also problem for using conn->refcnt for keeping it alive: it > > seems we want to do partial cleanup anyway even if someone is holding > > the conn, so it would in the end to boil down to same as hci_conn_get. > > (Having hci_conn_get keep items in list would simplify iteration, but > > other parts seem to become more complex so what was in this RFC is > > maybe simplest in that direction.) > > The idea here is that lookup by handle shall always be safe since if > hci_conn_del has already been called the conn shall no longer be in > the hash, now perhaps what you are afraid is that hci_conn_del is not > safe to be called concurrently, which is probably correct, so perhaps > we need a flag or something to check that HCI_CONN_DEL is in progress. For clarification, I mean here that constructs like conn = hci_conn_hash_lookup_handle(hdev, handle); if (!conn) return 0; do_something(conn); might not be safe if e.g. hci_event.c is doing hci_conn_del on another CPU at the same time (it appears to be in hdev->workqueue, cmd_sync in req_workqueue so I'm not sure they are serialized), and frees conn after hci_conn_hash_lookup_handle exits the critical section, before/during do_something runs. Something like rcu_read_lock(); conn = hci_conn_hash_lookup_handle(hdev, handle); if (conn) handle = conn->handle; rcu_read_unlock(); if (!conn) return 0; do_something(handle) could avoid that, or holding hdev->lock (IIUC in hci_sync callbacks it is not automatically held). Or, maybe there is some other guarantee that can be assumed here, and I'm missing something about how the locking/scheduling works here (don't know this well enough). The other comment was about if hci_cmd_sync_cancel(hdev, -ECANCELED); could cancel the wrong command. Note conn->state is set to BT_CONNECT in hci_connect_le, before hci_connect_le_sync sends its command, so if there was already another command in the hci_sync queue belonging to a different connection, it seems hci_cmd_sync_cancel might here cancel the wrong one. > > > > > > > > > > static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) > > > > > > { > > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > > > > > index 62a7ccfdfe63..d489a4829be7 100644 > > > > > > --- a/net/bluetooth/hci_conn.c > > > > > > +++ b/net/bluetooth/hci_conn.c > > > > > > @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) > > > > > > struct hci_conn *conn = container_of(work, struct hci_conn, > > > > > > le_scan_cleanup); > > > > > > struct hci_dev *hdev = conn->hdev; > > > > > > - struct hci_conn *c = NULL; > > > > > > > > > > > > BT_DBG("%s hcon %p", hdev->name, conn); > > > > > > > > > > > > hci_dev_lock(hdev); > > > > > > > > > > > > /* Check that the hci_conn is still around */ > > > > > > - rcu_read_lock(); > > > > > > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { > > > > > > - if (c == conn) > > > > > > - break; > > > > > > - } > > > > > > - rcu_read_unlock(); > > > > > > - > > > > > > - if (c == conn) { > > > > > > + if (hci_conn_is_alive(hdev, conn)) { > > > > > > > > > > Hmm, I don't think this is safe, except if we are doing hci_conn_get > > > > > we can't really access the conn pointer since it may be freed already, > > > > > anyway this is sort of broken already given that we do access > > > > > conn->hdev already. > > > > > > > > hci_conn_get is held here, there's a hci_conn_put at the end of this > > > > function. > > > > > > > > > > > > > > > hci_connect_le_scan_cleanup(conn, 0x00); > > > > > > hci_conn_cleanup(conn); > > > > > > } > > > > > > -- > > > > > > 2.41.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Hi Pauli, On Wed, Jun 28, 2023 at 1:11 PM Pauli Virtanen <pav@iki.fi> wrote: > > ke, 2023-06-28 kello 12:29 -0700, Luiz Augusto von Dentz kirjoitti: > > Hi Pauli, > > > > On Wed, Jun 28, 2023 at 9:24 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > Hi Luiz, > > > > > > ti, 2023-06-27 kello 16:05 -0700, Luiz Augusto von Dentz kirjoitti: > > > > Hi Pauli, > > > > > > > > On Fri, Jun 23, 2023 at 3:21 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > > > > > Hi Luiz, > > > > > > > > > > pe, 2023-06-23 kello 12:39 -0700, Luiz Augusto von Dentz kirjoitti: > > > > > > On Fri, Jun 23, 2023 at 10:37 AM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > > > > > > > > > A delayed operation such as hci_sync on a given hci_conn needs to take > > > > > > > hci_conn_get, so that the hci_conn doesn't get freed in the meantime. > > > > > > > This does not guarantee the conn is still alive in a valid state, as it > > > > > > > may be cleaned up in the meantime, so one needs to check if it is still > > > > > > > in conn_hash to know if it's still alive. > > > > > > > > > > > > > > Simplify this alive check, using HCI_CONN_DELETED flag. This is also > > > > > > > meaningful with RCU lock only, but with slightly different semantics. > > > > > > > > > > > > > > If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was > > > > > > > in conn_hash from the point of view of the current task when the flag > > > > > > > was read. Then its deletion cannot complete before rcu_read_unlock. > > > > > > > > > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > > > > > --- > > > > > > > > > > > > > > Notes: > > > > > > > This probably can be done with RCU primitives setting list.prev, but > > > > > > > that's maybe more magical... > > > > > > > > > > > > > > include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ > > > > > > > net/bluetooth/hci_conn.c | 10 +--------- > > > > > > > 2 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > > > > > index 05a9b3ab3f56..cab39bdd0592 100644 > > > > > > > --- a/include/net/bluetooth/hci_core.h > > > > > > > +++ b/include/net/bluetooth/hci_core.h > > > > > > > @@ -978,6 +978,7 @@ enum { > > > > > > > HCI_CONN_PER_ADV, > > > > > > > HCI_CONN_BIG_CREATED, > > > > > > > HCI_CONN_CREATE_CIS, > > > > > > > + HCI_CONN_DELETED, > > > > > > > }; > > > > > > > > > > > > > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > > > > > > > @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) > > > > > > > static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > > > > { > > > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > > > + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); > > > > > > > list_add_tail_rcu(&c->list, &h->list); > > > > > > > switch (c->type) { > > > > > > > case ACL_LINK: > > > > > > > @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > > > > > > static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > > > > { > > > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > > > + bool deleted; > > > > > > > + > > > > > > > + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); > > > > > > > + WARN_ON(deleted); > > > > > > > > > > > > > > list_del_rcu(&c->list); > > > > > > > synchronize_rcu(); > > > > > > > @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > +/* With hdev->lock: whether hci_conn is in conn_hash. > > > > > > > + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and > > > > > > > + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in > > > > > > > + * this critical section it is always valid, but this may return false!) > > > > > > > + */ > > > > > > > +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) > > > > > > > +{ > > > > > > > + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), > > > > > > > + "suspicious locking"); > > > > > > > + return !test_bit(HCI_CONN_DELETED, &c->flags); > > > > > > > +} > > > > > > > > > > > > I think we are better off doing something like > > > > > > hci_conn_hold_unless_zero like we do in l2cap_chan_hold_unless_zero, > > > > > > that said we need to check if the hci_conn_drop can still set the ref > > > > > > below zero, anyway that is probably a bug in itself and we should > > > > > > probably WARN_ON if that happens. > > > > > > > > > > The problem here is that we'd like to have both > > > > > > > > > > (1) to have hci_conn_del/cleanup delete the item from conn_hash > > > > > immediately > > > > > > > > > > (2) be able to continue iteration from the conn we held, after > > > > > releasing and reacquiring RCU or hdev->lock > > > > > > > > > > If conn is removed from the list, conn->list.next won't be updated any > > > > > more, so it is not safe to access after we have left the critical > > > > > section. So it seems we'd need some marker on whether it is still in > > > > > the list. > > > > > > > > > > Maybe (1) could be given up instead, something like: hci_conn_cleanup > > > > > sets HCI_CONN_DELETED instead of deleting from the list if refcount is > > > > > positive, and lookup functions skip items with this flag. > > > > > > > > > > Something along these lines could work, need to think a bit. > > > > > > > > Ive end up reworking this logic to use something similar to what > > > > mgmt.c was doing: > > > > > > > > https://patchwork.kernel.org/project/bluetooth/patch/20230627225915.2674812-1-luiz.dentz@gmail.com/ > > > > > > > > That way we just cancel by handle and don't have to make reference > > > > left and right, we just lookup by handle if the connection is still > > > > there when the work is scheduled we abort otherwise we don't have to > > > > do anything. > > > > > > Does this still rely on the conn not being freed concurrently, maybe to > > > be totally sure holding rcu_read_lock/hdev->lock or having refcount > > > would be needed around the lookup and *conn access? > > > > > > Unless there is something else guaranteeing the call sites of > > > hci_conn_del cannot occur at the same time? > > > > > > IIUC, hci_conn_del is called also from several other places that may > > > run concurrently if you don't lock, eg. in hci_event.c (seems to run in > > > different workqueue than hci_sync), and I guess controller could > > > trigger eg. HCI_Disconnection_Complete spontaneously. > > > > > > I'm not sure if these can be serialized behind hci_sync, if a handle is > > > disconnected it probably needs to do some teardown immediately like > > > unregistering it from sysfs, so that the same handle value can be > > > reused. > > > > > > This is also problem for using conn->refcnt for keeping it alive: it > > > seems we want to do partial cleanup anyway even if someone is holding > > > the conn, so it would in the end to boil down to same as hci_conn_get. > > > (Having hci_conn_get keep items in list would simplify iteration, but > > > other parts seem to become more complex so what was in this RFC is > > > maybe simplest in that direction.) > > > > The idea here is that lookup by handle shall always be safe since if > > hci_conn_del has already been called the conn shall no longer be in > > the hash, now perhaps what you are afraid is that hci_conn_del is not > > safe to be called concurrently, which is probably correct, so perhaps > > we need a flag or something to check that HCI_CONN_DEL is in progress. > > For clarification, I mean here that constructs like > > conn = hci_conn_hash_lookup_handle(hdev, handle); > if (!conn) > return 0; > do_something(conn); > > might not be safe if e.g. hci_event.c is doing hci_conn_del on another > CPU at the same time (it appears to be in hdev->workqueue, cmd_sync in > req_workqueue so I'm not sure they are serialized), and frees conn > after hci_conn_hash_lookup_handle exits the critical section, > before/during do_something runs. Something like > > rcu_read_lock(); > conn = hci_conn_hash_lookup_handle(hdev, handle); > if (conn) > handle = conn->handle; > rcu_read_unlock(); > if (!conn) > return 0; > do_something(handle) Perhaps we need some tests that cover these scenarios, for the most part all commands shall be serialized, so if the cmd_sync is doing hci_conn_del while the receive/event work is doing it as well then we have a bug in the code path. Btw, hci_conn_hash_lookup_handle does rcu_read_lock already. > could avoid that, or holding hdev->lock (IIUC in hci_sync callbacks it > is not automatically held). > > Or, maybe there is some other guarantee that can be assumed here, and > I'm missing something about how the locking/scheduling works here > (don't know this well enough). > > The other comment was about if > > hci_cmd_sync_cancel(hdev, -ECANCELED); > > could cancel the wrong command. Note conn->state is set to BT_CONNECT > in hci_connect_le, before hci_connect_le_sync sends its command, so if > there was already another command in the hci_sync queue belonging to a > different connection, it seems hci_cmd_sync_cancel might here cancel > the wrong one. Right, Im planning to introduce the following changes: https://gist.github.com/Vudentz/1dd1525980c200bf9617ae002e0eb1ea > > > > > > > > > > > > > static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) > > > > > > > { > > > > > > > struct hci_conn_hash *h = &hdev->conn_hash; > > > > > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > > > > > > index 62a7ccfdfe63..d489a4829be7 100644 > > > > > > > --- a/net/bluetooth/hci_conn.c > > > > > > > +++ b/net/bluetooth/hci_conn.c > > > > > > > @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) > > > > > > > struct hci_conn *conn = container_of(work, struct hci_conn, > > > > > > > le_scan_cleanup); > > > > > > > struct hci_dev *hdev = conn->hdev; > > > > > > > - struct hci_conn *c = NULL; > > > > > > > > > > > > > > BT_DBG("%s hcon %p", hdev->name, conn); > > > > > > > > > > > > > > hci_dev_lock(hdev); > > > > > > > > > > > > > > /* Check that the hci_conn is still around */ > > > > > > > - rcu_read_lock(); > > > > > > > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { > > > > > > > - if (c == conn) > > > > > > > - break; > > > > > > > - } > > > > > > > - rcu_read_unlock(); > > > > > > > - > > > > > > > - if (c == conn) { > > > > > > > + if (hci_conn_is_alive(hdev, conn)) { > > > > > > > > > > > > Hmm, I don't think this is safe, except if we are doing hci_conn_get > > > > > > we can't really access the conn pointer since it may be freed already, > > > > > > anyway this is sort of broken already given that we do access > > > > > > conn->hdev already. > > > > > > > > > > hci_conn_get is held here, there's a hci_conn_put at the end of this > > > > > function. > > > > > > > > > > > > > > > > > > hci_connect_le_scan_cleanup(conn, 0x00); > > > > > > > hci_conn_cleanup(conn); > > > > > > > } > > > > > > > -- > > > > > > > 2.41.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 05a9b3ab3f56..cab39bdd0592 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -978,6 +978,7 @@ enum { HCI_CONN_PER_ADV, HCI_CONN_BIG_CREATED, HCI_CONN_CREATE_CIS, + HCI_CONN_DELETED, }; static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) @@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn) static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) { struct hci_conn_hash *h = &hdev->conn_hash; + WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags)); list_add_tail_rcu(&c->list, &h->list); switch (c->type) { case ACL_LINK: @@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) { struct hci_conn_hash *h = &hdev->conn_hash; + bool deleted; + + deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags); + WARN_ON(deleted); list_del_rcu(&c->list); synchronize_rcu(); @@ -1049,6 +1055,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) } } +/* With hdev->lock: whether hci_conn is in conn_hash. + * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and + * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in + * this critical section it is always valid, but this may return false!) + */ +static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c) +{ + RCU_LOCKDEP_WARN(lockdep_is_held(&hdev->lock) || rcu_read_lock_held(), + "suspicious locking"); + return !test_bit(HCI_CONN_DELETED, &c->flags); +} + static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type) { struct hci_conn_hash *h = &hdev->conn_hash; diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 62a7ccfdfe63..d489a4829be7 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work) struct hci_conn *conn = container_of(work, struct hci_conn, le_scan_cleanup); struct hci_dev *hdev = conn->hdev; - struct hci_conn *c = NULL; BT_DBG("%s hcon %p", hdev->name, conn); hci_dev_lock(hdev); /* Check that the hci_conn is still around */ - rcu_read_lock(); - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) { - if (c == conn) - break; - } - rcu_read_unlock(); - - if (c == conn) { + if (hci_conn_is_alive(hdev, conn)) { hci_connect_le_scan_cleanup(conn, 0x00); hci_conn_cleanup(conn); }
A delayed operation such as hci_sync on a given hci_conn needs to take hci_conn_get, so that the hci_conn doesn't get freed in the meantime. This does not guarantee the conn is still alive in a valid state, as it may be cleaned up in the meantime, so one needs to check if it is still in conn_hash to know if it's still alive. Simplify this alive check, using HCI_CONN_DELETED flag. This is also meaningful with RCU lock only, but with slightly different semantics. If hci_conn_is_alive(conn) returns true inside rcu_read_lock, conn was in conn_hash from the point of view of the current task when the flag was read. Then its deletion cannot complete before rcu_read_unlock. Signed-off-by: Pauli Virtanen <pav@iki.fi> --- Notes: This probably can be done with RCU primitives setting list.prev, but that's maybe more magical... include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++ net/bluetooth/hci_conn.c | 10 +--------- 2 files changed, 19 insertions(+), 9 deletions(-)