Skip to content

RFC hci_conn and ISO concurrency fixes

Pauli Virtanen linux-bluetooth 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 RFC Latest5 patches 8 repliesIn review

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/5Bluetooth: hci_conn: add hci_conn_is_aliveinclude/net/bluetooth/hci_core.h net/bluetooth/hci_conn.c+19 -98 replies
2/5Bluetooth: hci_sync: iterate conn_hash safely in hci_disconnect_all_syncnet/bluetooth/hci_sync.c+127 -130 replies
3/5Bluetooth: hci_conn: hold ref while hci_connect_le_sync is pendingnet/bluetooth/hci_conn.c+14 -00 replies
4/5Bluetooth: ISO: fix use-after-free in __iso_sock_closenet/bluetooth/iso.c+42 -80 replies
5/5Bluetooth: ISO: fix locking in iso_conn_readynet/bluetooth/iso.c+12 -70 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.

Read the v1 thread

Cover letter [PATCH RFC 0/5] hci_conn and ISO concurrency fixes

This series addresses additional KASAN errors seen in the same test case as in the previous one. These are mostly due to hci_conn being deleted at a bad time, or manipulated without necessary locks.

With this series, the test setup doesn't seem to be producing KASAN crashes any more.

The test setup is still

while true; do bluetoothctl power on; sleep 12; bluetoothctl power off; sleep 1.5; bluetoothctl power off; sleep 2.5; done; while true; do sudo systemctl restart bluetooth; sleep 110; done
while true; do systemctl --user restart pipewire wireplumber pipewire-pulse; sleep 91; done
while true; do paplay sample.flac & sleep 2; kill %1; sleep 0.7; done

and equivalent operations manually, on VM + connect to TWS earbuds, and let it run until it hits a crash.

There's an RFC question here: it would seem useful to be able to keep references to hci_conn around without RCU or other locks, and be able to safely continue later if the conn is still around. I.e.

    hci_conn_get(conn);
    hci_dev_unlock(hdev);
    ...
    hci_dev_lock(hdev);
    if (!hci_conn_is_alive(hdev, conn)) {
	    hci_conn_put(conn);
	    goto bail_out;
    }
    hci_conn_put(conn);
    ...

The first commit here adds this function. It should also be RCU-correct too, but I'll need to think that through a second time.

It seems in several parts in hci_sync.c it is assumed the conn is not deleted while the code is blocking waiting for controller events. At first sight it's not so clear that it's really guaranteed there can't be UAF here, so I'm wondering if there would be a need to start polluting hci_sync.c with locks and aliveness checks after waits.

Or if it's guaranteed by something not apparent and nothing needs to be done, or if some other thing should be better (such as serializing operations that delete hci_conn through hci_sync).

Pauli Virtanen (5):
Bluetooth: hci_conn: add hci_conn_is_alive
Bluetooth: hci_sync: iterate conn_hash safely in
hci_disconnect_all_sync
Bluetooth: hci_conn: hold ref while hci_connect_le_sync is pending
Bluetooth: ISO: fix use-after-free in __iso_sock_close
Bluetooth: ISO: fix locking in iso_conn_ready

 include/net/bluetooth/hci_core.h |  18 ++++
 net/bluetooth/hci_conn.c         |  24 ++++--
 net/bluetooth/hci_sync.c         | 140 ++++++++++++++++++++++++++++---
 net/bluetooth/iso.c              |  69 +++++++++++----
 4 files changed, 214 insertions(+), 37 deletions(-)
Signature · 1 line
2.41.0