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