Hi Pauli, On Tue, Jul 16, 2024 at 3:00 PM Pauli Virtanen <pav@xxxxxx> wrote: > > Hi, > > ti, 2024-07-16 kello 10:24 -0400, Sasha Levin kirjoitti: > > From: Edward Adam Davis <eadavis@xxxxxx> > > > > [ 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@xxxxxxxxx/ > > 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@xxxxxxxxx/ > 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@xxxxxxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> > > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx> > > --- > > 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
