Possible sleep-in-atomic in BT SCO code

Commit Message

Takashi Iwai Sept. 10, 2021, 11 a.m. UTC

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

Comments

[e-mail] Sept. 10, 2021, 11:56 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=544853

---Test result---

Test Summary:
CheckPatch                    FAIL      0.46 seconds
GitLint                       PASS      0.10 seconds
BuildKernel                   PASS      512.47 seconds
TestRunner: Setup             PASS      338.44 seconds
TestRunner: l2cap-tester      PASS      2.68 seconds
TestRunner: bnep-tester       PASS      1.86 seconds
TestRunner: mgmt-tester       FAIL      31.33 seconds
TestRunner: rfcomm-tester     PASS      2.06 seconds
TestRunner: sco-tester        PASS      2.08 seconds
TestRunner: smp-tester        PASS      2.13 seconds
TestRunner: userchan-tester   PASS      1.98 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.46 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Possible sleep-in-atomic in BT SCO code
WARNING: Unknown commit id '27c24fda62b6', maybe rebased or not pulled?
#9: 
change in the commit 27c24fda62b6 ("Bluetooth: switch to lock_sock in

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 0 checks, 35 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Possible sleep-in-atomic in BT SCO code" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS - 0.10 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 512.47 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 338.44 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.68 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.86 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - FAIL - 31.33 seconds
Run test-runner with mgmt-tester
Total: 452, Passed: 451 (99.8%), Failed: 1, Not Run: 0

Failed Test Cases
Read Exp Feature - Success                           Failed       0.020 seconds

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.06 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.08 seconds
Run test-runner with sco-tester
Total: 11, Passed: 11 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - PASS - 2.13 seconds
Run test-runner with smp-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: userchan-tester - PASS - 1.98 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth

Patch

--- 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);
 	}
 }