Linux-Bluetooth Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context
@ 2019-01-03  0:11 Matthias Kaehlcke
  2019-01-07 18:50 ` Doug Anderson
  2019-01-18 10:49 ` Marcel Holtmann
  0 siblings, 2 replies; 3+ messages in thread
From: Matthias Kaehlcke @ 2019-01-03  0:11 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, David S . Miller, Dean Jenkins
  Cc: linux-bluetooth, linux-kernel, netdev, Konstantin Khlebnikov,
	Balakrishna Godavarthi, Douglas Anderson, Dmitry Grinberg,
	Matthias Kaehlcke

With commit e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket
atomically") lock_sock[_nested]() is used to acquire the socket lock
before manipulating the socket. lock_sock[_nested]() may block, which
is problematic since bt_accept_enqueue() can be called in bottom half
context (e.g. from rfcomm_connect_ind()):

[<ffffff80080d81ec>] __might_sleep+0x4c/0x80
[<ffffff800876c7b0>] lock_sock_nested+0x24/0x58
[<ffffff8000d7c27c>] bt_accept_enqueue+0x48/0xd4 [bluetooth]
[<ffffff8000e67d8c>] rfcomm_connect_ind+0x190/0x218 [rfcomm]

Add a parameter to bt_accept_enqueue() to indicate whether the
function is called from BH context, and acquire the socket lock
with bh_lock_sock_nested() if that's the case.

Also adapt all callers of bt_accept_enqueue() to pass the new
parameter:

- l2cap_sock_new_connection_cb()
  - uses lock_sock() to lock the parent socket => process context

- rfcomm_connect_ind()
  - acquires the parent socket lock with bh_lock_sock() => BH
    context

- __sco_chan_add()
  - called from sco_chan_add(), which is called from sco_connect().
    parent is NULL, hence bt_accept_enqueue() isn't called in this
    code path and we can ignore it
  - also called from sco_conn_ready(). uses bh_lock_sock() to acquire
    the parent lock => BH context

Fixes: e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket atomically")
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- use parameter in bt_accept_enqueue() to decide which lock to
  acquire and adapt all callers
- updated commit message
---
 include/net/bluetooth/bluetooth.h |  2 +-
 net/bluetooth/af_bluetooth.c      | 16 +++++++++++++---
 net/bluetooth/l2cap_sock.c        |  2 +-
 net/bluetooth/rfcomm/sock.c       |  2 +-
 net/bluetooth/sco.c               |  2 +-
 5 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index ec9d6bc658559..fabee6db0abb7 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -276,7 +276,7 @@ int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
 int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
 int  bt_sock_wait_ready(struct sock *sk, unsigned long flags);
 
-void bt_accept_enqueue(struct sock *parent, struct sock *sk);
+void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh);
 void bt_accept_unlink(struct sock *sk);
 struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
 
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index deacc52d7ff18..8d12198eaa949 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -154,15 +154,25 @@ void bt_sock_unlink(struct bt_sock_list *l, struct sock *sk)
 }
 EXPORT_SYMBOL(bt_sock_unlink);
 
-void bt_accept_enqueue(struct sock *parent, struct sock *sk)
+void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
 {
 	BT_DBG("parent %p, sk %p", parent, sk);
 
 	sock_hold(sk);
-	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+
+	if (bh)
+		bh_lock_sock_nested(sk);
+	else
+		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+
 	list_add_tail(&bt_sk(sk)->accept_q, &bt_sk(parent)->accept_q);
 	bt_sk(sk)->parent = parent;
-	release_sock(sk);
+
+	if (bh)
+		bh_unlock_sock(sk);
+	else
+		release_sock(sk);
+
 	parent->sk_ack_backlog++;
 }
 EXPORT_SYMBOL(bt_accept_enqueue);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 686bdc6b35b03..a3a2cd55e23a9 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1252,7 +1252,7 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
 
 	l2cap_sock_init(sk, parent);
 
-	bt_accept_enqueue(parent, sk);
+	bt_accept_enqueue(parent, sk, false);
 
 	release_sock(parent);
 
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index aa0db1d1bd9b4..b1f49fcc04780 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -988,7 +988,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 	rfcomm_pi(sk)->channel = channel;
 
 	sk->sk_state = BT_CONFIG;
-	bt_accept_enqueue(parent, sk);
+	bt_accept_enqueue(parent, sk, true);
 
 	/* Accept connection and return socket DLC */
 	*d = rfcomm_pi(sk)->dlc;
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 529b38996d8bc..9a580999ca57e 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -193,7 +193,7 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
 	conn->sk = sk;
 
 	if (parent)
-		bt_accept_enqueue(parent, sk);
+		bt_accept_enqueue(parent, sk, true);
 }
 
 static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
-- 
2.20.1.415.g653613c723-goog


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context
  2019-01-03  0:11 [PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context Matthias Kaehlcke
@ 2019-01-07 18:50 ` Doug Anderson
  2019-01-18 10:49 ` Marcel Holtmann
  1 sibling, 0 replies; 3+ messages in thread
From: Doug Anderson @ 2019-01-07 18:50 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Dean Jenkins,
	linux-bluetooth, LKML, netdev, Konstantin Khlebnikov,
	Balakrishna Godavarthi, Dmitry Grinberg, stable

Hi,

On Wed, Jan 2, 2019 at 4:11 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> With commit e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket
> atomically") lock_sock[_nested]() is used to acquire the socket lock
> before manipulating the socket. lock_sock[_nested]() may block, which
> is problematic since bt_accept_enqueue() can be called in bottom half
> context (e.g. from rfcomm_connect_ind()):
>
> [<ffffff80080d81ec>] __might_sleep+0x4c/0x80
> [<ffffff800876c7b0>] lock_sock_nested+0x24/0x58
> [<ffffff8000d7c27c>] bt_accept_enqueue+0x48/0xd4 [bluetooth]
> [<ffffff8000e67d8c>] rfcomm_connect_ind+0x190/0x218 [rfcomm]
>
> Add a parameter to bt_accept_enqueue() to indicate whether the
> function is called from BH context, and acquire the socket lock
> with bh_lock_sock_nested() if that's the case.
>
> Also adapt all callers of bt_accept_enqueue() to pass the new
> parameter:
>
> - l2cap_sock_new_connection_cb()
>   - uses lock_sock() to lock the parent socket => process context
>
> - rfcomm_connect_ind()
>   - acquires the parent socket lock with bh_lock_sock() => BH
>     context
>
> - __sco_chan_add()
>   - called from sco_chan_add(), which is called from sco_connect().
>     parent is NULL, hence bt_accept_enqueue() isn't called in this
>     code path and we can ignore it
>   - also called from sco_conn_ready(). uses bh_lock_sock() to acquire
>     the parent lock => BH context
>
> Fixes: e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket atomically")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - use parameter in bt_accept_enqueue() to decide which lock to
>   acquire and adapt all callers
> - updated commit message
> ---
>  include/net/bluetooth/bluetooth.h |  2 +-
>  net/bluetooth/af_bluetooth.c      | 16 +++++++++++++---
>  net/bluetooth/l2cap_sock.c        |  2 +-
>  net/bluetooth/rfcomm/sock.c       |  2 +-
>  net/bluetooth/sco.c               |  2 +-
>  5 files changed, 17 insertions(+), 7 deletions(-)

This looks nice to me, thanks!

Cc: stable@vger.kernel.org
Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context
  2019-01-03  0:11 [PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context Matthias Kaehlcke
  2019-01-07 18:50 ` Doug Anderson
@ 2019-01-18 10:49 ` Marcel Holtmann
  1 sibling, 0 replies; 3+ messages in thread
From: Marcel Holtmann @ 2019-01-18 10:49 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Johan Hedberg, David S. Miller, Dean Jenkins, Bluez mailing list,
	Linux Kernel Mailing List, netdev, Konstantin Khlebnikov,
	Balakrishna Godavarthi, Douglas Anderson, Dmitry Grinberg

Hi Matthias,

> With commit e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket
> atomically") lock_sock[_nested]() is used to acquire the socket lock
> before manipulating the socket. lock_sock[_nested]() may block, which
> is problematic since bt_accept_enqueue() can be called in bottom half
> context (e.g. from rfcomm_connect_ind()):
> 
> [<ffffff80080d81ec>] __might_sleep+0x4c/0x80
> [<ffffff800876c7b0>] lock_sock_nested+0x24/0x58
> [<ffffff8000d7c27c>] bt_accept_enqueue+0x48/0xd4 [bluetooth]
> [<ffffff8000e67d8c>] rfcomm_connect_ind+0x190/0x218 [rfcomm]
> 
> Add a parameter to bt_accept_enqueue() to indicate whether the
> function is called from BH context, and acquire the socket lock
> with bh_lock_sock_nested() if that's the case.
> 
> Also adapt all callers of bt_accept_enqueue() to pass the new
> parameter:
> 
> - l2cap_sock_new_connection_cb()
>  - uses lock_sock() to lock the parent socket => process context
> 
> - rfcomm_connect_ind()
>  - acquires the parent socket lock with bh_lock_sock() => BH
>    context
> 
> - __sco_chan_add()
>  - called from sco_chan_add(), which is called from sco_connect().
>    parent is NULL, hence bt_accept_enqueue() isn't called in this
>    code path and we can ignore it
>  - also called from sco_conn_ready(). uses bh_lock_sock() to acquire
>    the parent lock => BH context
> 
> Fixes: e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket atomically")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - use parameter in bt_accept_enqueue() to decide which lock to
>  acquire and adapt all callers
> - updated commit message
> ---
> include/net/bluetooth/bluetooth.h |  2 +-
> net/bluetooth/af_bluetooth.c      | 16 +++++++++++++---
> net/bluetooth/l2cap_sock.c        |  2 +-
> net/bluetooth/rfcomm/sock.c       |  2 +-
> net/bluetooth/sco.c               |  2 +-
> 5 files changed, 17 insertions(+), 7 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-03  0:11 [PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context Matthias Kaehlcke
2019-01-07 18:50 ` Doug Anderson
2019-01-18 10:49 ` Marcel Holtmann

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org linux-bluetooth@archiver.kernel.org
	public-inbox-index linux-bluetooth


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/ public-inbox