[PATCH] Bluetooth: SCO: fix sco_conn refcounting on sco_conn_ready

[PATCH] Bluetooth: SCO: fix sco_conn refcounting on sco_conn_ready

[Pauli Virtanen]

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

RE: Bluetooth: SCO: fix sco_conn refcounting on sco_conn_ready

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

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

Re: [PATCH] Bluetooth: SCO: fix sco_conn refcounting on sco_conn_ready

[Luiz Augusto von Dentz]

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
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: SCO: fix sco_conn refcounting on sco_conn_ready

[Pauli Virtanen]

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
> > 
> > 
> 
> 

-- 
Pauli Virtanen