Re: [PATCH] Bluetooth: RFCOMM: defer security confirmation to krfcommd

[Mikhail Gavrilov]

Hi Pauli,
thanks for the review.

> Sashiko review comment that security_cfm() cleanup should be run in
> rfcomm_init() after hci_unregister_cb(), appears correct.

Correct, and rfcomm_init() is exactly where it matters: the error path
runs kthread_stop() at the stop: label and hci_unregister_cb() only at
unregister:, so a confirmation queued in between would have outlived the
thread that frees it.  v2 has rfcomm_flush_security_cfm(), which just
drops the queued entries, called after hci_unregister_cb() in
rfcomm_init() and after kthread_stop() in rfcomm_exit().  The drain at
the end of rfcomm_run() is gone.

> However, rfcomm_session_get() could return a different session if
> processing is delayed. Is this a concern? ABA issue?

It is.  Nothing keeps a session alive while the confirmation waits:
__rfcomm_dlc_close() can drop the last DLC and rfcomm_dlc_open() can
create a new session to the same peer, both from a syscall, without
krfcommd running in between.  The lookup is by (src, dst), so the new
session matches and a stale RFCOMM_AUTH_ACCEPT can be set on a DLC that
asked for BT_SECURITY_HIGH while the current link is weaker.

v2 compares the session's hci_conn with the one the confirmation was
reported for and skips the session if they differ.  Comparing pointers
is safe because the entry holds a reference on the connection, so the
object cannot be freed and reused while it is queued.

> This patch introduces data race in read of conn->cfm->sec_level,
> probably benign, but these are not marked with READ_ONCE/WRITE_ONCE.
>
> I'd maybe take hdev_lock in rfcomm_process_security_cfm instead.

Done that way in v2: the entry pins the controller with hci_dev_hold()
and krfcommd takes hdev->lock around applying the confirmation, so
conn->sec_level is read in the same context as before the change and
nothing needs new READ_ONCE()/WRITE_ONCE() annotations.  rfcomm_mutex ->
hdev->lock is the order rfcomm_dlc_open() already uses, so this does not
bring the cycle back.

> Context analysis annotations would be useful here:

Added __guarded_by(&security_cfm_lock) on security_cfm_list and
__must_hold(&rfcomm_mutex) on __rfcomm_security_cfm().

I left the list_head member of struct rfcomm_sec_cfm unannotated:
rfcomm_process_security_cfm() moves the whole queue to a local list with
list_splice_init() and from then on the entries are private to that
thread, so the list_del() and the free happen without the spinlock and
__guarded_by(&security_cfm_lock) on the member would flag correct code.

I could not build with LLVM here - no clang 23 - so the annotations are
by inspection only, gcc W=1 and checkpatch --strict are clean.  If you
have the checker at hand a run on net/bluetooth/rfcomm/ would be
welcome.

-- 
Thanks,
Mikhail