Re: [PATCH] Bluetooth: RFCOMM: serialize session teardown

Hi,

la, 2026-08-22 kello 01:45 +0800, Chengfeng Ye kirjoitti:
> rfcomm_kill_listener() walks session_list and deletes every session
> without holding rfcomm_mutex.  A connect task holds that mutex while
> rfcomm_session_create() adds a session and while its error path deletes
> the session, but the unlocked teardown can still observe the object
> between those operations.

Reviewed-by: Pauli Virtanen <[email protected]>

While looking at these locking bugs, please also consider using LLVM
context analysis to globally audit the locking of the structure, to
catch the other related data races:

https://docs.kernel.org/dev-tools/context-analysis.html
https://mirrors.edge.kernel.org/pub/tools/llvm/

I think adding the annotations probably should usually go in separate
patch to make backporting of the fix easier.

For example, adding here:

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 0e496b85e6ce..6b5c44f0d89a 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -47,7 +47,7 @@ static DEFINE_MUTEX(rfcomm_mutex);
 #define rfcomm_unlock()        mutex_unlock(&rfcomm_mutex)
 
 
-static LIST_HEAD(session_list);
+static __guarded_by(&rfcomm_mutex) LIST_HEAD(session_list);
 
 static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
 static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
@@ -676,6 +676,7 @@ int rfcomm_dlc_get_modem_status(struct rfcomm_dlc *d, u8 *v24_sig)
 
 /* ---- RFCOMM sessions ---- */
 static struct rfcomm_session *rfcomm_session_add(struct socket *sock, int state)
+       __must_hold(&rfcomm_mutex)
 {
        struct rfcomm_session *s = kzalloc_obj(*s);
 
plus other __must_hold(&rfcomm_mutex) annotations to functions where
needed, compiler warnings show the session_list is accessed without
holding rfcomm_mutex also from

rfcomm_run() -> rfcomm_session_add()

rfcomm_security_cfm() -> rfcomm_session_get()

Are these reachable? Former is probably theoretical, the latter could
be reachable. Also if unreachable, in my view, taking the lock may be
better than relying on a possibly brittle and hard to understand
invariant to protect the access.

> 
> The race can proceed as follows:
> 
>   connect task                    krfcommd
>   ------------                    --------
>   rfcomm_lock()
>   rfcomm_session_add()
>                                   fetch session from session_list
>   kernel_connect() fails
>   rfcomm_session_del()
>     remove and free session
>                                   rfcomm_session_del(session)
> 
> The final call then reads the freed session and may corrupt the list.
> 
> KASAN reported:
> 
>   BUG: KASAN: slab-use-after-free in rfcomm_session_del+0x15f/0x170
>   Read of size 8 at addr ffff8881019e9b40 by task krfcommd/87
>   Call Trace:
>    rfcomm_session_del+0x15f/0x170
>    rfcomm_run+0x16d5/0x3de0
>    kthread+0x2c6/0x3b0
>    ret_from_fork+0x36e/0x5a0
>   Allocated by task 96:
>    rfcomm_session_add+0x9e/0x300
>    rfcomm_dlc_open+0x8b1/0xdf0
>    rfcomm_sock_connect+0x34c/0x530
>   Freed by task 96:
>    kfree+0x131/0x3c0
>    rfcomm_session_del+0x109/0x170
>    rfcomm_dlc_open+0x9eb/0xdf0
>    rfcomm_sock_connect+0x34c/0x530
> 
> Hold rfcomm_mutex across the teardown traversal, matching the locking
> used by normal session processing and connect error cleanup.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Signed-off-by: Chengfeng Ye <[email protected]>
> ---
>  net/bluetooth/rfcomm/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 9cdfea666a2c..5fe2758e8c47 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -2178,8 +2178,10 @@ static void rfcomm_kill_listener(void)
>  
>  	BT_DBG("");
>  
> +	rfcomm_lock();
>  	list_for_each_entry_safe(s, n, &session_list, list)
>  		rfcomm_session_del(s);
> +	rfcomm_unlock();
>  }
>  
>  static int rfcomm_run(void *unused)

-- 
Pauli Virtanen