| Message ID | tencent_8D9918CF808DC6A5FF8DCB12055DECDDF107@qq.com |
|---|---|
| State | Accepted |
| Commit | e3203b17771757fdcd259d6378673f1590e36694 |
| Headers | show |
| Series | [v2] bluetooth/l2cap: sync sock recv cb and release | expand |
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Sat, 15 Jun 2024 09:45:54 +0800 you wrote: > 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 > > [...] Here is the summary with links: - [v2] bluetooth/l2cap: sync sock recv cb and release https://git.kernel.org/bluetooth/bluetooth-next/c/e3203b177717 You are awesome, thank you!
Hi Edward, On Fri, Jun 14, 2024 at 9:46 PM Edward Adam Davis <eadavis@qq.com> wrote: > > 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> > --- > 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 6db60946c627..f45cdf9bc985 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 Looks like this was never really tested properly: ============================================ WARNING: possible recursive locking detected 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted -------------------------------------------- kworker/u5:0/35 is trying to acquire lock: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: l2cap_sock_recv_cb+0x44/0x1e0 but task is already holding lock: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: l2cap_get_chan_by_scid+0xaf/0xd0 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(&chan->lock#2/1); lock(&chan->lock#2/1); *** DEADLOCK *** May be due to missing lock nesting notation 3 locks held by kworker/u5:0/35: #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work+0x750/0x930 #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work+0x44e/0x930 #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: l2cap_get_chan_by_scid+0xaf/0xd0 l2cap_sock_recv_cb is assumed to be called with the chan_lock held so perhaps we can just do: sk = chan->data; if (!sk) return -ENXIO;
Hi Luiz Augusto von Dentz, On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote: > > release_sock(sk); > > + l2cap_chan_unlock(chan); > > + l2cap_chan_put(chan); > > > > return err; > > } > > -- > > 2.43.0 > > Looks like this was never really tested properly: > > ============================================ > WARNING: possible recursive locking detected > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted > -------------------------------------------- > kworker/u5:0/35 is trying to acquire lock: > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > l2cap_sock_recv_cb+0x44/0x1e0 > > but task is already holding lock: > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > l2cap_get_chan_by_scid+0xaf/0xd0 > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&chan->lock#2/1); > lock(&chan->lock#2/1); > > *** DEADLOCK *** > > May be due to missing lock nesting notation > > 3 locks held by kworker/u5:0/35: > #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at: > process_one_work+0x750/0x930 > #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, > at: process_one_work+0x44e/0x930 > #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > l2cap_get_chan_by_scid+0xaf/0xd0 > > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so > perhaps we can just do: > > sk = chan->data; > if (!sk) > return -ENXIO; If the release occurs after this judgment, the same problem will still occur. Recv and release must be synchronized using locks, which can be solved by adding new lock. Can you provide a reproduction program for the AA lock mentioned above? -- Edward
Hi, pe, 2024-06-21 kello 22:45 +0800, Edward Adam Davis kirjoitti: > Hi Luiz Augusto von Dentz, > > On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote: > > > release_sock(sk); > > > + l2cap_chan_unlock(chan); > > > + l2cap_chan_put(chan); > > > > > > return err; > > > } > > > -- > > > 2.43.0 > > > > Looks like this was never really tested properly: > > > > ============================================ > > WARNING: possible recursive locking detected > > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted > > -------------------------------------------- > > kworker/u5:0/35 is trying to acquire lock: > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > l2cap_sock_recv_cb+0x44/0x1e0 > > > > but task is already holding lock: > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > l2cap_get_chan_by_scid+0xaf/0xd0 > > > > other info that might help us debug this: > > Possible unsafe locking scenario: > > > > CPU0 > > ---- > > lock(&chan->lock#2/1); > > lock(&chan->lock#2/1); > > > > *** DEADLOCK *** > > > > May be due to missing lock nesting notation > > > > 3 locks held by kworker/u5:0/35: > > #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at: > > process_one_work+0x750/0x930 > > #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, > > at: process_one_work+0x44e/0x930 > > #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > l2cap_get_chan_by_scid+0xaf/0xd0 > > > > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so > > perhaps we can just do: > > > > sk = chan->data; > > if (!sk) > > return -ENXIO; > > If the release occurs after this judgment, the same problem will still occur. > Recv and release must be synchronized using locks, which can be solved by > adding new lock. > > Can you provide a reproduction program for the AA lock mentioned above? eg. l2cap_data_channel() calls l2cap_sock_recv_cb with chan->lock (and conn->chan_lock) held. All callsites eg. l2cap_raw_recv() don't seem to hold chan->lock, so you'd probably need to check the locking on all of them, if you want to use chan->lock for this synchronization.
Hi Luiz Augusto von Dentz, On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote: > > release_sock(sk); > > + l2cap_chan_unlock(chan); > > + l2cap_chan_put(chan); > > > > return err; > > } > > -- > > 2.43.0 > > Looks like this was never really tested properly: > > ============================================ > WARNING: possible recursive locking detected > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted > -------------------------------------------- > kworker/u5:0/35 is trying to acquire lock: > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > l2cap_sock_recv_cb+0x44/0x1e0 > > but task is already holding lock: > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > l2cap_get_chan_by_scid+0xaf/0xd0 > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&chan->lock#2/1); > lock(&chan->lock#2/1); > > *** DEADLOCK *** > > May be due to missing lock nesting notation > > 3 locks held by kworker/u5:0/35: > #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at: > process_one_work+0x750/0x930 > #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, > at: process_one_work+0x44e/0x930 > #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > l2cap_get_chan_by_scid+0xaf/0xd0 > > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so > perhaps we can just do: > > sk = chan->data; > if (!sk) > return -ENXIO; If the release occurs after this judgment, the same problem will still occur. Recv and release must be synchronized using locks, which can be solved by adding new lock. Please use the new patch https://syzkaller.appspot.com/text?tag=Patch&x=15d2c48e980000, I have tested in https://syzkaller.appspot.com/bug?extid=b7f6f8c9303466e16c8a -- Edward
Hi Edward, On Fri, Jun 21, 2024 at 11:46 PM Edward Adam Davis <eadavis@qq.com> wrote: > > Hi Luiz Augusto von Dentz, > > On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote: > > > release_sock(sk); > > > + l2cap_chan_unlock(chan); > > > + l2cap_chan_put(chan); > > > > > > return err; > > > } > > > -- > > > 2.43.0 > > > > Looks like this was never really tested properly: > > > > ============================================ > > WARNING: possible recursive locking detected > > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted > > -------------------------------------------- > > kworker/u5:0/35 is trying to acquire lock: > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > l2cap_sock_recv_cb+0x44/0x1e0 > > > > but task is already holding lock: > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > l2cap_get_chan_by_scid+0xaf/0xd0 > > > > other info that might help us debug this: > > Possible unsafe locking scenario: > > > > CPU0 > > ---- > > lock(&chan->lock#2/1); > > lock(&chan->lock#2/1); > > > > *** DEADLOCK *** > > > > May be due to missing lock nesting notation > > > > 3 locks held by kworker/u5:0/35: > > #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at: > > process_one_work+0x750/0x930 > > #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, > > at: process_one_work+0x44e/0x930 > > #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > l2cap_get_chan_by_scid+0xaf/0xd0 > > > > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so > > perhaps we can just do: > > > > sk = chan->data; > > if (!sk) > > return -ENXIO; > > If the release occurs after this judgment, the same problem will still occur. > Recv and release must be synchronized using locks, which can be solved by > adding new lock. > > Please use the new patch https://syzkaller.appspot.com/text?tag=Patch&x=15d2c48e980000, I have tested in > https://syzkaller.appspot.com/bug?extid=b7f6f8c9303466e16c8a Hmm, why don't we just fix l2cap_conless_channel? Btw, l2cap_conless_channel is normally not used by any profiles thus why there isn't any CI covering it, on the other hand l2cap_data_channel is used by 99% of the profiles. > -- > Edward >
Hi Edward, On Mon, Jun 24, 2024 at 8:53 PM Edward Adam Davis <eadavis@qq.com> wrote: > > Hi Luiz Augusto von Dentz, > > On Mon, 24 Jun 2024 09:36:14 -0400, Luiz Augusto von Dentz wrote: > > > > Looks like this was never really tested properly: > > > > > > > > ============================================ > > > > WARNING: possible recursive locking detected > > > > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted > > > > -------------------------------------------- > > > > kworker/u5:0/35 is trying to acquire lock: > > > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > > > l2cap_sock_recv_cb+0x44/0x1e0 > > > > > > > > but task is already holding lock: > > > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > > > l2cap_get_chan_by_scid+0xaf/0xd0 > > > > > > > > other info that might help us debug this: > > > > Possible unsafe locking scenario: > > > > > > > > CPU0 > > > > ---- > > > > lock(&chan->lock#2/1); > > > > lock(&chan->lock#2/1); > > > > > > > > *** DEADLOCK *** > > > > > > > > May be due to missing lock nesting notation > > > > > > > > 3 locks held by kworker/u5:0/35: > > > > #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at: > > > > process_one_work+0x750/0x930 > > > > #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, > > > > at: process_one_work+0x44e/0x930 > > > > #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: > > > > l2cap_get_chan_by_scid+0xaf/0xd0 > > > > > > > > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so > > > > perhaps we can just do: > > > > > > > > sk = chan->data; > > > > if (!sk) > > > > return -ENXIO; > > > > > > If the release occurs after this judgment, the same problem will still occur. > > > Recv and release must be synchronized using locks, which can be solved by > > > adding new lock. > > > > > > Please use the new patch https://syzkaller.appspot.com/text?tag=Patch&x=15d2c48e980000, I have tested in > > > https://syzkaller.appspot.com/bug?extid=b7f6f8c9303466e16c8a > > > > Hmm, why don't we just fix l2cap_conless_channel? Btw, > > l2cap_conless_channel is normally not used by any profiles thus why > > there isn't any CI covering it, on the other hand l2cap_data_channel > > is used by 99% of the profiles. > Yes, we can fix l2cap_conless_channel, but key point is that "chan->lock" > cannot be used to synchronize l2cap_conless_channel and l2cap_sock_release, > otherwise it will form an AA lock with l2cap_data_channel. Don't follow, what l2cap_conless_channel has to do with l2cap_data_channel? You can't have the same channel calling both, it is either connectionless or it is not. > Why not fix it in l2cap_conless_channel but in l2cap_sock_recv_cb, because > l2cap_sock_recv_cb is on the same layer as l2cap_sock_kill, using a new > lock for synchronization is more appropriate. There are dedicated locks for both layers and now that we are doing chan->lock for before chan->recv the locking sequence should be the same in all instances. > -- > Edward >
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index 6db60946c627..f45cdf9bc985 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; }
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> --- net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)