Re: [PATCH v2 1/1] Bluetooth: serialize accept_q access

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Ren,


Thank you for your patch.

Am 03.04.26 um 10:05 schrieb Ren Wei:
> From: Jiexun Wang <wangjiexun2025@gmail.com>
> 
> bt_sock_poll() walks the accept queue without synchronization, while
> child teardown can unlink the same socket and drop its last reference.
> 
> Protect accept_q with a dedicated lock for queue updates and polling.
> Also rework bt_accept_dequeue() to take temporary child references under
> the queue lock before dropping it and locking the child socket.
> 
> Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Co-developed-by: Yuan Tan <yuantan098@gmail.com>
> Signed-off-by: Yuan Tan <yuantan098@gmail.com>
> Suggested-by: Xin Liu <bird@lzu.edu.cn>
> Tested-by: Ren Wei <enjou1224z@gmail.com>
> Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
> Changes in v2:
> - add Tested-by: Ren Wei <enjou1224z@gmail.com>
> - resend to the public Bluetooth/netdev lists
> 
>   include/net/bluetooth/bluetooth.h |  1 +
>   net/bluetooth/af_bluetooth.c      | 85 +++++++++++++++++++++++--------
>   2 files changed, 66 insertions(+), 20 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 69eed69f7f26..3faea66b1979 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -398,6 +398,7 @@ void baswap(bdaddr_t *dst, const bdaddr_t *src);
>   struct bt_sock {
>   	struct sock sk;
>   	struct list_head accept_q;
> +	spinlock_t accept_q_lock; /* protects accept_q */
>   	struct sock *parent;
>   	unsigned long flags;
>   	void (*skb_msg_name)(struct sk_buff *, void *, int *);
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 2b94e2077203..f44e1ecc83d8 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -154,6 +154,7 @@ struct sock *bt_sock_alloc(struct net *net, struct socket *sock,
>   
>   	sock_init_data(sock, sk);
>   	INIT_LIST_HEAD(&bt_sk(sk)->accept_q);
> +	spin_lock_init(&bt_sk(sk)->accept_q_lock);
>   
>   	sock_reset_flag(sk, SOCK_ZAPPED);
>   
> @@ -214,6 +215,7 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
>   {
>   	const struct cred *old_cred;
>   	struct pid *old_pid;
> +	struct bt_sock *par = bt_sk(parent);
>   
>   	BT_DBG("parent %p, sk %p", parent, sk);
>   
> @@ -224,9 +226,12 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
>   	else
>   		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>   
> -	list_add_tail(&bt_sk(sk)->accept_q, &bt_sk(parent)->accept_q);
>   	bt_sk(sk)->parent = parent;
>   
> +	spin_lock_bh(&par->accept_q_lock);
> +	list_add_tail(&bt_sk(sk)->accept_q, &par->accept_q);
> +	spin_unlock_bh(&par->accept_q_lock);
> +
>   	/* Copy credentials from parent since for incoming connections the
>   	 * socket is allocated by the kernel.
>   	 */
> @@ -254,45 +259,73 @@ EXPORT_SYMBOL(bt_accept_enqueue);
>    */
>   void bt_accept_unlink(struct sock *sk)
>   {
> +	struct sock *parent = bt_sk(sk)->parent;
> +
>   	BT_DBG("sk %p state %d", sk, sk->sk_state);
>   
> +	spin_lock_bh(&bt_sk(parent)->accept_q_lock);
>   	list_del_init(&bt_sk(sk)->accept_q);
> -	sk_acceptq_removed(bt_sk(sk)->parent);
> +	spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
> +
> +	sk_acceptq_removed(parent);
>   	bt_sk(sk)->parent = NULL;
>   	sock_put(sk);
>   }
>   EXPORT_SYMBOL(bt_accept_unlink);
>   
> +static struct sock *bt_accept_get(struct sock *parent, struct sock *sk)
> +{
> +	struct bt_sock *bt = bt_sk(parent);
> +	struct sock *next = NULL;
> +
> +	/* accept_q is modified from child teardown paths too, so take a
> +	 * temporary reference before dropping the queue lock.
> +	 */
> +	spin_lock_bh(&bt->accept_q_lock);
> +
> +	if (sk) {
> +		if (bt_sk(sk)->parent != parent)
> +			goto out;
> +
> +		if (!list_is_last(&bt_sk(sk)->accept_q, &bt->accept_q)) {
> +			next = &list_next_entry(bt_sk(sk), accept_q)->sk;
> +			sock_hold(next);
> +		}
> +	} else if (!list_empty(&bt->accept_q)) {
> +		next = &list_first_entry(&bt->accept_q,
> +					 struct bt_sock, accept_q)->sk;
> +		sock_hold(next);
> +	}
> +
> +out:
> +	spin_unlock_bh(&bt->accept_q_lock);
> +	return next;
> +}
> +
>   struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
>   {
> -	struct bt_sock *s, *n;
> -	struct sock *sk;
> +	struct sock *sk, *next;
>   
>   	BT_DBG("parent %p", parent);
>   
>   restart:
> -	list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
> -		sk = (struct sock *)s;
> -
> +	for (sk = bt_accept_get(parent, NULL); sk; sk = next) {
>   		/* Prevent early freeing of sk due to unlink and sock_kill */
> -		sock_hold(sk);
>   		lock_sock(sk);
>   
>   		/* Check sk has not already been unlinked via
>   		 * bt_accept_unlink() due to serialisation caused by sk locking
>   		 */
> -		if (!bt_sk(sk)->parent) {
> +		if (bt_sk(sk)->parent != parent) {
>   			BT_DBG("sk %p, already unlinked", sk);
>   			release_sock(sk);
>   			sock_put(sk);
>   
> -			/* Restart the loop as sk is no longer in the list
> -			 * and also avoid a potential infinite loop because
> -			 * list_for_each_entry_safe() is not thread safe.
> -			 */
>   			goto restart;
>   		}
>   
> +		next = bt_accept_get(parent, sk);
> +
>   		/* sk is safely in the parent list so reduce reference count */
>   		sock_put(sk);
>   
> @@ -310,6 +343,8 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
>   				sock_graft(sk, newsock);
>   
>   			release_sock(sk);
> +			if (next)
> +				sock_put(next);
>   			return sk;
>   		}
>   
> @@ -518,18 +553,28 @@ EXPORT_SYMBOL(bt_sock_stream_recvmsg);
>   
>   static inline __poll_t bt_accept_poll(struct sock *parent)
>   {
> -	struct bt_sock *s, *n;
> +	struct bt_sock *bt = bt_sk(parent);
> +	struct bt_sock *s;
>   	struct sock *sk;
> +	__poll_t mask = 0;
> +
> +	spin_lock_bh(&bt->accept_q_lock);
> +	list_for_each_entry(s, &bt->accept_q, accept_q) {
> +		int state;
>   
> -	list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
>   		sk = (struct sock *)s;
> -		if (sk->sk_state == BT_CONNECTED ||
> -		    (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags) &&
> -		     sk->sk_state == BT_CONNECT2))
> -			return EPOLLIN | EPOLLRDNORM;
> +		state = READ_ONCE(sk->sk_state);
> +
> +		if (state == BT_CONNECTED ||
> +		    (test_bit(BT_SK_DEFER_SETUP, &bt->flags) &&
> +		     state == BT_CONNECT2)) {
> +			mask = EPOLLIN | EPOLLRDNORM;
> +			break;
> +		}
>   	}
> +	spin_unlock_bh(&bt->accept_q_lock);
>   
> -	return 0;
> +	return mask;
>   }
>   
>   __poll_t bt_sock_poll(struct file *file, struct socket *sock,

Google’s Gemini review service has a comment [1]. Do you think it’s 
valid and related?


Kind regards,

Paul


[1]: 
https://sashiko.dev/#/patchset/06a6b4549acba207847ce532dedbf1c95ab22d13.1774925231.git.wangjiexun2025%40gmail.com