#syz test On Mon, Oct 7, 2024 at 1:16 PM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > > #syz test > > On Fri, Oct 4, 2024 at 1:24 PM Luiz Augusto von Dentz > <luiz.dentz@xxxxxxxxx> wrote: > > > > #syz test > > > > On Fri, Oct 4, 2024 at 12:06 PM Luiz Augusto von Dentz > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > #syz test > > > > > > On Thu, Oct 3, 2024 at 3:21 PM Luiz Augusto von Dentz > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > #syz test > > > > > > > > On Thu, Oct 3, 2024 at 12:32 PM Luiz Augusto von Dentz > > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > > > #syz test > > > > > > > > > > On Thu, Oct 3, 2024 at 11:38 AM Luiz Augusto von Dentz > > > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > > > > > #syz test > > > > > > > > > > > > On Wed, Oct 2, 2024 at 4:46 PM Luiz Augusto von Dentz > > > > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > > > > > > > #syz test > > > > > > > > > > > > > > On Wed, Oct 2, 2024 at 3:46 PM Luiz Augusto von Dentz > > > > > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > #syz test > > > > > > > > > > > > > > > > On Wed, Oct 2, 2024 at 3:19 PM Luiz Augusto von Dentz > > > > > > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > #syz test > > > > > > > > > > > > > > > > > > On Wed, Oct 2, 2024 at 3:04 PM Luiz Augusto von Dentz > > > > > > > > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> > > > > > > > > > > > > > > > > > > > > This makes use of disable_delayed_work_sync instead > > > > > > > > > > cancel_delayed_work_sync as it not only cancel the ongoing work but also > > > > > > > > > > disables new submit which is disarable since the object holding the work > > > > > > > > > > is about to be freed. > > > > > > > > > > > > > > > > > > > > In addition to it remove call to sco_sock_set_timer on __sco_sock_close > > > > > > > > > > since at that point it is useless to set a timer as the sk will be freed > > > > > > > > > > there is nothing to be done in sco_sock_timeout. > > > > > > > > > > > > > > > > > > > > Reported-by: syzbot+4c0d0c4cde787116d465@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > > > > > > > Closes: https://syzkaller.appspot.com/bug?extid=4c0d0c4cde787116d465 > > > > > > > > > > Fixes: ba316be1b6a0 ("Bluetooth: schedule SCO timeouts with delayed_work") > > > > > > > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> > > > > > > > > > > --- > > > > > > > > > > net/bluetooth/sco.c | 13 +------------ > > > > > > > > > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > > > > > > > > index a5ac160c592e..2b1e66976068 100644 > > > > > > > > > > --- a/net/bluetooth/sco.c > > > > > > > > > > +++ b/net/bluetooth/sco.c > > > > > > > > > > @@ -208,7 +208,7 @@ static void sco_conn_del(struct hci_conn *hcon, int err) > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > /* Ensure no more work items will run before freeing conn. */ > > > > > > > > > > - cancel_delayed_work_sync(&conn->timeout_work); > > > > > > > > > > + disable_delayed_work_sync(&conn->timeout_work); > > > > > > > > > > > > > > > > > > > > hcon->sco_data = NULL; > > > > > > > > > > kfree(conn); > > > > > > > > > > @@ -442,17 +442,6 @@ static void __sco_sock_close(struct sock *sk) > > > > > > > > > > > > > > > > > > > > case BT_CONNECTED: > > > > > > > > > > case BT_CONFIG: > > > > > > > > > > - if (sco_pi(sk)->conn->hcon) { > > > > > > > > > > - sk->sk_state = BT_DISCONN; > > > > > > > > > > - sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); > > > > > > > > > > - sco_conn_lock(sco_pi(sk)->conn); > > > > > > > > > > - hci_conn_drop(sco_pi(sk)->conn->hcon); > > > > > > > > > > - sco_pi(sk)->conn->hcon = NULL; > > > > > > > > > > - sco_conn_unlock(sco_pi(sk)->conn); > > > > > > > > > > - } else > > > > > > > > > > - sco_chan_del(sk, ECONNRESET); > > > > > > > > > > - break; > > > > > > > > > > - > > > > > > > > > > case BT_CONNECT2: > > > > > > > > > > case BT_CONNECT: > > > > > > > > > > case BT_DISCONN: > > > > > > > > > > -- > > > > > > > > > > 2.46.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > > > > > > > > > -- > > > > > Luiz Augusto von Dentz > > > > > > > > > > > > > > > > -- > > > > Luiz Augusto von Dentz > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > > > > > -- > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz -- Luiz Augusto von Dentz
From 1202ae4e16c042a149725e30b3e8857120f0f2a7 Mon Sep 17 00:00:00 2001 From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> Date: Tue, 1 Oct 2024 15:46:10 -0400 Subject: [PATCH v4 1/2] Bluetooth: SCO: Use disable_delayed_work_sync This makes use of disable_delayed_work_sync instead cancel_delayed_work_sync as it not only cancel the ongoing work but also disables new submit which is disarable since the object holding the work is about to be freed. In addition to it remove call to sco_sock_set_timer on __sco_sock_close since at that point it is useless to set a timer as the sk will be freed there is nothing to be done in sco_sock_timeout. Reported-by: syzbot+4c0d0c4cde787116d465@xxxxxxxxxxxxxxxxxxxxxxxxx Closes: https://syzkaller.appspot.com/bug?extid=4c0d0c4cde787116d465 Fixes: ba316be1b6a0 ("Bluetooth: schedule SCO timeouts with delayed_work") Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> --- net/bluetooth/sco.c | 99 ++++++++++++++++++++++++++++++++------------- 1 file changed, 71 insertions(+), 28 deletions(-) diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index a5ac160c592e..a937c3b9d639 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -51,6 +51,7 @@ struct sco_conn { struct delayed_work timeout_work; unsigned int mtu; + struct kref ref; }; #define sco_conn_lock(c) spin_lock(&c->lock) @@ -76,12 +77,59 @@ struct sco_pinfo { #define SCO_CONN_TIMEOUT (HZ * 40) #define SCO_DISCONN_TIMEOUT (HZ * 2) +static void sco_conn_free(struct kref *ref) +{ + struct sco_conn *conn = container_of(ref, struct sco_conn, ref); + + BT_DBG("conn %p", conn); + + if (conn->sk) + sco_pi(conn->sk)->conn = NULL; + + if (conn->hcon) { + conn->hcon->sco_data = NULL; + hci_conn_drop(conn->hcon); + } + + /* Ensure no more work items will run since hci_conn has been dropped */ + disable_delayed_work_sync(&conn->timeout_work); + + kfree(conn); +} + +static void sco_conn_put(struct sco_conn *conn) +{ + if (!conn) + return; + + BT_DBG("conn %p refcnt %d", conn, kref_read(&conn->ref)); + + kref_put(&conn->ref, sco_conn_free); +} + +static struct sco_conn *sco_conn_hold_unless_zero(struct sco_conn *conn) +{ + if (!conn) + return NULL; + + BT_DBG("conn %p refcnt %u", conn, kref_read(&conn->ref)); + + if (!kref_get_unless_zero(&conn->ref)) + return NULL; + + return conn; +} + static void sco_sock_timeout(struct work_struct *work) { struct sco_conn *conn = container_of(work, struct sco_conn, timeout_work.work); struct sock *sk; + conn = sco_conn_hold_unless_zero(conn); + if (!conn) + return; + sco_conn_lock(conn); if (!conn->hcon) { sco_conn_unlock(conn); @@ -91,6 +139,7 @@ static void sco_sock_timeout(struct work_struct *work) if (sk) sock_hold(sk); sco_conn_unlock(conn); + sco_conn_put(conn); if (!sk) return; @@ -128,9 +177,14 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon) { struct sco_conn *conn = hcon->sco_data; + conn = sco_conn_hold_unless_zero(conn); if (conn) { - if (!conn->hcon) + if (!conn->hcon) { + sco_conn_lock(conn); conn->hcon = hcon; + sco_conn_unlock(conn); + } + sco_conn_put(conn); return conn; } @@ -138,6 +192,7 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon) if (!conn) return NULL; + kref_init(&conn->ref); spin_lock_init(&conn->lock); INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout); @@ -162,17 +217,15 @@ static void sco_chan_del(struct sock *sk, int err) struct sco_conn *conn; conn = sco_pi(sk)->conn; + sco_pi(sk)->conn = NULL; BT_DBG("sk %p, conn %p, err %d", sk, conn, err); if (conn) { sco_conn_lock(conn); conn->sk = NULL; - sco_pi(sk)->conn = NULL; sco_conn_unlock(conn); - - if (conn->hcon) - hci_conn_drop(conn->hcon); + sco_conn_put(conn); } sk->sk_state = BT_CLOSED; @@ -187,31 +240,30 @@ static void sco_conn_del(struct hci_conn *hcon, int err) struct sco_conn *conn = hcon->sco_data; struct sock *sk; + conn = sco_conn_hold_unless_zero(conn); if (!conn) return; BT_DBG("hcon %p conn %p, err %d", hcon, conn, err); - /* Kill socket */ sco_conn_lock(conn); sk = conn->sk; if (sk) sock_hold(sk); sco_conn_unlock(conn); + sco_conn_put(conn); - if (sk) { - lock_sock(sk); - sco_sock_clear_timer(sk); - sco_chan_del(sk, err); - release_sock(sk); - sock_put(sk); + if (!sk) { + sco_conn_put(conn); + return; } - /* Ensure no more work items will run before freeing conn. */ - cancel_delayed_work_sync(&conn->timeout_work); - - hcon->sco_data = NULL; - kfree(conn); + /* Kill socket */ + lock_sock(sk); + sco_sock_clear_timer(sk); + sco_chan_del(sk, err); + release_sock(sk); + sock_put(sk); } static void __sco_chan_add(struct sco_conn *conn, struct sock *sk, @@ -395,6 +447,8 @@ static void sco_sock_destruct(struct sock *sk) { BT_DBG("sk %p", sk); + sco_conn_put(sco_pi(sk)->conn); + skb_queue_purge(&sk->sk_receive_queue); skb_queue_purge(&sk->sk_write_queue); } @@ -442,17 +496,6 @@ static void __sco_sock_close(struct sock *sk) case BT_CONNECTED: case BT_CONFIG: - if (sco_pi(sk)->conn->hcon) { - sk->sk_state = BT_DISCONN; - sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); - sco_conn_lock(sco_pi(sk)->conn); - hci_conn_drop(sco_pi(sk)->conn->hcon); - sco_pi(sk)->conn->hcon = NULL; - sco_conn_unlock(sco_pi(sk)->conn); - } else - sco_chan_del(sk, ECONNRESET); - break; - case BT_CONNECT2: case BT_CONNECT: case BT_DISCONN: -- 2.46.1
