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