From: Eric Dumazet <eric.dumazet@gmail.com> To: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>, Eric Dumazet <eric.dumazet@gmail.com>, marcel@holtmann.org, johan.hedberg@gmail.com, luiz.dentz@gmail.com, davem@davemloft.net, kuba@kernel.org, sudipm.mukherjee@gmail.com Cc: linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, skhan@linuxfoundation.org, gregkh@linuxfoundation.org, linux-kernel-mentees@lists.linuxfoundation.org, syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com Subject: Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work Date: Thu, 2 Sep 2021 14:41:21 -0700 [thread overview] Message-ID: <94942257-927c-efbc-b3fd-44cc097ad71f@gmail.com> (raw) In-Reply-To: <bad67d05-366b-bebe-cbdb-6555386497de@gmail.com> On 9/2/21 12:32 PM, Desmond Cheong Zhi Xi wrote: > > Hi Eric, > > This actually seems to be a pre-existing error in sco_sock_connect that we now hit in sco_sock_timeout. > > Any thoughts on the following patch to address the problem? > > Link: https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/ syzbot is still working on finding a repro, this is obviously not trivial, because this is a race window. I think this can happen even with a single SCO connection. This might be triggered more easily forcing a delay in sco_sock_timeout() diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 98a88158651281c9f75c4e0371044251e976e7ef..71ebe0243fab106c676c308724fe3a3f92a62cbd 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -84,8 +84,14 @@ static void sco_sock_timeout(struct work_struct *work) sco_conn_lock(conn); sk = conn->sk; - if (sk) + if (sk) { + // lets pretend cpu has been busy (in interrupts) for 100ms + int i; + for (i=0;i<100000;i++) + udelay(1); + sock_hold(sk); + } sco_conn_unlock(conn); if (!sk) Stack trace tells us that sco_sock_timeout() is running after last reference on socket has been released. __refcount_add include/linux/refcount.h:199 [inline] __refcount_inc include/linux/refcount.h:250 [inline] refcount_inc include/linux/refcount.h:267 [inline] sock_hold include/net/sock.h:702 [inline] sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88 process_one_work+0x98d/0x1630 kernel/workqueue.c:2276 worker_thread+0x658/0x11f0 kernel/workqueue.c:2422 kthread+0x3e5/0x4d0 kernel/kthread.c:319 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 This is why I suggested to delay sock_put() to make sure this can not happen. diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 98a88158651281c9f75c4e0371044251e976e7ef..bd0222e3f05a6bcb40cffe8405c9dfff98d7afde 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -195,10 +195,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err) sco_sock_clear_timer(sk); sco_chan_del(sk, err); release_sock(sk); - sock_put(sk); /* Ensure no more work items will run before freeing conn. */ cancel_delayed_work_sync(&conn->timeout_work); + + sock_put(sk); } hcon->sco_data = NULL;
WARNING: multiple messages have this Message-ID (diff)
From: Eric Dumazet <eric.dumazet@gmail.com> To: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>, Eric Dumazet <eric.dumazet@gmail.com>, marcel@holtmann.org, johan.hedberg@gmail.com, luiz.dentz@gmail.com, davem@davemloft.net, kuba@kernel.org, sudipm.mukherjee@gmail.com Cc: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com, linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org, linux-kernel-mentees@lists.linuxfoundation.org Subject: Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work Date: Thu, 2 Sep 2021 14:41:21 -0700 [thread overview] Message-ID: <94942257-927c-efbc-b3fd-44cc097ad71f@gmail.com> (raw) In-Reply-To: <bad67d05-366b-bebe-cbdb-6555386497de@gmail.com> On 9/2/21 12:32 PM, Desmond Cheong Zhi Xi wrote: > > Hi Eric, > > This actually seems to be a pre-existing error in sco_sock_connect that we now hit in sco_sock_timeout. > > Any thoughts on the following patch to address the problem? > > Link: https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/ syzbot is still working on finding a repro, this is obviously not trivial, because this is a race window. I think this can happen even with a single SCO connection. This might be triggered more easily forcing a delay in sco_sock_timeout() diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 98a88158651281c9f75c4e0371044251e976e7ef..71ebe0243fab106c676c308724fe3a3f92a62cbd 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -84,8 +84,14 @@ static void sco_sock_timeout(struct work_struct *work) sco_conn_lock(conn); sk = conn->sk; - if (sk) + if (sk) { + // lets pretend cpu has been busy (in interrupts) for 100ms + int i; + for (i=0;i<100000;i++) + udelay(1); + sock_hold(sk); + } sco_conn_unlock(conn); if (!sk) Stack trace tells us that sco_sock_timeout() is running after last reference on socket has been released. __refcount_add include/linux/refcount.h:199 [inline] __refcount_inc include/linux/refcount.h:250 [inline] refcount_inc include/linux/refcount.h:267 [inline] sock_hold include/net/sock.h:702 [inline] sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88 process_one_work+0x98d/0x1630 kernel/workqueue.c:2276 worker_thread+0x658/0x11f0 kernel/workqueue.c:2422 kthread+0x3e5/0x4d0 kernel/kthread.c:319 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 This is why I suggested to delay sock_put() to make sure this can not happen. diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 98a88158651281c9f75c4e0371044251e976e7ef..bd0222e3f05a6bcb40cffe8405c9dfff98d7afde 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -195,10 +195,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err) sco_sock_clear_timer(sk); sco_chan_del(sk, err); release_sock(sk); - sock_put(sk); /* Ensure no more work items will run before freeing conn. */ cancel_delayed_work_sync(&conn->timeout_work); + + sock_put(sk); } hcon->sco_data = NULL; _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2021-09-02 21:41 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-10 4:14 [PATCH v6 0/6] Bluetooth: fix locking and socket killing in SCO and RFCOMM Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 5:14 ` Bluetooth: fix locking and socket killing in SCO and RFCOMM bluez.test.bot 2021-08-10 17:51 ` Luiz Augusto von Dentz 2021-09-02 19:17 ` [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work Eric Dumazet 2021-09-02 19:17 ` Eric Dumazet 2021-09-02 19:32 ` Desmond Cheong Zhi Xi 2021-09-02 19:32 ` Desmond Cheong Zhi Xi 2021-09-02 21:41 ` Eric Dumazet [this message] 2021-09-02 21:41 ` Eric Dumazet 2021-09-02 22:53 ` Desmond Cheong Zhi Xi 2021-09-02 22:53 ` Desmond Cheong Zhi Xi 2021-09-02 23:05 ` Desmond Cheong Zhi Xi 2021-09-02 23:05 ` Desmond Cheong Zhi Xi 2021-09-02 23:42 ` Luiz Augusto von Dentz 2021-09-02 23:42 ` Luiz Augusto von Dentz 2021-09-03 3:17 ` Desmond Cheong Zhi Xi 2021-09-03 3:17 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 2/6] Bluetooth: avoid circular locks in sco_sock_connect Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 3/6] Bluetooth: switch to lock_sock in SCO Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 4/6] Bluetooth: serialize calls to sco_sock_{set,clear}_timer Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 4/6] Bluetooth: serialize calls to sco_sock_{set, clear}_timer Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 5/6] Bluetooth: switch to lock_sock in RFCOMM Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 6/6] Bluetooth: fix repeated calls to sco_sock_kill Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=94942257-927c-efbc-b3fd-44cc097ad71f@gmail.com \ --to=eric.dumazet@gmail.com \ --cc=davem@davemloft.net \ --cc=desmondcheongzx@gmail.com \ --cc=gregkh@linuxfoundation.org \ --cc=johan.hedberg@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-bluetooth@vger.kernel.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luiz.dentz@gmail.com \ --cc=marcel@holtmann.org \ --cc=netdev@vger.kernel.org \ --cc=skhan@linuxfoundation.org \ --cc=sudipm.mukherjee@gmail.com \ --cc=syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.