[PATCH v2 0/2] Bluetooth: Lock nesting for L2CAP channels and sockets

[PATCH v2 0/2] Bluetooth: Lock nesting for L2CAP channels and sockets

[johan.hedberg]

Hi,

v2 has minor fixes to a code comment in patch 1/2 and the commit message
of patch 2/2.

>From the original cover letter:

I've spent the last few days fighting with all sorts of lockdep warnings
in the Bluetooth subsystem. A lot of them come from the fact that we
have cross-channel access for L2CAP channels. This ends up looking
suspicious to lockdep unless we help out a bit by adding nesting level
annotations.

It seems like we need at least three distinct nesting levels: SMP
channels, "normal" channels, and parent/listening channels. This is
because the parent channel lock may be held while also taking the lock
of child channels, and the SMP channel may be accessed while holding the
lock of any other channel (e.g. to request security level elevation).

We could in theory identify the type of channel in question implicitly
by inspecting the channel in the l2cap_chan_lock function, however this
is problematic since accessing channel data would usually already
require holding the channel lock (not to mention that the logic for
selecting the needed nesting level wouldn't be completely trivial).

A simpler (imo) solution that I've gone for is to introduce a new
"atomic_t nesting" channel member which contains the necessary nesting
level and is set as soon as the level is known (i.e. initialized to
"normal" upon channel creation or set to one of the two other levels
when creating the SMP chan in smp.c or when setting chan->state to
BT_LISTEN).

The second patch contains a long needed fix for l2cap_sock_teardown_cb
which only got a half-fix earlier this week (also from me) for its
locking. This function gets called for all types of channels so it can't
have a hard-coded nesting level for the socket lock. Instead I've opted
for the socket lock inheriting the chan level and then also using that
for the two other places calling lock_sock_nested() to be consistent.

Johan

----------------------------------------------------------------
Johan Hedberg (2):
      Bluetooth: Use proper nesting annotation for l2cap_chan lock
      Bluetooth: Fix L2CAP socket lock nesting level

 include/net/bluetooth/l2cap.h | 15 ++++++++++++++-
 net/bluetooth/l2cap_sock.c    | 22 +++++++++++++++++++---
 net/bluetooth/smp.c           | 10 ++++++++++
 3 files changed, 43 insertions(+), 4 deletions(-)

[PATCH v2 1/2] Bluetooth: Use proper nesting annotation for l2cap_chan lock

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

By default lockdep considers all L2CAP channels equal. This would mean
that we get warnings if a channel is locked when another one's lock is
tried to be acquired in the same thread. This kind of inter-channel
locking dependencies exist in the form of parent-child channels as well
as any channel wishing to elevate the security by requesting procedures
on the SMP channel.

To eliminate the chance for these lockdep warnings we introduce a
nesting level for each channel and use that when acquiring the channel
lock. For now there exists the earlier mentioned three identified
categories: SMP, "normal" channels and parent channels (i.e. those in
BT_LISTEN state). The nesting level is defined as atomic_t since we need
access to it before the lock is actually acquired.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/l2cap.h | 15 ++++++++++++++-
 net/bluetooth/l2cap_sock.c    |  9 +++++++++
 net/bluetooth/smp.c           | 10 ++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index ead99f032f7a..061e648052c8 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -28,6 +28,7 @@
 #define __L2CAP_H
 
 #include <asm/unaligned.h>
+#include <linux/atomic.h>
 
 /* L2CAP defaults */
 #define L2CAP_DEFAULT_MTU		672
@@ -481,6 +482,7 @@ struct l2cap_chan {
 	struct hci_conn		*hs_hcon;
 	struct hci_chan		*hs_hchan;
 	struct kref	kref;
+	atomic_t	nesting;
 
 	__u8		state;
 
@@ -713,6 +715,17 @@ enum {
 	FLAG_HOLD_HCI_CONN,
 };
 
+/* Lock nesting levels for L2CAP channels. We need these because lockdep
+ * otherwise considers all channels equal and will e.g. complain about a
+ * connection oriented channel triggering SMP procedures or a listening
+ * channel creating and locking a child channel.
+ */
+enum {
+	L2CAP_NESTING_SMP,
+	L2CAP_NESTING_NORMAL,
+	L2CAP_NESTING_PARENT,
+};
+
 enum {
 	L2CAP_TX_STATE_XMIT,
 	L2CAP_TX_STATE_WAIT_F,
@@ -778,7 +791,7 @@ void l2cap_chan_put(struct l2cap_chan *c);
 
 static inline void l2cap_chan_lock(struct l2cap_chan *chan)
 {
-	mutex_lock(&chan->lock);
+	mutex_lock_nested(&chan->lock, atomic_read(&chan->nesting));
 }
 
 static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ad1cf82fee02..f1a51564b8fd 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -285,6 +285,12 @@ static int l2cap_sock_listen(struct socket *sock, int backlog)
 	sk->sk_max_ack_backlog = backlog;
 	sk->sk_ack_backlog = 0;
 
+	/* Listening channels need to use nested locking in order not to
+	 * cause lockdep warnings when the created child channels end up
+	 * being locked in the same thread as the parent channel.
+	 */
+	atomic_set(&chan->nesting, L2CAP_NESTING_PARENT);
+
 	chan->state = BT_LISTEN;
 	sk->sk_state = BT_LISTEN;
 
@@ -1497,6 +1503,9 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
 		l2cap_chan_set_defaults(chan);
 	}
 
+	/* Set default lock nesting level */
+	atomic_set(&chan->nesting, L2CAP_NESTING_NORMAL);
+
 	/* Default config options */
 	chan->flush_to = L2CAP_DEFAULT_FLUSH_TO;
 
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 3d38553eb526..3b63c7f09dd5 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1658,6 +1658,13 @@ static inline struct l2cap_chan *smp_new_conn_cb(struct l2cap_chan *pchan)
 	chan->omtu	= pchan->omtu;
 	chan->mode	= pchan->mode;
 
+	/* Other L2CAP channels may request SMP routines in order to
+	 * change the security level. This means that the SMP channel
+	 * lock must be considered in its own category to avoid lockdep
+	 * warnings.
+	 */
+	atomic_set(&chan->nesting, L2CAP_NESTING_SMP);
+
 	BT_DBG("created chan %p", chan);
 
 	return chan;
@@ -1715,6 +1722,9 @@ int smp_register(struct hci_dev *hdev)
 	chan->imtu = L2CAP_DEFAULT_MTU;
 	chan->ops = &smp_root_chan_ops;
 
+	/* Set correct nesting level for a parent/listening channel */
+	atomic_set(&chan->nesting, L2CAP_NESTING_PARENT);
+
 	hdev->smp_data = chan;
 
 	return 0;
-- 
2.1.0

[PATCH v2 2/2] Bluetooth: Fix L2CAP socket lock nesting level

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The teardown callback for L2CAP channels is problematic in that it is
explicitly called for all types of channels from l2cap_chan_del(),
meaning it's not possible to hard-code a nesting level when taking the
socket lock. The simplest way to have a correct nesting level for the
socket locking is to use the same value as for the chan. This also means
that the other places trying to lock parent sockets need to be update to
use the chan value (since L2CAP_NESTING_PARENT is defined as 2 whereas
SINGLE_DEPTH_NESTING has the value 1).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_sock.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index f1a51564b8fd..7913c28c643d 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -307,7 +307,7 @@ static int l2cap_sock_accept(struct socket *sock, struct socket *newsock,
 	long timeo;
 	int err = 0;
 
-	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+	lock_sock_nested(sk, L2CAP_NESTING_PARENT);
 
 	timeo = sock_rcvtimeo(sk, flags & O_NONBLOCK);
 
@@ -339,7 +339,7 @@ static int l2cap_sock_accept(struct socket *sock, struct socket *newsock,
 
 		release_sock(sk);
 		timeo = schedule_timeout(timeo);
-		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+		lock_sock_nested(sk, L2CAP_NESTING_PARENT);
 	}
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(sk_sleep(sk), &wait);
@@ -1252,7 +1252,14 @@ static void l2cap_sock_teardown_cb(struct l2cap_chan *chan, int err)
 	struct sock *sk = chan->data;
 	struct sock *parent;
 
-	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+	/* This callback can be called both for server (BT_LISTEN)
+	 * sockets as well as "normal" ones. To avoid lockdep warnings
+	 * with child socket locking (through l2cap_sock_cleanup_listen)
+	 * we need separation into separate nesting levels. The simplest
+	 * way to accomplish this is to inherit the nesting level used
+	 * for the channel.
+	 */
+	lock_sock_nested(sk, atomic_read(&chan->nesting));
 
 	parent = bt_sk(sk)->parent;
 
-- 
2.1.0

Re: [PATCH v2 0/2] Bluetooth: Lock nesting for L2CAP channels and sockets

[Marcel Holtmann]

Hi Johan,

> v2 has minor fixes to a code comment in patch 1/2 and the commit message
> of patch 2/2.
> 
> From the original cover letter:
> 
> I've spent the last few days fighting with all sorts of lockdep warnings
> in the Bluetooth subsystem. A lot of them come from the fact that we
> have cross-channel access for L2CAP channels. This ends up looking
> suspicious to lockdep unless we help out a bit by adding nesting level
> annotations.
> 
> It seems like we need at least three distinct nesting levels: SMP
> channels, "normal" channels, and parent/listening channels. This is
> because the parent channel lock may be held while also taking the lock
> of child channels, and the SMP channel may be accessed while holding the
> lock of any other channel (e.g. to request security level elevation).
> 
> We could in theory identify the type of channel in question implicitly
> by inspecting the channel in the l2cap_chan_lock function, however this
> is problematic since accessing channel data would usually already
> require holding the channel lock (not to mention that the logic for
> selecting the needed nesting level wouldn't be completely trivial).
> 
> A simpler (imo) solution that I've gone for is to introduce a new
> "atomic_t nesting" channel member which contains the necessary nesting
> level and is set as soon as the level is known (i.e. initialized to
> "normal" upon channel creation or set to one of the two other levels
> when creating the SMP chan in smp.c or when setting chan->state to
> BT_LISTEN).
> 
> The second patch contains a long needed fix for l2cap_sock_teardown_cb
> which only got a half-fix earlier this week (also from me) for its
> locking. This function gets called for all types of channels so it can't
> have a hard-coded nesting level for the socket lock. Instead I've opted
> for the socket lock inheriting the chan level and then also using that
> for the two other places calling lock_sock_nested() to be consistent.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (2):
>      Bluetooth: Use proper nesting annotation for l2cap_chan lock
>      Bluetooth: Fix L2CAP socket lock nesting level
> 
> include/net/bluetooth/l2cap.h | 15 ++++++++++++++-
> net/bluetooth/l2cap_sock.c    | 22 +++++++++++++++++++---
> net/bluetooth/smp.c           | 10 ++++++++++
> 3 files changed, 43 insertions(+), 4 deletions(-)

both patches have been applied to bluetooth-next tree.

Regards

Marcel