| Message ID | dd9eaa0aee1e2fbeeb2015b807a3a2701af3a1bf.1740686998.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: SCO: fix sco_conn refcounting on sco_conn_ready | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | warning | CheckSparse WARNING net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | fail | TestRunner_mgmt-tester: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | fail | TestRunner_mesh-tester: Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0 |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
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=938683 ---Test result--- Test Summary: CheckPatch PENDING 0.39 seconds GitLint PENDING 0.23 seconds SubjectPrefix PASS 0.07 seconds BuildKernel PASS 24.55 seconds CheckAllWarning PASS 26.91 seconds CheckSparse WARNING 30.18 seconds BuildKernel32 PASS 24.02 seconds TestRunnerSetup PASS 431.58 seconds TestRunner_l2cap-tester PASS 25.21 seconds TestRunner_iso-tester PASS 32.05 seconds TestRunner_bnep-tester PASS 4.78 seconds TestRunner_mgmt-tester FAIL 122.21 seconds TestRunner_rfcomm-tester PASS 7.90 seconds TestRunner_sco-tester PASS 11.65 seconds TestRunner_ioctl-tester PASS 8.46 seconds TestRunner_mesh-tester FAIL 6.27 seconds TestRunner_smp-tester PASS 7.25 seconds TestRunner_userchan-tester PASS 4.99 seconds IncrementalBuild PENDING 0.65 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 Failed Test Cases LL Privacy - Add Device 2 (2 Devices to AL) Failed 0.170 seconds LL Privacy - Set Flags 2 (Enable RL) Failed 0.146 seconds LL Privacy - Set Flags 3 (2 Devices to RL) Failed 0.198 seconds ############################## Test: TestRunner_mesh-tester - FAIL Desc: Run mesh-tester with test-runner Output: Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0 Failed Test Cases Mesh - Send cancel - 2 Failed 0.112 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hi Pauli, On Thu, Feb 27, 2025 at 3:12 PM Pauli Virtanen <pav@iki.fi> wrote: > > sco_conn refcount shall not be incremented a second time if the sk > already owns the refcount. > > Fixes SCO socket shutdown not actually closing the SCO socket. > > Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn") > --- > > Notes: > Making the sco_conn_add refcounts consistent in ed9588554943 exposed the > issue here. > > I think this should fix the situation, but didn't yet test this in real > use, only the sco-tester test case. > > net/bluetooth/sco.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > index aa7bfe26cb40..ad09b1b8e89a 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -1353,6 +1353,7 @@ static void sco_conn_ready(struct sco_conn *conn) > bacpy(&sco_pi(sk)->src, &conn->hcon->src); > bacpy(&sco_pi(sk)->dst, &conn->hcon->dst); > > + sco_conn_hold_unless_zero(conn); Shouldn't it check if the result is NULL otherwise perhaps we should add sco_conn_hold which doesn't use kref_get_unless_zero. > hci_conn_hold(conn->hcon); > __sco_chan_add(conn, sk, parent); > > @@ -1411,8 +1412,10 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > struct sco_conn *conn; > > conn = sco_conn_add(hcon); > - if (conn) > + if (conn) { > sco_conn_ready(conn); > + sco_conn_put(conn); Well we did this a little bit differently in iso: conn = iso_conn_hold_unless_zero(conn); if (conn) { if (!conn->hcon) { iso_conn_lock(conn); conn->hcon = hcon; iso_conn_unlock(conn); } iso_conn_put(conn); return conn; But it looks like we changed that with ed9588554943 ("Bluetooth: SCO: remove the redundant sco_conn_put"), I wonder if we have something similar in ISO as well. > + } > } else > sco_conn_del(hcon, bt_to_errno(status)); > } > -- > 2.48.1 > >
Hi, pe, 2025-02-28 kello 01:38 +0500, Luiz Augusto von Dentz kirjoitti: > Hi Pauli, > > On Thu, Feb 27, 2025 at 3:12 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > sco_conn refcount shall not be incremented a second time if the sk > > already owns the refcount. > > > > Fixes SCO socket shutdown not actually closing the SCO socket. > > > > Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn") > > --- > > > > Notes: > > Making the sco_conn_add refcounts consistent in ed9588554943 exposed the > > issue here. > > > > I think this should fix the situation, but didn't yet test this in real > > use, only the sco-tester test case. I now tested this, it seems OK. > > > > net/bluetooth/sco.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > index aa7bfe26cb40..ad09b1b8e89a 100644 > > --- a/net/bluetooth/sco.c > > +++ b/net/bluetooth/sco.c > > @@ -1353,6 +1353,7 @@ static void sco_conn_ready(struct sco_conn *conn) > > bacpy(&sco_pi(sk)->src, &conn->hcon->src); > > bacpy(&sco_pi(sk)->dst, &conn->hcon->dst); > > > > + sco_conn_hold_unless_zero(conn); > > Shouldn't it check if the result is NULL otherwise perhaps we should > add sco_conn_hold which doesn't use kref_get_unless_zero. It's guaranteed to have positive refcount here, as the sco_conn_add() from connect_cfm keeps it alive. I'll add sco_conn_hold() to indicate that. Also need Signed-off-by... > > > hci_conn_hold(conn->hcon); > > __sco_chan_add(conn, sk, parent); > > > > @@ -1411,8 +1412,10 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) > > struct sco_conn *conn; > > > > conn = sco_conn_add(hcon); > > - if (conn) > > + if (conn) { > > sco_conn_ready(conn); > > + sco_conn_put(conn); > > Well we did this a little bit differently in iso: > > conn = iso_conn_hold_unless_zero(conn); > if (conn) { > if (!conn->hcon) { > iso_conn_lock(conn); > conn->hcon = hcon; > iso_conn_unlock(conn); > } > iso_conn_put(conn); > return conn; > > But it looks like we changed that with ed9588554943 ("Bluetooth: SCO: > remove the redundant sco_conn_put"), I wonder if we have something > similar in ISO as well. Probably making sco_conn_add() always return the extra refcount as in ed9588554943 makes sense. AFAICS the current ISO code is OK vs. this issue, although it would be best to keep them in sync here. > > > + } > > } else > > sco_conn_del(hcon, bt_to_errno(status)); > > } > > -- > > 2.48.1 > > > > > >
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index aa7bfe26cb40..ad09b1b8e89a 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -1353,6 +1353,7 @@ static void sco_conn_ready(struct sco_conn *conn) bacpy(&sco_pi(sk)->src, &conn->hcon->src); bacpy(&sco_pi(sk)->dst, &conn->hcon->dst); + sco_conn_hold_unless_zero(conn); hci_conn_hold(conn->hcon); __sco_chan_add(conn, sk, parent); @@ -1411,8 +1412,10 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status) struct sco_conn *conn; conn = sco_conn_add(hcon); - if (conn) + if (conn) { sco_conn_ready(conn); + sco_conn_put(conn); + } } else sco_conn_del(hcon, bt_to_errno(status)); }
sco_conn refcount shall not be incremented a second time if the sk already owns the refcount. Fixes SCO socket shutdown not actually closing the SCO socket. Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn") --- Notes: Making the sco_conn_add refcounts consistent in ed9588554943 exposed the issue here. I think this should fix the situation, but didn't yet test this in real use, only the sco-tester test case. net/bluetooth/sco.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)