Possible sleep-in-atomic in BT SCO code

* Possible sleep-in-atomic in BT SCO code
@ 2021-09-10 11:00 Takashi Iwai
  2021-09-10 11:56 ` bluez.test.bot
  0 siblings, 1 reply; 2+ messages in thread
From: Takashi Iwai @ 2021-09-10 11:00 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: linux-kernel, Nicolai Stange

Hi,

while investigation of the recent BT fixes, Nicolai found out that the
change in the commit 27c24fda62b6 ("Bluetooth: switch to lock_sock in
SCO") may cause a sleep-in-atomic.

The commit replaced bh_lock_sock() with lock_sock(), which can sleep.
Meanwhile, in sco_conn_ready(), this is called after sco_conn_lock(),
and sco_conn_lock() is a simple spinlock.  So this may lead to a
sleep-in-atomic.

I can imagine a fix like the below, but this also made us wonder
whether the sco_conn_lock() would be needed at all.  In the code path,
conn->hcon won't be changed, right?


thanks,

Takashi

--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -1118,18 +1118,22 @@ static void sco_conn_ready(struct sco_conn *conn)
 			return;
 		}
 
+		sock_hold(parent);
+		sco_conn_unlock(conn);
+
 		lock_sock(parent);
 
 		sk = sco_sock_alloc(sock_net(parent), NULL,
 				    BTPROTO_SCO, GFP_ATOMIC, 0);
 		if (!sk) {
 			release_sock(parent);
-			sco_conn_unlock(conn);
+			sock_put(parent);
 			return;
 		}
 
 		sco_sock_init(sk, parent);
 
+		sco_conn_lock(conn);
 		bacpy(&sco_pi(sk)->src, &conn->hcon->src);
 		bacpy(&sco_pi(sk)->dst, &conn->hcon->dst);
 
@@ -1143,10 +1147,10 @@ static void sco_conn_ready(struct sco_conn *conn)
 
 		/* Wake up parent */
 		parent->sk_data_ready(parent);
+		sco_conn_unlock(conn);
 
 		release_sock(parent);
-
-		sco_conn_unlock(conn);
+		sock_put(parent);
 	}
 }
 


^ permalink raw reply	[flat|nested] 2+ messages in thread