[RFC PATCH] Bluetooth: SCO: remove sco_conn and simplify locking scheme

[RFC PATCH] Bluetooth: SCO: remove sco_conn and simplify locking scheme

[Pauli Virtanen]

The sco_conn struct is not needed, as every SCO hci_conn is associated
with at most one SCO socket at a time. It adds unnecessary complexity in
lifetime and locking management, which has led to bugs.  E.g. access to
hci_conn::sco_data currently has a race in sco_conn_hold_unless_zero vs.
sco_conn_put->sco_conn_free in sco_chan_del.

Simplify by removing sco_conn and moving all fields to sco_pi.
The sco_pi(sk)->hcon field is guarded by lock_sock(sk).

Add lock hci_conn::proto_lock for protocols to use.  Make
hci_conn::sco_data guarded by RCU, and use that lock for updates.  The
RCU usage relies on SOCK_RCU_FREE, which moves destruct to call_rcu, so
it can be paired with refcount_inc_not_zero(&sk->sk_refcnt) as done in
other net/ protocols.

sk owns hci_conn_hold logical refcount when sco_pi(sk)->hcon != NULL.

Enable Clang context analysis for sco.c, and add relevant __must_hold
and lockdep markers.

This fixes: race on sco_data, calling sleeping functions under spinlock
in sco_conn_ready, dubious sco_conn locking & sk refcounting in
sco_conn_ready, missing locking in getsockopt / setsockopt.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    It's probably possible to get rid of the proto_lock, but this requires
    some custom memory ordering reasoning, so probably best avoided.

 include/net/bluetooth/hci_core.h |   6 +-
 net/bluetooth/Makefile           |   2 +
 net/bluetooth/hci_conn.c         |   2 +
 net/bluetooth/sco.c              | 458 +++++++++++++------------------
 4 files changed, 196 insertions(+), 272 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a7bffb908c1e..555c2ce2dd15 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -770,8 +770,12 @@ struct hci_conn {
 	struct dentry	*debugfs;
 
 	struct hci_dev	*hdev;
+
+	/* Spinlock to guard sco_data */
+	spinlock_t	proto_lock;
+
 	void		*l2cap_data;
-	void		*sco_data;
+	void __rcu	*sco_data;
 	void		*iso_data;
 
 	struct list_head link_list;
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index a7eede7616d8..5fcc93fdd54e 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -26,3 +26,5 @@ bluetooth-$(CONFIG_BT_MSFTEXT) += msft.o
 bluetooth-$(CONFIG_BT_AOSPEXT) += aosp.o
 bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
 bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o
+
+CONTEXT_ANALYSIS_sco.o := y
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 3a0592599086..b8014f8bd844 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1057,6 +1057,8 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type,
 	INIT_DELAYED_WORK(&conn->idle_work, hci_conn_idle);
 	INIT_DELAYED_WORK(&conn->le_conn_timeout, le_conn_timeout);
 
+	spin_lock_init(&conn->proto_lock);
+
 	atomic_set(&conn->refcnt, 0);
 
 	hci_dev_hold(hdev);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 18826d4b9c0b..259bb45b0d65 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -42,21 +42,6 @@ static struct bt_sock_list sco_sk_list = {
 };
 
 /* ---- SCO connections ---- */
-struct sco_conn {
-	struct hci_conn	*hcon;
-
-	spinlock_t	lock;
-	struct sock	*sk;
-
-	struct delayed_work	timeout_work;
-
-	unsigned int    mtu;
-	struct kref	ref;
-};
-
-#define sco_conn_lock(c)	spin_lock(&c->lock)
-#define sco_conn_unlock(c)	spin_unlock(&c->lock)
-
 static void sco_sock_close(struct sock *sk);
 static void sco_sock_kill(struct sock *sk);
 
@@ -69,97 +54,21 @@ struct sco_pinfo {
 	bdaddr_t	dst;
 	__u32		flags;
 	__u16		setting;
+	unsigned int    mtu;
 	struct bt_codec codec;
-	struct sco_conn	*conn;
+	struct delayed_work timeout_work;
+	struct hci_conn *hcon;
 };
 
 /* ---- SCO timers ---- */
 #define SCO_CONN_TIMEOUT	(HZ * 40)
 #define SCO_DISCONN_TIMEOUT	(HZ * 2)
 
-static void sco_conn_free(struct kref *ref)
-{
-	struct sco_conn *conn = container_of(ref, struct sco_conn, ref);
-
-	BT_DBG("conn %p", conn);
-
-	if (conn->sk)
-		sco_pi(conn->sk)->conn = NULL;
-
-	if (conn->hcon) {
-		conn->hcon->sco_data = NULL;
-		hci_conn_drop(conn->hcon);
-	}
-
-	/* Ensure no more work items will run since hci_conn has been dropped */
-	disable_delayed_work_sync(&conn->timeout_work);
-
-	kfree(conn);
-}
-
-static void sco_conn_put(struct sco_conn *conn)
-{
-	if (!conn)
-		return;
-
-	BT_DBG("conn %p refcnt %d", conn, kref_read(&conn->ref));
-
-	kref_put(&conn->ref, sco_conn_free);
-}
-
-static struct sco_conn *sco_conn_hold(struct sco_conn *conn)
-{
-	BT_DBG("conn %p refcnt %u", conn, kref_read(&conn->ref));
-
-	kref_get(&conn->ref);
-	return conn;
-}
-
-static struct sco_conn *sco_conn_hold_unless_zero(struct sco_conn *conn)
-{
-	if (!conn)
-		return NULL;
-
-	BT_DBG("conn %p refcnt %u", conn, kref_read(&conn->ref));
-
-	if (!kref_get_unless_zero(&conn->ref))
-		return NULL;
-
-	return conn;
-}
-
-static struct sock *sco_sock_hold(struct sco_conn *conn)
-{
-	if (!conn || !bt_sock_linked(&sco_sk_list, conn->sk))
-		return NULL;
-
-	sock_hold(conn->sk);
-
-	return conn->sk;
-}
-
 static void sco_sock_timeout(struct work_struct *work)
 {
-	struct sco_conn *conn = container_of(work, struct sco_conn,
-					     timeout_work.work);
-	struct sock *sk;
-
-	conn = sco_conn_hold_unless_zero(conn);
-	if (!conn)
-		return;
-
-	sco_conn_lock(conn);
-	if (!conn->hcon) {
-		sco_conn_unlock(conn);
-		sco_conn_put(conn);
-		return;
-	}
-	sk = sco_sock_hold(conn);
-	sco_conn_unlock(conn);
-	sco_conn_put(conn);
-
-	if (!sk)
-		return;
+	struct sco_pinfo *pi = container_of(work, struct sco_pinfo,
+					    timeout_work.work);
+	struct sock *sk = (struct sock *)pi;
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
 
@@ -167,82 +76,94 @@ static void sco_sock_timeout(struct work_struct *work)
 	sk->sk_err = ETIMEDOUT;
 	sk->sk_state_change(sk);
 	release_sock(sk);
-	sock_put(sk);
 }
 
 static void sco_sock_set_timer(struct sock *sk, long timeout)
 {
-	if (!sco_pi(sk)->conn)
-		return;
-
 	BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
-	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
-	schedule_delayed_work(&sco_pi(sk)->conn->timeout_work, timeout);
+	cancel_delayed_work(&sco_pi(sk)->timeout_work);
+	schedule_delayed_work(&sco_pi(sk)->timeout_work, timeout);
 }
 
 static void sco_sock_clear_timer(struct sock *sk)
 {
-	if (!sco_pi(sk)->conn)
-		return;
-
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
-	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
+	cancel_delayed_work(&sco_pi(sk)->timeout_work);
 }
 
 /* ---- SCO connections ---- */
-static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
+static struct sock *__sco_peek_sk(struct hci_conn *hcon)
+	__must_hold(&hcon->proto_lock)
 {
-	struct sco_conn *conn = hcon->sco_data;
-
-	conn = sco_conn_hold_unless_zero(conn);
-	if (conn) {
-		if (!conn->hcon) {
-			sco_conn_lock(conn);
-			conn->hcon = hcon;
-			sco_conn_unlock(conn);
-		}
-		return conn;
-	}
-
-	conn = kzalloc_obj(struct sco_conn);
-	if (!conn)
-		return NULL;
-
-	kref_init(&conn->ref);
-	spin_lock_init(&conn->lock);
-	INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
-
-	hcon->sco_data = conn;
-	conn->hcon = hcon;
-	conn->mtu = hcon->mtu;
-
-	if (hcon->mtu > 0)
-		conn->mtu = hcon->mtu;
-	else
-		conn->mtu = 60;
-
-	BT_DBG("hcon %p conn %p", hcon, conn);
-
-	return conn;
+	return rcu_dereference_protected(hcon->sco_data,
+					 lockdep_is_held(&hcon->proto_lock));
+}
+
+static struct sock *__sco_get_sk(struct hci_conn *hcon)
+{
+	struct sock *sk;
+
+	sk = rcu_dereference(hcon->sco_data);
+
+	if (sk && !refcount_inc_not_zero(&sk->sk_refcnt))
+		sk = NULL;
+
+	return sk;
+}
+
+static struct sock *sco_get_sk(struct hci_conn *hcon)
+{
+	struct sock *sk;
+
+	rcu_read_lock();
+	sk = __sco_get_sk(hcon);
+	rcu_read_unlock();
+	if (!sk)
+		return NULL;
+
+	lock_sock(sk);
+
+	/* Deal with sk detached in the meantime */
+	if (sco_pi(sk)->hcon != hcon) {
+		release_sock(sk);
+		sock_put(sk);
+		return NULL;
+	}
+
+	return sk;
+}
+
+static struct hci_conn *sco_sk_hcon(struct sock *sk)
+{
+	lockdep_assert(lockdep_sock_is_held(sk));
+	return sco_pi(sk)->hcon;
+}
+
+static void sco_sk_detach(struct sock *sk)
+{
+	struct hci_conn *hcon = sco_sk_hcon(sk);
+
+	if (hcon) {
+		/* RCU update (see sco_chan_add) */
+		spin_lock(&hcon->proto_lock);
+
+		lockdep_assert(__sco_peek_sk(hcon) == sk);
+		rcu_assign_pointer(hcon->sco_data, NULL);
+		sco_pi(sk)->hcon = NULL;
+
+		spin_unlock(&hcon->proto_lock);
+
+		hci_conn_drop(hcon);
+	}
 }
 
-/* Delete channel.
- * Must be called on the locked socket. */
 static void sco_chan_del(struct sock *sk, int err)
 {
-	struct sco_conn *conn;
+	BT_DBG("sk %p, hcon %p, err %d", sk, sco_sk_hcon(sk), err);
 
-	conn = sco_pi(sk)->conn;
-	sco_pi(sk)->conn = NULL;
+	lockdep_assert(lockdep_sock_is_held(sk));
 
-	BT_DBG("sk %p, conn %p, err %d", sk, conn, err);
-
-	if (conn) {
-		sco_conn_lock(conn);
-		conn->sk = NULL;
-		sco_conn_unlock(conn);
-		sco_conn_put(conn);
-	}
+	sco_sk_detach(sk);
 
 	sk->sk_state = BT_CLOSED;
 	sk->sk_err   = err;
@@ -252,64 +173,67 @@ static void sco_chan_del(struct sock *sk, int err)
 }
 
 static void sco_conn_del(struct hci_conn *hcon, int err)
+	__must_hold(&hcon->hdev->lock)
 {
-	struct sco_conn *conn = hcon->sco_data;
-	struct sock *sk;
+	struct sock *sk = sco_get_sk(hcon);
 
-	conn = sco_conn_hold_unless_zero(conn);
-	if (!conn)
+	if (!sk)
 		return;
 
-	BT_DBG("hcon %p conn %p, err %d", hcon, conn, err);
-
-	sco_conn_lock(conn);
-	sk = sco_sock_hold(conn);
-	sco_conn_unlock(conn);
-	sco_conn_put(conn);
-
-	if (!sk) {
-		sco_conn_put(conn);
-		return;
-	}
-
 	/* Kill socket */
-	lock_sock(sk);
 	sco_sock_clear_timer(sk);
 	sco_chan_del(sk, err);
 	release_sock(sk);
 	sock_put(sk);
 }
 
-static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
+static void __sco_chan_add(struct hci_conn *hcon, struct sock *sk,
 			   struct sock *parent)
+	__must_hold(&hcon->hdev->lock)
+	__must_hold(&hcon->proto_lock)
 {
-	BT_DBG("conn %p", conn);
+	BT_DBG("hcon %p sk %p mtu %d", hcon, sk, (int)hcon->mtu);
 
-	sco_pi(sk)->conn = conn;
-	conn->sk = sk;
+	/* If parent, sk is not yet visible to user tasks, so no sk lock needed.
+	 */
+	lockdep_assert(parent || lockdep_sock_is_held(sk));
+	lockdep_assert_held(&hcon->hdev->lock);
+	lockdep_assert_held(&hcon->proto_lock);
+
+	if (hcon->mtu > 0)
+		sco_pi(sk)->mtu = hcon->mtu;
+	else
+		sco_pi(sk)->mtu = 60;
+
+	sock_set_flag(sk, SOCK_RCU_FREE);
+
+	sco_pi(sk)->hcon = hci_conn_hold(hcon);
+	rcu_assign_pointer(hcon->sco_data, sk);
 
 	if (parent)
 		bt_accept_enqueue(parent, sk, true);
 }
 
-static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
+static int sco_chan_add(struct hci_conn *hcon, struct sock *sk,
 			struct sock *parent)
+	__must_hold(&hcon->hdev->lock)
 {
 	int err = 0;
 
-	sco_conn_lock(conn);
-	if (conn->sk || sco_pi(sk)->conn)
+	spin_lock(&hcon->proto_lock);
+
+	if (__sco_peek_sk(hcon) || sco_pi(sk)->hcon)
 		err = -EBUSY;
 	else
-		__sco_chan_add(conn, sk, parent);
+		__sco_chan_add(hcon, sk, parent);
+
+	spin_unlock(&hcon->proto_lock);
 
-	sco_conn_unlock(conn);
 	return err;
 }
 
 static int sco_connect(struct sock *sk)
 {
-	struct sco_conn *conn;
 	struct hci_conn *hcon;
 	struct hci_dev  *hdev;
 	int err, type;
@@ -344,13 +268,6 @@ static int sco_connect(struct sock *sk)
 		goto unlock;
 	}
 
-	conn = sco_conn_add(hcon);
-	if (!conn) {
-		hci_conn_drop(hcon);
-		err = -ENOMEM;
-		goto unlock;
-	}
-
 	lock_sock(sk);
 
 	/* Recheck state after reacquiring the socket lock, as another
@@ -363,10 +280,12 @@ static int sco_connect(struct sock *sk)
 		goto unlock;
 	}
 
-	err = sco_chan_add(conn, sk, NULL);
+	lockdep_assert_held(&hcon->hdev->lock);
+
+	err = sco_chan_add(hcon, sk, NULL);
+	hci_conn_drop(hcon);
 	if (err) {
 		release_sock(sk);
-		hci_conn_drop(hcon);
 		goto unlock;
 	}
 
@@ -392,44 +311,35 @@ static int sco_connect(struct sock *sk)
 static int sco_send_frame(struct sock *sk, struct sk_buff *skb,
 			  const struct sockcm_cookie *sockc)
 {
-	struct sco_conn *conn = sco_pi(sk)->conn;
+	struct hci_conn *hcon = sco_sk_hcon(sk);
 	int len = skb->len;
 
 	/* Check outgoing MTU */
-	if (len > conn->mtu)
+	if (len > sco_pi(sk)->mtu)
 		return -EINVAL;
+	if (!hcon)
+		return -ENOTCONN;
 
 	BT_DBG("sk %p len %d", sk, len);
 
 	hci_setup_tx_timestamp(skb, 1, sockc);
-	hci_send_sco(conn->hcon, skb);
+	hci_send_sco(hcon, skb);
 
 	return len;
 }
 
-static void sco_recv_frame(struct sco_conn *conn, struct sk_buff *skb)
+static void sco_recv_frame(struct sock *sk, struct sk_buff *skb)
 {
-	struct sock *sk;
-
-	sco_conn_lock(conn);
-	sk = sco_sock_hold(conn);
-	sco_conn_unlock(conn);
-
-	if (!sk)
-		goto drop;
-
 	BT_DBG("sk %p len %u", sk, skb->len);
 
-	if (sk->sk_state != BT_CONNECTED)
-		goto drop_put;
+	/* NOTE: sk not locked */
 
-	if (!sock_queue_rcv_skb(sk, skb)) {
-		sock_put(sk);
+	if (READ_ONCE(sk->sk_state) != BT_CONNECTED)
+		goto drop;
+
+	if (!sock_queue_rcv_skb(sk, skb))
 		return;
-	}
 
-drop_put:
-	sock_put(sk);
 drop:
 	kfree_skb(skb);
 }
@@ -472,16 +382,20 @@ static struct sock *sco_get_sock_listen(bdaddr_t *src)
 			sk1 = sk;
 	}
 
+	sk = sk ? sk : sk1;
+	if (sk)
+		sock_hold(sk);
+
 	read_unlock(&sco_sk_list.lock);
 
-	return sk ? sk : sk1;
+	return sk;
 }
 
 static void sco_sock_destruct(struct sock *sk)
 {
 	BT_DBG("sk %p", sk);
 
-	sco_conn_put(sco_pi(sk)->conn);
+	WARN_ON(sco_pi(sk)->hcon);
 
 	skb_queue_purge(&sk->sk_receive_queue);
 	skb_queue_purge(&sk->sk_write_queue);
@@ -514,12 +428,11 @@ static void sco_sock_kill(struct sock *sk)
 
 	BT_DBG("sk %p state %d", sk, sk->sk_state);
 
-	/* Sock is dead, so set conn->sk to NULL to avoid possible UAF */
-	if (sco_pi(sk)->conn) {
-		sco_conn_lock(sco_pi(sk)->conn);
-		sco_pi(sk)->conn->sk = NULL;
-		sco_conn_unlock(sco_pi(sk)->conn);
-	}
+	disable_delayed_work_sync(&sco_pi(sk)->timeout_work);
+
+	lock_sock(sk);
+	sco_sk_detach(sk);
+	release_sock(sk);
 
 	/* Kill poor orphan */
 	bt_sock_unlink(&sco_sk_list, sk);
@@ -595,6 +508,8 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
 	sco_pi(sk)->codec.vid = 0xffff;
 	sco_pi(sk)->codec.data_path = 0x00;
 
+	INIT_DELAYED_WORK(&sco_pi(sk)->timeout_work, sco_sock_timeout);
+
 	bt_sock_link(&sco_sk_list, sk);
 	return sk;
 }
@@ -905,7 +820,6 @@ static int sco_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 			    size_t len, int flags)
 {
 	struct sock *sk = sock->sk;
-	struct sco_pinfo *pi = sco_pi(sk);
 
 	if (unlikely(flags & MSG_ERRQUEUE))
 		return sock_recv_errqueue(sk, msg, len, SOL_BLUETOOTH,
@@ -914,8 +828,9 @@ static int sco_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	lock_sock(sk);
 
 	if (sk->sk_state == BT_CONNECT2 &&
-	    test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
-		sco_conn_defer_accept(pi->conn->hcon, pi->setting);
+	    test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags) &&
+	    sco_sk_hcon(sk)) {
+		sco_conn_defer_accept(sco_sk_hcon(sk), sco_pi(sk)->setting);
 		sk->sk_state = BT_CONFIG;
 
 		release_sock(sk);
@@ -1090,7 +1005,7 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname,
 			break;
 		}
 
-		opts.mtu = sco_pi(sk)->conn->mtu;
+		opts.mtu = sco_pi(sk)->mtu;
 
 		BT_DBG("mtu %u", opts.mtu);
 
@@ -1108,9 +1023,14 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname,
 			break;
 		}
 
+		if (!sco_sk_hcon(sk)) {
+			err = -ENOTCONN;
+			break;
+		}
+
 		memset(&cinfo, 0, sizeof(cinfo));
-		cinfo.hci_handle = sco_pi(sk)->conn->hcon->handle;
-		memcpy(cinfo.dev_class, sco_pi(sk)->conn->hcon->dev_class, 3);
+		cinfo.hci_handle = sco_sk_hcon(sk)->handle;
+		memcpy(cinfo.dev_class, sco_sk_hcon(sk)->dev_class, 3);
 
 		len = min(len, sizeof(cinfo));
 		if (copy_to_user(optval, (char *)&cinfo, len))
@@ -1175,12 +1095,12 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case BT_PHY:
-		if (sk->sk_state != BT_CONNECTED) {
+		if (sk->sk_state != BT_CONNECTED || !sco_sk_hcon(sk)) {
 			err = -ENOTCONN;
 			break;
 		}
 
-		phys = hci_conn_get_phy(sco_pi(sk)->conn->hcon);
+		phys = hci_conn_get_phy(sco_sk_hcon(sk));
 
 		if (put_user(phys, (u32 __user *) optval))
 			err = -EFAULT;
@@ -1199,7 +1119,7 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		if (put_user(sco_pi(sk)->conn->mtu, (u32 __user *)optval))
+		if (put_user(sco_pi(sk)->mtu, (u32 __user *)optval))
 			err = -EFAULT;
 		break;
 
@@ -1363,51 +1283,46 @@ static int sco_sock_release(struct socket *sock)
 	return err;
 }
 
-static void sco_conn_ready(struct sco_conn *conn)
+static void sco_conn_ready(struct hci_conn *hcon)
 {
 	struct sock *parent;
-	struct sock *sk = conn->sk;
+	struct sock *sk;
 
-	BT_DBG("conn %p", conn);
+	lockdep_assert_held(&hcon->hdev->lock);
+
+	sk = sco_get_sk(hcon);
+
+	BT_DBG("hcon %p sk %p", hcon, sk);
 
 	if (sk) {
-		lock_sock(sk);
 		sco_sock_clear_timer(sk);
 		sk->sk_state = BT_CONNECTED;
 		sk->sk_state_change(sk);
 		release_sock(sk);
+		sock_put(sk);
 	} else {
-		sco_conn_lock(conn);
-
-		if (!conn->hcon) {
-			sco_conn_unlock(conn);
+		parent = sco_get_sock_listen(&hcon->src);
+		if (!parent)
 			return;
-		}
-
-		parent = sco_get_sock_listen(&conn->hcon->src);
-		if (!parent) {
-			sco_conn_unlock(conn);
-			return;
-		}
 
 		lock_sock(parent);
 
+		spin_lock(&hcon->proto_lock);
+
+		if (parent->sk_state != BT_LISTEN || __sco_peek_sk(hcon))
+			goto release;
+
 		sk = sco_sock_alloc(sock_net(parent), NULL,
 				    BTPROTO_SCO, GFP_ATOMIC, 0);
-		if (!sk) {
-			release_sock(parent);
-			sco_conn_unlock(conn);
-			return;
-		}
+		if (!sk)
+			goto release;
 
 		sco_sock_init(sk, parent);
 
-		bacpy(&sco_pi(sk)->src, &conn->hcon->src);
-		bacpy(&sco_pi(sk)->dst, &conn->hcon->dst);
+		bacpy(&sco_pi(sk)->src, &hcon->src);
+		bacpy(&sco_pi(sk)->dst, &hcon->dst);
 
-		sco_conn_hold(conn);
-		hci_conn_hold(conn->hcon);
-		__sco_chan_add(conn, sk, parent);
+		__sco_chan_add(hcon, sk, parent);
 
 		if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
 			sk->sk_state = BT_CONNECT2;
@@ -1417,9 +1332,10 @@ static void sco_conn_ready(struct sco_conn *conn)
 		/* Wake up parent */
 		parent->sk_data_ready(parent);
 
+release:
+		spin_unlock(&hcon->proto_lock);
 		release_sock(parent);
-
-		sco_conn_unlock(conn);
+		sock_put(parent);
 	}
 }
 
@@ -1452,26 +1368,26 @@ int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
 }
 
 static void sco_connect_cfm(struct hci_conn *hcon, __u8 status)
+	__must_hold(&hcon->hdev->lock)
 {
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK)
 		return;
 
 	BT_DBG("hcon %p bdaddr %pMR status %u", hcon, &hcon->dst, status);
 
-	if (!status) {
-		struct sco_conn *conn;
-
-		conn = sco_conn_add(hcon);
-		if (conn) {
-			sco_conn_ready(conn);
-			sco_conn_put(conn);
-		}
-	} else
+	if (!status)
+		sco_conn_ready(hcon);
+	else
 		sco_conn_del(hcon, bt_to_errno(status));
 }
 
 static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason)
+	__must_hold(&hcon->hdev->lock)
 {
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK)
 		return;
 
@@ -1483,35 +1399,35 @@ static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason)
 int sco_recv_scodata(struct hci_dev *hdev, u16 handle, struct sk_buff *skb)
 {
 	struct hci_conn *hcon;
-	struct sco_conn *conn;
+	struct sock *sk;
 
-	hci_dev_lock(hdev);
+	rcu_read_lock();
 
 	hcon = hci_conn_hash_lookup_handle(hdev, handle);
 	if (!hcon) {
-		hci_dev_unlock(hdev);
+		rcu_read_unlock();
 		kfree_skb(skb);
 		return -ENOENT;
 	}
 
-	conn = sco_conn_hold_unless_zero(hcon->sco_data);
+	sk = __sco_get_sk(hcon);
 	hcon = NULL;
 
-	hci_dev_unlock(hdev);
+	rcu_read_unlock();
 
-	if (!conn) {
+	if (!sk) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
 
-	BT_DBG("conn %p len %u", conn, skb->len);
+	BT_DBG("sk %p len %u", sk, skb->len);
 
 	if (skb->len)
-		sco_recv_frame(conn, skb);
+		sco_recv_frame(sk, skb);
 	else
 		kfree_skb(skb);
 
-	sco_conn_put(conn);
+	sock_put(sk);
 	return 0;
 }
 
-- 
2.53.0

RE: [RFC] Bluetooth: SCO: remove sco_conn and simplify locking scheme

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 4178 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1080418

---Test result---

Test Summary:
CheckPatch                    PENDING   0.55 seconds
GitLint                       PENDING   0.31 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      26.24 seconds
CheckAllWarning               PASS      28.84 seconds
CheckSparse                   PASS      27.61 seconds
BuildKernel32                 PASS      26.70 seconds
TestRunnerSetup               PASS      569.66 seconds
TestRunner_l2cap-tester       FAIL      28.28 seconds
TestRunner_iso-tester         PASS      37.77 seconds
TestRunner_bnep-tester        PASS      6.31 seconds
TestRunner_mgmt-tester        FAIL      115.37 seconds
TestRunner_rfcomm-tester      PASS      9.36 seconds
TestRunner_sco-tester         PASS      14.29 seconds
TestRunner_ioctl-tester       PASS      10.13 seconds
TestRunner_mesh-tester        FAIL      12.54 seconds
TestRunner_smp-tester         PASS      8.60 seconds
TestRunner_userchan-tester    PASS      6.64 seconds
TestRunner_6lowpan-tester     FAIL      8.98 seconds
IncrementalBuild              PENDING   0.27 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
Total: 96, Passed: 95 (99.0%), Failed: 1, Not Run: 0

Failed Test Cases
L2CAP BR/EDR Server - Set PHY 3M                     Failed       0.115 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.103 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.783 seconds
Mesh - Send cancel - 2                               Timed out    1.994 seconds
##############################
Test: TestRunner_6lowpan-tester - FAIL
Desc: Run 6lowpan-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
7.0.0-rc2-ga09fdde43cea #1 Not tainted
------------------------------------------------------
kworker/0:1/11 is trying to acquire lock:
ffff888002777140 ((wq_completion)hci0#2){+.+.}-{0:0}, at: touch_wq_lockdep_map+0x75/0x180

but task is already holding lock:
ffffffff8464d720 (rtnl_mutex){+.+.}-{4:4}, at: lowpan_unregister_netdev+0xd/0x30

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #4 (rtnl_mutex){+.+.}-{4:4}:
       lock_acquire+0xf7/0x2c0
       __mutex_lock+0x16b/0x1fc0
       lowpan_register_netdev+0x11/0x30
       chan_ready_cb+0x836/0xd00
       l2cap_recv_frame+0x6a06/0x8920
       l2cap_recv_acldata+0x790/0xdf0
       hci_rx_work+0x500/0xd00
       process_scheduled_works+0xb16/0x1ac0
       worker_thread+0x4ff/0xba0
       kthread+0x368/0x490
       ret_from_fork+0x498/0x7e0
       ret_from_fork_asm+0x19/0x30

-> #3 (&chan->lock#3/1){+.+.}-{4:4}:
       lock_acquire+0xf7/0x2c0
       __mutex_lock+0x16b/0x1fc0
       l2cap_chan_connect+0x74e/0x1980
       lowpan_control_write+0x523/0x660
       full_proxy_write+0x10b/0x190
       vfs_write+0x1c0/0xf60
       ksys_write+0xf1/0x1d0
       do_syscall_64+0xa0/0x570
       entry_SYSCALL_64_after_hwframe+0x74/0x7c

-> #2 (&conn->lock){+.+.}-{4:4}:
...
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



https://github.com/bluez/bluetooth-next/pull/73

---
Regards,
Linux Bluetooth