2023-07-26 21:45:47

by Pauli Virtanen

[permalink] [raw]
Subject: [PATCH RFC 0/6] Locking in hci_sync

The current guarantees that a given hci_conn is not freed concurrently
appear to be:

- hci_dev_lock is held, or,
- rcu_read_lock is held (doesn't guarantee valid conn state), or,
- hci_conn_get refcount held (doesn't guarantee valid conn state),
- current task is running from hdev->workqueue (which is ordered)

The last condition is not exactly true as hci_conn_del is currently
called also elsewhere, but is assumed in hci_event.c and hci_core.c.

It does not appear possible to assume in hdev->req_workqueue, especially
in hci_sync callbacks, that a given conn stays alive at any time if none
of the above precautions are taken. Similarly, any conn_hash iteration
without locks there appears invalid.

E.g. Disconnect events emitted from remote appear possible to occur at
any time in hci_sync. There are also some hci_conn_del callsites not in
hdev->workqueue which can run concurrently, eg in __iso_sock_close
(which also doesn't hold hdev->lock).

KASAN crashes are known on real HW, and the deletion race conditions can
be hit in VHCI (see the other BlueZ tester patch series).

***

What to do?

One way to guard against using already freed conns is hci_conn_get +
check that the connection was not deleted, inside suitable critical
section. I didn't find a reliable existing check if hci_conn_del was
run.

To enable using hci_conn_get, the series here suggests again adding
HCI_CONN_DELETED flag to indicate whether hci_conn_del has run (with
hdev->lock or in hdev->workqueue).

It also adds some helpers to make writing hci_sync callbacks that
operate on a given hci_conn or need hdev->lock easier to write. The
patches here add hci_conn_sync_queue, which check if hci_conn was
deleted in meantime, and hold hdev->lock in the callback (but release it
during waits), so that concurrent modification of hci_conn can only
happen during event waits. This is something that is already assumed but
might not be true eg. if the two workqueues run jobs on different CPU.

The last two patches in the series are some motivating ISO related
changes, for proper cleanup of CIS, lookup by handles doesn't quite
work (and also doesn't protect against the deletion race).

***

I tried to find some alternatives, but this seemed minimal one.

I don't see how to make use of hci_conn_drop/hold here, as they appear
to have different purpose, if we change that then how socket channels
work seem to a new mechanism. E.g. nonzero refcnt from socket should not
keep connection alive when eg. Disconnect event from remote occurs. In
that case we also need to clean up the connection ASAP so the handle can
be reused.

One alternative that makes continuing conn_hash iteration a bit simpler
would be to remove hci_conn from conn_hash only when hci_conn_get
refcount hits zero, so holding hci_conn_get reference would keep the
conn a valid iteration cursor. Also setting conn->type to INVALID_LINK
on deletion could then make lookup functions skip deleted conns.
However, one would need to take a look at all places where conn_hash is
iterated and think it through.

It maybe could also be possible to not allow events to be processed
while hci_sync is running, except when it is waiting for an event (=
more or less, take hci_cmd_sync_dev_lock in all callbacks so hdev->lock
serializes things). However, it seems a deletion flag would be needed
also here, as the conn might be gone while we are waiting for events.

Pauli Virtanen (6):
Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del
Bluetooth: hci_conn: add hci_conn_is_alive
Bluetooth: hci_sync: add hci_conn_sync_queue and
hci_cmd_sync_dev_(un)lock
Bluetooth: hci_sync: fix locking in hci_conn_abort and
hci_disconnect_all_sync
Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting
Bluetooth: ISO: handle bound CIS cleanup via hci_conn

include/net/bluetooth/hci_core.h | 22 +++++
include/net/bluetooth/hci_sync.h | 3 +
net/bluetooth/hci_conn.c | 157 ++++++++++++++++++++-----------
net/bluetooth/hci_sync.c | 97 +++++++++++++++----
net/bluetooth/iso.c | 14 +--
5 files changed, 211 insertions(+), 82 deletions(-)

--
2.41.0



2023-07-26 21:52:29

by Pauli Virtanen

[permalink] [raw]
Subject: [PATCH RFC 1/6] Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del

hci_conn_cleanup is no longer needed, so move the code back to
hci_conn_del to keep the hci_conn teardown in a single place.

This undoes commit b958f9a3e877 ("Bluetooth: Fix reference counting for
LE-scan based connections"), but keeps the current order of cleanup
operations.

Signed-off-by: Pauli Virtanen <[email protected]>
---
net/bluetooth/hci_conn.c | 78 +++++++++++++++++-----------------------
1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index cccc2b8b60a8..a71a54a5c8d8 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -139,45 +139,6 @@ static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
hci_update_passive_scan(hdev);
}

-static void hci_conn_cleanup(struct hci_conn *conn)
-{
- struct hci_dev *hdev = conn->hdev;
-
- if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
- hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
-
- if (test_and_clear_bit(HCI_CONN_FLUSH_KEY, &conn->flags))
- hci_remove_link_key(hdev, &conn->dst);
-
- hci_chan_list_flush(conn);
-
- hci_conn_hash_del(hdev, conn);
-
- if (conn->cleanup)
- conn->cleanup(conn);
-
- if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
- switch (conn->setting & SCO_AIRMODE_MASK) {
- case SCO_AIRMODE_CVSD:
- case SCO_AIRMODE_TRANSP:
- if (hdev->notify)
- hdev->notify(hdev, HCI_NOTIFY_DISABLE_SCO);
- break;
- }
- } else {
- if (hdev->notify)
- hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
- }
-
- hci_conn_del_sysfs(conn);
-
- debugfs_remove_recursive(conn->debugfs);
-
- hci_dev_put(hdev);
-
- hci_conn_put(conn);
-}
-
static void hci_acl_create_connection(struct hci_conn *conn)
{
struct hci_dev *hdev = conn->hdev;
@@ -1127,12 +1088,39 @@ void hci_conn_del(struct hci_conn *conn)

skb_queue_purge(&conn->data_q);

- /* Remove the connection from the list and cleanup its remaining
- * state. This is a separate function since for some cases like
- * BT_CONNECT_SCAN we *only* want the cleanup part without the
- * rest of hci_conn_del.
- */
- hci_conn_cleanup(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
+ if (test_and_clear_bit(HCI_CONN_FLUSH_KEY, &conn->flags))
+ hci_remove_link_key(hdev, &conn->dst);
+
+ hci_chan_list_flush(conn);
+
+ hci_conn_hash_del(hdev, conn);
+
+ if (conn->cleanup)
+ conn->cleanup(conn);
+
+ if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
+ switch (conn->setting & SCO_AIRMODE_MASK) {
+ case SCO_AIRMODE_CVSD:
+ case SCO_AIRMODE_TRANSP:
+ if (hdev->notify)
+ hdev->notify(hdev, HCI_NOTIFY_DISABLE_SCO);
+ break;
+ }
+ } else {
+ if (hdev->notify)
+ hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
+ }
+
+ hci_conn_del_sysfs(conn);
+
+ debugfs_remove_recursive(conn->debugfs);
+
+ hci_dev_put(hdev);
+
+ hci_conn_put(conn);
}

struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
--
2.41.0


2023-07-26 21:53:49

by Pauli Virtanen

[permalink] [raw]
Subject: [PATCH RFC 3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock

Operations on a given hci_conn in hci_sync are hard to write safely,
because the conn might be deleted or modified unexpectedly either
concurrently or when waiting for the events. Holding hci_dev_lock in
the sync callbacks is also inconvenient since it has to be manually
released before entering the event waits.

Add convenience utilities to make things easier:

Add hci_cmd_sync_dev_lock/unlock, for easier writing of hci_sync
callbacks where hci_dev_lock should be held. The lock is however still
released and reacquired around request waits. Callbacks using them can
then assume that state changes protected by hci_dev_lock can only occur
when waiting for events. (This is currently assumed in many of the
callbacks.)

Add hci_conn_sync_queue/submit, whose callback on entry holds
hci_dev_lock and the hci_conn has not been deleted. If it was deleted
while the sync command was queued, the destroy routine has err -ENODEV.
Similarly, synchronous commands called in the callback fail with ENODEV
if the conn was deleted during the wait.

Signed-off-by: Pauli Virtanen <[email protected]>
---
include/net/bluetooth/hci_core.h | 7 ++++
include/net/bluetooth/hci_sync.h | 3 ++
net/bluetooth/hci_conn.c | 60 ++++++++++++++++++++++++++++++++
net/bluetooth/hci_sync.c | 31 +++++++++++++++++
4 files changed, 101 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 34e4ad7c32e7..8e218300ef4e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -519,6 +519,9 @@ struct hci_dev {
struct work_struct cmd_sync_cancel_work;
struct work_struct reenable_adv_work;

+ bool cmd_sync_locked;
+ struct hci_conn *cmd_sync_conn;
+
__u16 discov_timeout;
struct delayed_work discov_off;

@@ -1400,6 +1403,10 @@ void hci_conn_del(struct hci_conn *conn);
void hci_conn_hash_flush(struct hci_dev *hdev);
void hci_conn_check_pending(struct hci_dev *hdev);

+void hci_conn_sync_set_conn(struct hci_dev *hdev, struct hci_conn *conn);
+int hci_conn_sync_queue(struct hci_conn *conn, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy);
+
struct hci_chan *hci_chan_create(struct hci_conn *conn);
void hci_chan_del(struct hci_chan *chan);
void hci_chan_list_flush(struct hci_conn *conn);
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index b516a0f4a55b..a9a94950d523 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -46,6 +46,9 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);

+void hci_cmd_sync_dev_lock(struct hci_dev *hdev);
+void hci_cmd_sync_dev_unlock(struct hci_dev *hdev);
+
int hci_update_eir_sync(struct hci_dev *hdev);
int hci_update_class_sync(struct hci_dev *hdev);

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ee304618bf0a..208eb5eea451 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2889,3 +2889,63 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
return hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
NULL);
}
+
+struct hci_conn_sync_work_entry {
+ struct hci_conn *conn;
+ hci_cmd_sync_work_func_t func;
+ void *data;
+ hci_cmd_sync_work_destroy_t destroy;
+};
+
+static int hci_conn_sync_work_run(struct hci_dev *hdev, void *data)
+{
+ struct hci_conn_sync_work_entry *entry = data;
+ int err;
+
+ hci_cmd_sync_dev_lock(hdev);
+ hdev->cmd_sync_conn = entry->conn;
+
+ if (hci_conn_is_alive(hdev, entry->conn))
+ err = entry->func(hdev, entry->data);
+ else
+ err = -ENODEV;
+
+ hdev->cmd_sync_conn = NULL;
+ hci_cmd_sync_dev_unlock(hdev);
+
+ return err;
+}
+
+static void hci_conn_sync_work_destroy(struct hci_dev *hdev, void *data,
+ int err)
+{
+ struct hci_conn_sync_work_entry *entry = data;
+
+ if (entry->destroy)
+ entry->destroy(hdev, entry->data, err);
+ hci_conn_put(entry->conn);
+ kfree(entry);
+}
+
+int hci_conn_sync_queue(struct hci_conn *conn, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy)
+{
+ struct hci_conn_sync_work_entry *entry;
+ int err;
+
+ entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ entry->func = func;
+ entry->data = data;
+ entry->destroy = destroy;
+ entry->conn = hci_conn_get(conn);
+
+ err = hci_cmd_sync_queue(conn->hdev, hci_conn_sync_work_run, entry,
+ hci_conn_sync_work_destroy);
+ if (err)
+ kfree(entry);
+
+ return err;
+}
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 3348a1b0e3f7..6a295a9e1f5d 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -164,10 +164,16 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
if (err < 0)
return ERR_PTR(err);

+ if (hdev->cmd_sync_locked)
+ hci_dev_unlock(hdev);
+
err = wait_event_interruptible_timeout(hdev->req_wait_q,
hdev->req_status != HCI_REQ_PEND,
timeout);

+ if (hdev->cmd_sync_locked)
+ hci_dev_lock(hdev);
+
if (err == -ERESTARTSYS)
return ERR_PTR(-EINTR);

@@ -185,6 +191,11 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
break;
}

+ if (hdev->cmd_sync_conn) {
+ if (!hci_conn_is_alive(hdev, hdev->cmd_sync_conn))
+ err = -ENODEV;
+ }
+
hdev->req_status = 0;
hdev->req_result = 0;
skb = hdev->req_skb;
@@ -740,6 +751,26 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
}
EXPORT_SYMBOL(hci_cmd_sync_queue);

+void hci_cmd_sync_dev_lock(struct hci_dev *hdev)
+{
+ lockdep_assert_held(&hdev->req_lock);
+
+ hci_dev_lock(hdev);
+
+ WARN_ON_ONCE(hdev->cmd_sync_locked);
+ hdev->cmd_sync_locked = true;
+}
+
+void hci_cmd_sync_dev_unlock(struct hci_dev *hdev)
+{
+ lockdep_assert_held(&hdev->req_lock);
+
+ WARN_ON_ONCE(!hdev->cmd_sync_locked);
+ hdev->cmd_sync_locked = false;
+
+ hci_dev_unlock(hdev);
+}
+
int hci_update_eir_sync(struct hci_dev *hdev)
{
struct hci_cp_write_eir cp;
--
2.41.0


2023-07-26 21:54:22

by Pauli Virtanen

[permalink] [raw]
Subject: [PATCH RFC 4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and hci_disconnect_all_sync

hci_conn_abort may run concurrently with operations that e.g. change
conn handle or delete it. Similarly, hci_disconnect_all_sync iterates
conn_hash without locks/RCU which is not OK.

To make it correct vs locking, use hci_conn_sync_queue to hold
hdev->lock and check hci_conn aliveness after waiting for events.

Make iteration in hci_disconnect_all_sync safe. Since we don't have a
flag for indicating whether a connection was aborted, just make a copy
of the conn_hash and iterate the copy.

Signed-off-by: Pauli Virtanen <[email protected]>
---

Notes:
The iterate-over-copy is ugly, could add another hci_conn flag. Or
change things so that hci_conn_get keeps the hci_conn in conn_hash so we
can always continue the iteration safely as long as a reference was
held.

net/bluetooth/hci_conn.c | 10 ++------
net/bluetooth/hci_sync.c | 52 ++++++++++++++++++++++++++++------------
2 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 208eb5eea451..ba339a0eb851 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2845,12 +2845,7 @@ u32 hci_conn_get_phy(struct hci_conn *conn)

static int abort_conn_sync(struct hci_dev *hdev, void *data)
{
- struct hci_conn *conn;
- u16 handle = PTR_ERR(data);
-
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
+ struct hci_conn *conn = data;

return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
}
@@ -2886,8 +2881,7 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
}
}

- return hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
- NULL);
+ return hci_conn_sync_queue(conn, abort_conn_sync, conn, NULL);
}

struct hci_conn_sync_work_entry {
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 6a295a9e1f5d..101548fe81da 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5407,6 +5407,8 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
{
int err;

+ lockdep_assert_held(&hdev->lock);
+
switch (conn->state) {
case BT_CONNECTED:
case BT_CONFIG:
@@ -5418,21 +5420,15 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
* or in case of LE it was still scanning so it can be cleanup
* safely.
*/
- if (err) {
- hci_dev_lock(hdev);
+ if (err && hci_conn_is_alive(hdev, conn))
hci_conn_failed(conn, err);
- hci_dev_unlock(hdev);
- }
return err;
case BT_CONNECT2:
return hci_reject_conn_sync(hdev, conn, reason);
case BT_OPEN:
/* Cleanup bises that failed to be established */
- if (test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags)) {
- hci_dev_lock(hdev);
+ if (test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))
hci_conn_failed(conn, reason);
- hci_dev_unlock(hdev);
- }
break;
default:
conn->state = BT_CLOSED;
@@ -5444,16 +5440,42 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)

static int hci_disconnect_all_sync(struct hci_dev *hdev, u8 reason)
{
- struct hci_conn *conn, *tmp;
- int err;
+ struct hci_conn *conn;
+ struct hci_conn **conns;
+ int err = 0;
+ size_t i, n;

- list_for_each_entry_safe(conn, tmp, &hdev->conn_hash.list, list) {
- err = hci_abort_conn_sync(hdev, conn, reason);
- if (err)
- return err;
+ hci_cmd_sync_dev_lock(hdev);
+
+ /* Make a copy of conn_hash, because hci_abort_conn_sync may release the
+ * lock and wait for events, during which the list may be mutated
+ * arbitrarily.
+ */
+ n = 0;
+ list_for_each_entry(conn, &hdev->conn_hash.list, list)
+ ++n;
+
+ conns = kvcalloc(n, sizeof(*conns), GFP_KERNEL);
+ if (!conns) {
+ err = -ENOMEM;
+ goto unlock;
}

- return 0;
+ i = 0;
+ list_for_each_entry(conn, &hdev->conn_hash.list, list)
+ conns[i++] = hci_conn_get(conn);
+
+ for (i = 0; i < n; ++i) {
+ if (!err)
+ err = hci_abort_conn_sync(hdev, conns[i], reason);
+ hci_conn_put(conns[i]);
+ }
+
+ kvfree(conns);
+
+unlock:
+ hci_cmd_sync_dev_unlock(hdev);
+ return err;
}

/* This function perform power off HCI command sequence as follows:
--
2.41.0


2023-07-26 22:41:12

by bluez.test.bot

[permalink] [raw]
Subject: RE: Locking in hci_sync

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=769854

---Test result---

Test Summary:
CheckPatch PASS 5.24 seconds
GitLint FAIL 2.16 seconds
SubjectPrefix PASS 0.75 seconds
BuildKernel PASS 32.99 seconds
CheckAllWarning PASS 36.22 seconds
CheckSparse PASS 42.20 seconds
CheckSmatch PASS 112.30 seconds
BuildKernel32 PASS 31.92 seconds
TestRunnerSetup PASS 484.75 seconds
TestRunner_l2cap-tester PASS 23.23 seconds
TestRunner_iso-tester PASS 44.83 seconds
TestRunner_bnep-tester PASS 10.64 seconds
TestRunner_mgmt-tester PASS 216.30 seconds
TestRunner_rfcomm-tester PASS 16.08 seconds
TestRunner_sco-tester PASS 16.79 seconds
TestRunner_ioctl-tester PASS 17.90 seconds
TestRunner_mesh-tester PASS 13.44 seconds
TestRunner_smp-tester PASS 14.31 seconds
TestRunner_userchan-tester PASS 11.30 seconds
IncrementalBuild PASS 102.70 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[RFC,2/6] Bluetooth: hci_conn: add hci_conn_is_alive

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
22: B2 Line has trailing whitespace: " "
28: B2 Line has trailing whitespace: " "
33: B2 Line has trailing whitespace: " "
37: B2 Line has trailing whitespace: " "
[RFC,3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock

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 (84>80): "[RFC,3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock"
[RFC,4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and 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 (88>80): "[RFC,4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and hci_disconnect_all_sync"


---
Regards,
Linux Bluetooth

2023-07-27 04:36:42

by Paul Menzel

[permalink] [raw]
Subject: Re: [PATCH RFC 1/6] Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del

Dear Pauli,


Thank you for your patch.

You might want to make the commit message summary a statement about the
action. Maybe:

Combine unneeded hci_conn_cleanup() with hci_conn_del()

Am 26.07.23 um 23:25 schrieb Pauli Virtanen:
> hci_conn_cleanup is no longer needed, so move the code back to

Why is it no longer needed?

> hci_conn_del to keep the hci_conn teardown in a single place.
>
> This undoes commit b958f9a3e877 ("Bluetooth: Fix reference counting for
> LE-scan based connections"), but keeps the current order of cleanup
> operations.
>
> Signed-off-by: Pauli Virtanen <[email protected]>
> ---
> net/bluetooth/hci_conn.c | 78 +++++++++++++++++-----------------------
> 1 file changed, 33 insertions(+), 45 deletions(-)

[…]


Kind regards,

Paul

2023-08-04 23:34:35

by patchwork-bot+bluetooth

[permalink] [raw]
Subject: Re: [PATCH RFC 0/6] Locking in hci_sync

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[email protected]>:

On Thu, 27 Jul 2023 00:25:20 +0300 you wrote:
> The current guarantees that a given hci_conn is not freed concurrently
> appear to be:
>
> - hci_dev_lock is held, or,
> - rcu_read_lock is held (doesn't guarantee valid conn state), or,
> - hci_conn_get refcount held (doesn't guarantee valid conn state),
> - current task is running from hdev->workqueue (which is ordered)
>
> [...]

Here is the summary with links:
- [RFC,1/6] Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del
(no matching commit)
- [RFC,2/6] Bluetooth: hci_conn: add hci_conn_is_alive
(no matching commit)
- [RFC,3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock
(no matching commit)
- [RFC,4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and hci_disconnect_all_sync
(no matching commit)
- [RFC,5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting
(no matching commit)
- [RFC,6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn
https://git.kernel.org/bluetooth/bluetooth-next/c/2dfe76d58d3a

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html