Hi Edward,
On Tue, Jun 11, 2024 at 10:50 AM Edward Adam Davis <[email protected]> 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.
>
> Reported-and-tested-by: [email protected]
> Signed-off-by: Edward Adam Davis <[email protected]>
> ---
> net/bluetooth/l2cap_sock.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 6db60946c627..f3e9236293e1 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1413,6 +1413,8 @@ static int l2cap_sock_release(struct socket *sock)
> l2cap_chan_hold(chan);
> l2cap_chan_lock(chan);
>
> + if (refcount_read(&sk->sk_refcnt) == 1)
> + chan->data = NULL;
Might be a good idea to add some comment on why checking for refcount
== 1 is the right thing to do here, or perhaps we can always assign
chan->data to NULL, instead of that perhaps we could have it done in
l2cap_sock_kill?
> sock_orphan(sk);
> l2cap_sock_kill(sk);
>
> @@ -1481,12 +1483,22 @@ 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);
> + 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 +1547,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
>
--
Luiz Augusto von Dentz
Hi Luiz Augusto von Dentz,
On Tue, 11 Jun 2024 15:24:52 -0400, Luiz Augusto von Dentz 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.
> >
> > Reported-and-tested-by: [email protected]
> > Signed-off-by: Edward Adam Davis <[email protected]>
> > ---
> > net/bluetooth/l2cap_sock.c | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 6db60946c627..f3e9236293e1 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -1413,6 +1413,8 @@ static int l2cap_sock_release(struct socket *sock)
> > l2cap_chan_hold(chan);
> > l2cap_chan_lock(chan);
> >
> > + if (refcount_read(&sk->sk_refcnt) == 1)
> > + chan->data = NULL;
>
> Might be a good idea to add some comment on why checking for refcount
> == 1 is the right thing to do here, or perhaps we can always assign
> chan->data to NULL, instead of that perhaps we could have it done in
> l2cap_sock_kill?
In this case, it is possible to always set chan->data to NULL, but I think a
better approach would be to release sock in l2cap_sock_kill when the reference
count of the sock is 1. Previously, it was mentioned that setting chan->data to
NULL is more rigorous.
And chan->data is bound one-on-one to the sock, if the sock is not released,
I can't confirm whether setting chan->data to NULL will affect the operation of
the sock on other paths.
>
> > sock_orphan(sk);
> > l2cap_sock_kill(sk);
> >
> > @@ -1481,12 +1483,22 @@ 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);
> > + 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 +1547,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;
> > }
--
Edward
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: [email protected]
Signed-off-by: Edward Adam Davis <[email protected]>
---
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