* [PATCH AUTOSEL 6.9 07/22] Bluetooth: hci_core: cancel all works upon hci_unregister_dev() [not found] <20240716142519.2712487-1-sashal@kernel.org> @ 2024-07-16 14:24 ` Sasha Levin 2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 08/22] Bluetooth: btnxpuart: Enable Power Save feature on startup Sasha Levin 2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release Sasha Levin 2 siblings, 0 replies; 6+ messages in thread From: Sasha Levin @ 2024-07-16 14:24 UTC (permalink / raw) To: linux-kernel, stable Cc: Tetsuo Handa, syzbot, Luiz Augusto von Dentz, Sasha Levin, marcel, johan.hedberg, luiz.dentz, linux-bluetooth From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> [ Upstream commit 0d151a103775dd9645c78c97f77d6e2a5298d913 ] syzbot is reporting that calling hci_release_dev() from hci_error_reset() due to hci_dev_put() from hci_error_reset() can cause deadlock at destroy_workqueue(), for hci_error_reset() is called from hdev->req_workqueue which destroy_workqueue() needs to flush. We need to make sure that hdev->{rx_work,cmd_work,tx_work} which are queued into hdev->workqueue and hdev->{power_on,error_reset} which are queued into hdev->req_workqueue are no longer running by the moment destroy_workqueue(hdev->workqueue); destroy_workqueue(hdev->req_workqueue); are called from hci_release_dev(). Call cancel_work_sync() on these work items from hci_unregister_dev() as soon as hdev->list is removed from hci_dev_list. Reported-by: syzbot <syzbot+da0a9c9721e36db712e8@syzkaller.appspotmail.com> Closes: https://syzkaller.appspot.com/bug?extid=da0a9c9721e36db712e8 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- net/bluetooth/hci_core.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 24f6b6a5c7721..131bb8b5777b4 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2744,7 +2744,11 @@ void hci_unregister_dev(struct hci_dev *hdev) list_del(&hdev->list); write_unlock(&hci_dev_list_lock); + cancel_work_sync(&hdev->rx_work); + cancel_work_sync(&hdev->cmd_work); + cancel_work_sync(&hdev->tx_work); cancel_work_sync(&hdev->power_on); + cancel_work_sync(&hdev->error_reset); hci_cmd_sync_clear(hdev); -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 6.9 08/22] Bluetooth: btnxpuart: Enable Power Save feature on startup [not found] <20240716142519.2712487-1-sashal@kernel.org> 2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 07/22] Bluetooth: hci_core: cancel all works upon hci_unregister_dev() Sasha Levin @ 2024-07-16 14:24 ` Sasha Levin 2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release Sasha Levin 2 siblings, 0 replies; 6+ messages in thread From: Sasha Levin @ 2024-07-16 14:24 UTC (permalink / raw) To: linux-kernel, stable Cc: Neeraj Sanjay Kale, Paul Menzel, Luiz Augusto von Dentz, Sasha Levin, amitkumar.karwar, marcel, luiz.dentz, linux-bluetooth From: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> [ Upstream commit 4183a7be77009fc31c5760429fe095f163bf96a9 ] This sets the default power save mode setting to enabled. The power save feature is now stable and stress test issues, such as the TX timeout error, have been resolved. commit c7ee0bc8db32 ("Bluetooth: btnxpuart: Resolve TX timeout error in power save stress test") With this setting, the driver will send the vendor command to FW at startup, to enable power save feature. User can disable this feature using the following vendor command: hcitool cmd 3f 23 03 00 00 (HCI_NXP_AUTO_SLEEP_MODE) Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- drivers/bluetooth/btnxpuart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c index 9d0c7e278114b..9bfa9a6ad56c8 100644 --- a/drivers/bluetooth/btnxpuart.c +++ b/drivers/bluetooth/btnxpuart.c @@ -281,7 +281,7 @@ static u8 crc8_table[CRC8_TABLE_SIZE]; /* Default configurations */ #define DEFAULT_H2C_WAKEUP_MODE WAKEUP_METHOD_BREAK -#define DEFAULT_PS_MODE PS_MODE_DISABLE +#define DEFAULT_PS_MODE PS_MODE_ENABLE #define FW_INIT_BAUDRATE HCI_NXP_PRI_BAUDRATE static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode, -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release [not found] <20240716142519.2712487-1-sashal@kernel.org> 2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 07/22] Bluetooth: hci_core: cancel all works upon hci_unregister_dev() Sasha Levin 2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 08/22] Bluetooth: btnxpuart: Enable Power Save feature on startup Sasha Levin @ 2024-07-16 14:24 ` Sasha Levin 2024-07-16 18:59 ` Pauli Virtanen 2 siblings, 1 reply; 6+ messages in thread From: Sasha Levin @ 2024-07-16 14:24 UTC (permalink / raw) To: linux-kernel, stable Cc: Edward Adam Davis, syzbot+b7f6f8c9303466e16c8a, Luiz Augusto von Dentz, Sasha Levin, marcel, johan.hedberg, luiz.dentz, linux-bluetooth From: Edward Adam Davis <eadavis@qq.com> [ Upstream commit 89e856e124f9ae548572c56b1b70c2255705f8fe ] The problem occurs between the system call to close the sock and hci_rx_work, where the former releases the sock and the latter accesses it without lock protection. CPU0 CPU1 ---- ---- sock_close hci_rx_work l2cap_sock_release hci_acldata_packet l2cap_sock_kill l2cap_recv_frame sk_free l2cap_conless_channel l2cap_sock_recv_cb If hci_rx_work processes the data that needs to be received before the sock is closed, then everything is normal; Otherwise, the work thread may access the released sock when receiving data. Add a chan mutex in the rx callback of the sock to achieve synchronization between the sock release and recv cb. Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer. Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index 8645461d45e81..64827e553d638 100644 --- a/net/bluetooth/l2cap_sock.c +++ b/net/bluetooth/l2cap_sock.c @@ -1239,6 +1239,10 @@ static void l2cap_sock_kill(struct sock *sk) BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state)); + /* Sock is dead, so set chan data to NULL, avoid other task use invalid + * sock pointer. + */ + l2cap_pi(sk)->chan->data = NULL; /* Kill poor orphan */ l2cap_chan_put(l2cap_pi(sk)->chan); @@ -1481,12 +1485,25 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan) static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) { - struct sock *sk = chan->data; - struct l2cap_pinfo *pi = l2cap_pi(sk); + struct sock *sk; + struct l2cap_pinfo *pi; int err; - lock_sock(sk); + /* To avoid race with sock_release, a chan lock needs to be added here + * to synchronize the sock. + */ + l2cap_chan_hold(chan); + l2cap_chan_lock(chan); + sk = chan->data; + if (!sk) { + l2cap_chan_unlock(chan); + l2cap_chan_put(chan); + return -ENXIO; + } + + pi = l2cap_pi(sk); + lock_sock(sk); if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) { err = -ENOMEM; goto done; @@ -1535,6 +1552,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) done: release_sock(sk); + l2cap_chan_unlock(chan); + l2cap_chan_put(chan); return err; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release 2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release Sasha Levin @ 2024-07-16 18:59 ` Pauli Virtanen 2024-07-16 19:06 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 6+ messages in thread From: Pauli Virtanen @ 2024-07-16 18:59 UTC (permalink / raw) To: Sasha Levin, linux-kernel, stable Cc: Edward Adam Davis, syzbot+b7f6f8c9303466e16c8a, Luiz Augusto von Dentz, marcel, johan.hedberg, luiz.dentz, linux-bluetooth Hi, ti, 2024-07-16 kello 10:24 -0400, Sasha Levin kirjoitti: > From: Edward Adam Davis <eadavis@qq.com> > > [ Upstream commit 89e856e124f9ae548572c56b1b70c2255705f8fe ] This one needed an additional fixup that I don't see AUTOSEL picked up, otherwise it results to a worse regression: https://lore.kernel.org/linux-bluetooth/20240624134637.3790278-1-luiz.dentz@gmail.com/ https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1a8f402f13f94263cf349216c257b2985100927 Looks like f1a8f402f13f94263cf349216c257b2985100927 also contains other changes not related to this patch, seems like https://lore.kernel.org/linux-bluetooth/20240624144911.3817479-1-luiz.dentz@gmail.com/ was squashed. > The problem occurs between the system call to close the sock and hci_rx_work, > where the former releases the sock and the latter accesses it without lock protection. > > CPU0 CPU1 > ---- ---- > sock_close hci_rx_work > l2cap_sock_release hci_acldata_packet > l2cap_sock_kill l2cap_recv_frame > sk_free l2cap_conless_channel > l2cap_sock_recv_cb > > If hci_rx_work processes the data that needs to be received before the sock is > closed, then everything is normal; Otherwise, the work thread may access the > released sock when receiving data. > > Add a chan mutex in the rx callback of the sock to achieve synchronization between > the sock release and recv cb. > > Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer. > > Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > Signed-off-by: Sasha Levin <sashal@kernel.org> > --- > net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > index 8645461d45e81..64827e553d638 100644 > --- a/net/bluetooth/l2cap_sock.c > +++ b/net/bluetooth/l2cap_sock.c > @@ -1239,6 +1239,10 @@ static void l2cap_sock_kill(struct sock *sk) > > BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state)); > > + /* Sock is dead, so set chan data to NULL, avoid other task use invalid > + * sock pointer. > + */ > + l2cap_pi(sk)->chan->data = NULL; > /* Kill poor orphan */ > > l2cap_chan_put(l2cap_pi(sk)->chan); > @@ -1481,12 +1485,25 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan) > > static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) > { > - struct sock *sk = chan->data; > - struct l2cap_pinfo *pi = l2cap_pi(sk); > + struct sock *sk; > + struct l2cap_pinfo *pi; > int err; > > - lock_sock(sk); > + /* To avoid race with sock_release, a chan lock needs to be added here > + * to synchronize the sock. > + */ > + l2cap_chan_hold(chan); > + l2cap_chan_lock(chan); > + sk = chan->data; > > + if (!sk) { > + l2cap_chan_unlock(chan); > + l2cap_chan_put(chan); > + return -ENXIO; > + } > + > + pi = l2cap_pi(sk); > + lock_sock(sk); > if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) { > err = -ENOMEM; > goto done; > @@ -1535,6 +1552,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) > > done: > release_sock(sk); > + l2cap_chan_unlock(chan); > + l2cap_chan_put(chan); > > return err; > } -- Pauli Virtanen ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release 2024-07-16 18:59 ` Pauli Virtanen @ 2024-07-16 19:06 ` Luiz Augusto von Dentz 2024-07-22 12:45 ` Sasha Levin 0 siblings, 1 reply; 6+ messages in thread From: Luiz Augusto von Dentz @ 2024-07-16 19:06 UTC (permalink / raw) To: Pauli Virtanen Cc: Sasha Levin, linux-kernel, stable, Edward Adam Davis, syzbot+b7f6f8c9303466e16c8a, Luiz Augusto von Dentz, marcel, johan.hedberg, linux-bluetooth Hi Pauli, On Tue, Jul 16, 2024 at 3:00 PM Pauli Virtanen <pav@iki.fi> wrote: > > Hi, > > ti, 2024-07-16 kello 10:24 -0400, Sasha Levin kirjoitti: > > From: Edward Adam Davis <eadavis@qq.com> > > > > [ Upstream commit 89e856e124f9ae548572c56b1b70c2255705f8fe ] > > This one needed an additional fixup that I don't see AUTOSEL picked up, > otherwise it results to a worse regression: > > https://lore.kernel.org/linux-bluetooth/20240624134637.3790278-1-luiz.dentz@gmail.com/ > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1a8f402f13f94263cf349216c257b2985100927 > > > Looks like f1a8f402f13f94263cf349216c257b2985100927 also contains other > changes not related to this patch, seems like > https://lore.kernel.org/linux-bluetooth/20240624144911.3817479-1-luiz.dentz@gmail.com/ > was squashed. Yep, it seems I messed them up while doing the pull-request and 2 commits were merged together, not sure if we can rebase them now that are in Linus tree, anyway for stable it would be better to unmerge them if possible. > > The problem occurs between the system call to close the sock and hci_rx_work, > > where the former releases the sock and the latter accesses it without lock protection. > > > > CPU0 CPU1 > > ---- ---- > > sock_close hci_rx_work > > l2cap_sock_release hci_acldata_packet > > l2cap_sock_kill l2cap_recv_frame > > sk_free l2cap_conless_channel > > l2cap_sock_recv_cb > > > > If hci_rx_work processes the data that needs to be received before the sock is > > closed, then everything is normal; Otherwise, the work thread may access the > > released sock when receiving data. > > > > Add a chan mutex in the rx callback of the sock to achieve synchronization between > > the sock release and recv cb. > > > > Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer. > > > > Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Signed-off-by: Sasha Levin <sashal@kernel.org> > > --- > > net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++--- > > 1 file changed, 22 insertions(+), 3 deletions(-) > > > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > > index 8645461d45e81..64827e553d638 100644 > > --- a/net/bluetooth/l2cap_sock.c > > +++ b/net/bluetooth/l2cap_sock.c > > @@ -1239,6 +1239,10 @@ static void l2cap_sock_kill(struct sock *sk) > > > > BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state)); > > > > + /* Sock is dead, so set chan data to NULL, avoid other task use invalid > > + * sock pointer. > > + */ > > + l2cap_pi(sk)->chan->data = NULL; > > /* Kill poor orphan */ > > > > l2cap_chan_put(l2cap_pi(sk)->chan); > > @@ -1481,12 +1485,25 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan) > > > > static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) > > { > > - struct sock *sk = chan->data; > > - struct l2cap_pinfo *pi = l2cap_pi(sk); > > + struct sock *sk; > > + struct l2cap_pinfo *pi; > > int err; > > > > - lock_sock(sk); > > + /* To avoid race with sock_release, a chan lock needs to be added here > > + * to synchronize the sock. > > + */ > > + l2cap_chan_hold(chan); > > + l2cap_chan_lock(chan); > > + sk = chan->data; > > > > + if (!sk) { > > + l2cap_chan_unlock(chan); > > + l2cap_chan_put(chan); > > + return -ENXIO; > > + } > > + > > + pi = l2cap_pi(sk); > > + lock_sock(sk); > > if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) { > > err = -ENOMEM; > > goto done; > > @@ -1535,6 +1552,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) > > > > done: > > release_sock(sk); > > + l2cap_chan_unlock(chan); > > + l2cap_chan_put(chan); > > > > return err; > > } > > -- > Pauli Virtanen -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release 2024-07-16 19:06 ` Luiz Augusto von Dentz @ 2024-07-22 12:45 ` Sasha Levin 0 siblings, 0 replies; 6+ messages in thread From: Sasha Levin @ 2024-07-22 12:45 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Pauli Virtanen, linux-kernel, stable, Edward Adam Davis, syzbot+b7f6f8c9303466e16c8a, Luiz Augusto von Dentz, marcel, johan.hedberg, linux-bluetooth On Tue, Jul 16, 2024 at 03:06:01PM -0400, Luiz Augusto von Dentz wrote: >Hi Pauli, > >On Tue, Jul 16, 2024 at 3:00 PM Pauli Virtanen <pav@iki.fi> wrote: >> >> Hi, >> >> ti, 2024-07-16 kello 10:24 -0400, Sasha Levin kirjoitti: >> > From: Edward Adam Davis <eadavis@qq.com> >> > >> > [ Upstream commit 89e856e124f9ae548572c56b1b70c2255705f8fe ] >> >> This one needed an additional fixup that I don't see AUTOSEL picked up, >> otherwise it results to a worse regression: >> >> https://lore.kernel.org/linux-bluetooth/20240624134637.3790278-1-luiz.dentz@gmail.com/ >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1a8f402f13f94263cf349216c257b2985100927 >> >> >> Looks like f1a8f402f13f94263cf349216c257b2985100927 also contains other >> changes not related to this patch, seems like >> https://lore.kernel.org/linux-bluetooth/20240624144911.3817479-1-luiz.dentz@gmail.com/ >> was squashed. > >Yep, it seems I messed them up while doing the pull-request and 2 >commits were merged together, not sure if we can rebase them now that >are in Linus tree, anyway for stable it would be better to unmerge >them if possible. We really want to keep patches the same as they are in Linus's tree - it doesn't make sense to edit them just for stable. -- Thanks, Sasha ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-22 12:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240716142519.2712487-1-sashal@kernel.org>
2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 07/22] Bluetooth: hci_core: cancel all works upon hci_unregister_dev() Sasha Levin
2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 08/22] Bluetooth: btnxpuart: Enable Power Save feature on startup Sasha Levin
2024-07-16 14:24 ` [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release Sasha Levin
2024-07-16 18:59 ` Pauli Virtanen
2024-07-16 19:06 ` Luiz Augusto von Dentz
2024-07-22 12:45 ` Sasha Levin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox