[PATCH v4 0/8] Bluetooth: Various L2CAP fixes

[PATCH v4 0/8] Bluetooth: Various L2CAP fixes

[johan.hedberg]

Hi,

Here's a fourth revision of this patch set with all feedback received so
far taken into account.

----------------------------------------------------------------
Johan Hedberg (8):
      Bluetooth: Remove unused event mask struct
      Bluetooth: Fix double error response for l2cap_create_chan_req
      Bluetooth: Fix L2CAP command reject reason
      Bluetooth: Fix L2CAP Disconnect response for unknown CID
      Bluetooth: Fix L2CAP error return used for failed channel lookups
      Bluetooth: Fix sending responses to identified L2CAP response packets
      Bluetooth: Fix responding to invalid L2CAP signaling commands
      Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag

 include/net/bluetooth/bluetooth.h |  1 +
 include/net/bluetooth/hci.h       |  3 ---
 net/bluetooth/af_bluetooth.c      | 39 +++++++++++++++++++++++++++++
 net/bluetooth/l2cap_core.c        | 50 ++++++++++++++++++++++++++-----------
 net/bluetooth/l2cap_sock.c        |  6 +++++
 net/bluetooth/rfcomm/sock.c       |  9 +++++--
 6 files changed, 88 insertions(+), 20 deletions(-)

[PATCH v4 1/8] Bluetooth: Remove unused event mask struct

[johan.hedberg]

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

The struct for HCI_Set_Event_Mask is never used. Instead a local 8-byte
array is used for sending this command. Therefore, remove the
unnecessary struct definition.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 57aa408..7ede266 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -695,9 +695,6 @@ struct hci_cp_sniff_subrate {
 } __packed;
 
 #define HCI_OP_SET_EVENT_MASK		0x0c01
-struct hci_cp_set_event_mask {
-	__u8     mask[8];
-} __packed;
 
 #define HCI_OP_RESET			0x0c03
 
-- 
1.8.4.rc3

[PATCH v4 2/8] Bluetooth: Fix double error response for l2cap_create_chan_req

[johan.hedberg]

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

When an L2CAP request handler returns non-zero the calling code will
send a command reject response. The l2cap_create_chan_req function will
in some cases send its own response but then still return a -EFAULT
error which would cause two responses to be sent. This patch fixes this
by making the function return 0 after sending its own response.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b3bb7bc..0b8a270 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4462,7 +4462,7 @@ error:
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
 		       sizeof(rsp), &rsp);
 
-	return -EFAULT;
+	return 0;
 }
 
 static void l2cap_send_move_chan_req(struct l2cap_chan *chan, u8 dest_amp_id)
-- 
1.8.4.rc3

[PATCH v4 3/8] Bluetooth: Fix L2CAP command reject reason

[johan.hedberg]

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

There are several possible reason codes that can be sent in the command
reject L2CAP packet. Before this patch the code has used a hard-coded
single response code ("command not understood"). This patch adds a
helper function to map the return value of an L2CAP handler function to
the correct command reject reason.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0b8a270..8c718dd 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5294,6 +5294,20 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
 	}
 }
 
+static __le16 l2cap_err_to_reason(int err)
+{
+	switch (err) {
+	case -ENOENT:
+		return __constant_cpu_to_le16(L2CAP_REJ_INVALID_CID);
+	case -EMSGSIZE:
+		return __constant_cpu_to_le16(L2CAP_REJ_MTU_EXCEEDED);
+	case -EINVAL:
+	case -EPROTO:
+	default:
+		return __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+	}
+}
+
 static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 					struct sk_buff *skb)
 {
@@ -5326,8 +5340,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 
 			BT_ERR("Wrong link type (%d)", err);
 
-			/* FIXME: Map err to a valid reason */
-			rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+			rej.reason = l2cap_err_to_reason(err);
 			l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
 				       sizeof(rej), &rej);
 		}
@@ -5371,8 +5384,7 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 
 			BT_ERR("Wrong link type (%d)", err);
 
-			/* FIXME: Map err to a valid reason */
-			rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+			rej.reason = l2cap_err_to_reason(err);
 			l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
 				       sizeof(rej), &rej);
 		}
-- 
1.8.4.rc3

[PATCH v4 4/8] Bluetooth: Fix L2CAP Disconnect response for unknown CID

[johan.hedberg]

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

If we receive an L2CAP Disconnect Request for an unknown CID we should
not just silently drop it but reply with a proper Command Reject
response. This patch fixes this by ensuring that the disconnect handler
returns a proper error instead of 0 and will cause the function caller
to send the right response.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 8c718dd..0b1a656 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
 	chan = __l2cap_get_chan_by_scid(conn, dcid);
 	if (!chan) {
 		mutex_unlock(&conn->chan_lock);
-		return 0;
+		return -ENOENT;
 	}
 
 	l2cap_chan_lock(chan);
-- 
1.8.4.rc3

[PATCH v4 5/8] Bluetooth: Fix L2CAP error return used for failed channel lookups

[johan.hedberg]

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

The EFAULT error should only be used for memory address related errors
whereas ENOENT should be used for failed lookups for the given CID. This
patch fixes the l2cap_connect_create_rsp and l2cap_create_channel_req
handlers to use the correct ENOENT error return.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0b1a656..3b0633f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3884,13 +3884,13 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
 	if (scid) {
 		chan = __l2cap_get_chan_by_scid(conn, scid);
 		if (!chan) {
-			err = -EFAULT;
+			err = -ENOENT;
 			goto unlock;
 		}
 	} else {
 		chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
 		if (!chan) {
-			err = -EFAULT;
+			err = -ENOENT;
 			goto unlock;
 		}
 	}
@@ -4438,7 +4438,7 @@ static int l2cap_create_channel_req(struct l2cap_conn *conn,
 		hs_hcon = hci_conn_hash_lookup_ba(hdev, AMP_LINK, conn->dst);
 		if (!hs_hcon) {
 			hci_dev_put(hdev);
-			return -EFAULT;
+			return -ENOENT;
 		}
 
 		BT_DBG("mgr %p bredr_chan %p hs_hcon %p", mgr, chan, hs_hcon);
-- 
1.8.4.rc3

[PATCH v4 6/8] Bluetooth: Fix sending responses to identified L2CAP response packets

[johan.hedberg]

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

When L2CAP packets return a non-zero error and the value is passed
onwards by l2cap_bredr_sig_cmd this will trigger a command reject packet
to be sent. However, the core specification (page 1416 in core 4.0) says
the following: "Command Reject packets should not be sent in response to
an identified Response packet.".

This patch ensures that a command reject packet is not sent for any
identified response packet by ignoring the error return value from the
response handler functions.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 3b0633f..020b723 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5212,7 +5212,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 
 	case L2CAP_CONN_RSP:
 	case L2CAP_CREATE_CHAN_RSP:
-		err = l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
+		l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
 		break;
 
 	case L2CAP_CONF_REQ:
@@ -5220,7 +5220,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 		break;
 
 	case L2CAP_CONF_RSP:
-		err = l2cap_config_rsp(conn, cmd, cmd_len, data);
+		l2cap_config_rsp(conn, cmd, cmd_len, data);
 		break;
 
 	case L2CAP_DISCONN_REQ:
@@ -5228,7 +5228,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 		break;
 
 	case L2CAP_DISCONN_RSP:
-		err = l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
+		l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
 		break;
 
 	case L2CAP_ECHO_REQ:
@@ -5243,7 +5243,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 		break;
 
 	case L2CAP_INFO_RSP:
-		err = l2cap_information_rsp(conn, cmd, cmd_len, data);
+		l2cap_information_rsp(conn, cmd, cmd_len, data);
 		break;
 
 	case L2CAP_CREATE_CHAN_REQ:
@@ -5255,7 +5255,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 		break;
 
 	case L2CAP_MOVE_CHAN_RSP:
-		err = l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
+		l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
 		break;
 
 	case L2CAP_MOVE_CHAN_CFM:
@@ -5263,7 +5263,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 		break;
 
 	case L2CAP_MOVE_CHAN_CFM_RSP:
-		err = l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
+		l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
 		break;
 
 	default:
-- 
1.8.4.rc3

[PATCH v4 7/8] Bluetooth: Fix responding to invalid L2CAP signaling commands

[johan.hedberg]

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

When we have an LE link we should not respond to any data on the BR/EDR
L2CAP signaling channel (0x0001) and vice-versa when we have a BR/EDR
link we should not respond to LE L2CAP (CID 0x0005) signaling commands.
This patch fixes this issue by checking for a valid link type and
ignores data if it is wrong.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 020b723..fe40d3e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5311,6 +5311,7 @@ static __le16 l2cap_err_to_reason(int err)
 static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 					struct sk_buff *skb)
 {
+	struct hci_conn *hcon = conn->hcon;
 	u8 *data = skb->data;
 	int len = skb->len;
 	struct l2cap_cmd_hdr cmd;
@@ -5318,6 +5319,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 
 	l2cap_raw_recv(conn, skb);
 
+	if (hcon->type != LE_LINK)
+		return;
+
 	while (len >= L2CAP_CMD_HDR_SIZE) {
 		u16 cmd_len;
 		memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
@@ -5355,6 +5359,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 				     struct sk_buff *skb)
 {
+	struct hci_conn *hcon = conn->hcon;
 	u8 *data = skb->data;
 	int len = skb->len;
 	struct l2cap_cmd_hdr cmd;
@@ -5362,6 +5367,9 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 
 	l2cap_raw_recv(conn, skb);
 
+	if (hcon->type != ACL_LINK)
+		return;
+
 	while (len >= L2CAP_CMD_HDR_SIZE) {
 		u16 cmd_len;
 		memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
-- 
1.8.4.rc3

[PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag

[johan.hedberg]

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

In the case of blocking sockets we should not proceed with sendmsg() if
the socket has the BT_SK_SUSPEND flag set. So far the code was only
ensuring that POLLOUT doesn't get set for non-blocking sockets using
poll() but there was no code in place to ensure that blocking sockets do
the right thing when writing to them.

This patch adds a new bt_sock_wait_ready helper function to sleep in the
sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon as it
is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks to take
advantage of this new helper function.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/af_bluetooth.c      | 39 +++++++++++++++++++++++++++++++++++++++
 net/bluetooth/l2cap_sock.c        |  6 ++++++
 net/bluetooth/rfcomm/sock.c       |  9 +++++++--
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 10d43d8..afbc711 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -249,6 +249,7 @@ int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
 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_unlink(struct sock *sk);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 9096137..a883171c 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
 }
 EXPORT_SYMBOL(bt_sock_wait_state);
 
+int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
+{
+	DECLARE_WAITQUEUE(wait, current);
+	unsigned long timeo;
+	int err = 0;
+
+	BT_DBG("sk %p", sk);
+
+	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
+
+	add_wait_queue(sk_sleep(sk), &wait);
+	set_current_state(TASK_INTERRUPTIBLE);
+	while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
+		if (!timeo) {
+			err = -EAGAIN;
+			break;
+		}
+
+		if (signal_pending(current)) {
+			err = sock_intr_errno(timeo);
+			break;
+		}
+
+		release_sock(sk);
+		timeo = schedule_timeout(timeo);
+		lock_sock(sk);
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		err = sock_error(sk);
+		if (err)
+			break;
+	}
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(sk_sleep(sk), &wait);
+
+	return err;
+}
+EXPORT_SYMBOL(bt_sock_wait_ready);
+
 #ifdef CONFIG_PROC_FS
 struct bt_seq_state {
 	struct bt_sock_list *l;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0098af8..ad95b42 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	if (sk->sk_state != BT_CONNECTED)
 		return -ENOTCONN;
 
+	lock_sock(sk);
+	err = bt_sock_wait_ready(sk, msg->msg_flags);
+	release_sock(sk);
+	if (err)
+		return err;
+
 	l2cap_chan_lock(chan);
 	err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
 	l2cap_chan_unlock(chan);
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 30b3721..02d96e4 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -544,7 +544,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	struct sock *sk = sock->sk;
 	struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
 	struct sk_buff *skb;
-	int sent = 0;
+	int err, sent = 0;
 
 	if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
 		return -ENOTCONN;
@@ -559,9 +559,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 
 	lock_sock(sk);
 
+	err = bt_sock_wait_ready(sk, msg->msg_flags);
+	if (err) {
+		release_sock(sk);
+		return err;
+	}
+
 	while (len) {
 		size_t size = min_t(size_t, len, d->mtu);
-		int err;
 
 		skb = sock_alloc_send_skb(sk, size + RFCOMM_SKB_RESERVE,
 				msg->msg_flags & MSG_DONTWAIT, &err);
-- 
1.8.4.rc3

Re: [PATCH v4 2/8] Bluetooth: Fix double error response for l2cap_create_chan_req

[Marcel Holtmann]

Hi Johan,

> When an L2CAP request handler returns non-zero the calling code will
> send a command reject response. The l2cap_create_chan_req function will
> in some cases send its own response but then still return a -EFAULT
> error which would cause two responses to be sent. This patch fixes this
> by making the function return 0 after sending its own response.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag

[Marcel Holtmann]

Hi Johan,

> In the case of blocking sockets we should not proceed with sendmsg() if
> the socket has the BT_SK_SUSPEND flag set. So far the code was only
> ensuring that POLLOUT doesn't get set for non-blocking sockets using
> poll() but there was no code in place to ensure that blocking sockets do
> the right thing when writing to them.
> 
> This patch adds a new bt_sock_wait_ready helper function to sleep in the
> sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon as it
> is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks to take
> advantage of this new helper function.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/bluetooth.h |  1 +
> net/bluetooth/af_bluetooth.c      | 39 +++++++++++++++++++++++++++++++++++++++
> net/bluetooth/l2cap_sock.c        |  6 ++++++
> net/bluetooth/rfcomm/sock.c       |  9 +++++++--
> 4 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 10d43d8..afbc711 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -249,6 +249,7 @@ int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
> 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_unlink(struct sock *sk);
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 9096137..a883171c 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
> }
> EXPORT_SYMBOL(bt_sock_wait_state);
> 

add a small comment here that this needs to be called with sk lock held.

> +int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +	unsigned long timeo;
> +	int err = 0;
> +
> +	BT_DBG("sk %p", sk);
> +
> +	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> +
> +	add_wait_queue(sk_sleep(sk), &wait);
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
> +		if (!timeo) {
> +			err = -EAGAIN;
> +			break;
> +		}
> +
> +		if (signal_pending(current)) {
> +			err = sock_intr_errno(timeo);
> +			break;
> +		}
> +
> +		release_sock(sk);
> +		timeo = schedule_timeout(timeo);
> +		lock_sock(sk);
> +		set_current_state(TASK_INTERRUPTIBLE);
> +
> +		err = sock_error(sk);
> +		if (err)
> +			break;
> +	}
> +	__set_current_state(TASK_RUNNING);
> +	remove_wait_queue(sk_sleep(sk), &wait);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(bt_sock_wait_ready);
> +
> #ifdef CONFIG_PROC_FS
> struct bt_seq_state {
> 	struct bt_sock_list *l;
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 0098af8..ad95b42 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> 	if (sk->sk_state != BT_CONNECTED)
> 		return -ENOTCONN;
> 
> +	lock_sock(sk);
> +	err = bt_sock_wait_ready(sk, msg->msg_flags);
> +	release_sock(sk);
> +	if (err)
> +		return err;
> +

After starting to look into this now, I am not sure we have proper locking in that function in the first place. Can you check that it is actually fine not holding the socket lock for the send operation itself.

> 	l2cap_chan_lock(chan);
> 	err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
> 	l2cap_chan_unlock(chan);
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 30b3721..02d96e4 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -544,7 +544,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> 	struct sock *sk = sock->sk;
> 	struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
> 	struct sk_buff *skb;
> -	int sent = 0;
> +	int err, sent = 0;
> 
> 	if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
> 		return -ENOTCONN;
> @@ -559,9 +559,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> 
> 	lock_sock(sk);
> 
> +	err = bt_sock_wait_ready(sk, msg->msg_flags);
> +	if (err) {
> +		release_sock(sk);
> +		return err;
> +	}
> +

I would be fine with using the sent variable here. And then of course create a label for the failure case. Assuming bt_sock_wait_ready() returning 0 on success you could use it to initialize sent here.

However at minimum, I want a label here to jump at the end of the function in error case. I like to keep the number of lock_sock + release_sock balanced.

Which is something that needs to be fixed for l2cap_sock_recvmsg() actually.

Regards

Marcel

Re: [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag

[Johan Hedberg]

Hi Marcel,

On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> > @@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
> > }
> > EXPORT_SYMBOL(bt_sock_wait_state);
> > 
> 
> add a small comment here that this needs to be called with sk lock held.

I suppose I should do that to the old bt_sock_wait_state function too?
(which btw my wait_ready function is essentially a copy of, with the
exception of which condition it looks for)

> > +int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
> > +{
> > +	DECLARE_WAITQUEUE(wait, current);
> > +	unsigned long timeo;
> > +	int err = 0;
> > +
> > +	BT_DBG("sk %p", sk);
> > +
> > +	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> > +
> > +	add_wait_queue(sk_sleep(sk), &wait);
> > +	set_current_state(TASK_INTERRUPTIBLE);
> > +	while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
> > +		if (!timeo) {
> > +			err = -EAGAIN;
> > +			break;
> > +		}
> > +
> > +		if (signal_pending(current)) {
> > +			err = sock_intr_errno(timeo);
> > +			break;
> > +		}
> > +
> > +		release_sock(sk);
> > +		timeo = schedule_timeout(timeo);
> > +		lock_sock(sk);
> > +		set_current_state(TASK_INTERRUPTIBLE);
> > +
> > +		err = sock_error(sk);
> > +		if (err)
> > +			break;
> > +	}
> > +	__set_current_state(TASK_RUNNING);
> > +	remove_wait_queue(sk_sleep(sk), &wait);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL(bt_sock_wait_ready);
> > +
> > #ifdef CONFIG_PROC_FS
> > struct bt_seq_state {
> > 	struct bt_sock_list *l;
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 0098af8..ad95b42 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> > 	if (sk->sk_state != BT_CONNECTED)
> > 		return -ENOTCONN;
> > 
> > +	lock_sock(sk);
> > +	err = bt_sock_wait_ready(sk, msg->msg_flags);
> > +	release_sock(sk);
> > +	if (err)
> > +		return err;
> > +
> 
> After starting to look into this now, I am not sure we have proper
> locking in that function in the first place. Can you check that it is
> actually fine not holding the socket lock for the send operation
> itself.

What procedure did you have in mind for checking what's fine and what's
not? Some quick googling and trying to look at the code of other address
families didn't help me getting much wiser about this.

This is the way that RFCOMM and L2CAP sockets seem to always have done
locking and we haven't seen any obvious breakage because of it. The
RFCOMM side does indeed use lock_sock for the send operation and L2CAP
doesn't. However, I think the reason it works is that l2cap_chan_lock
(which is used before calling l2cap_chan_send) serves a similar purpose.

> > --- a/net/bluetooth/rfcomm/sock.c
> > +++ b/net/bluetooth/rfcomm/sock.c
> > @@ -544,7 +544,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> > 	struct sock *sk = sock->sk;
> > 	struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
> > 	struct sk_buff *skb;
> > -	int sent = 0;
> > +	int err, sent = 0;
> > 
> > 	if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
> > 		return -ENOTCONN;
> > @@ -559,9 +559,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> > 
> > 	lock_sock(sk);
> > 
> > +	err = bt_sock_wait_ready(sk, msg->msg_flags);
> > +	if (err) {
> > +		release_sock(sk);
> > +		return err;
> > +	}
> > +
> 
> I would be fine with using the sent variable here. And then of course
> create a label for the failure case. Assuming bt_sock_wait_ready()
> returning 0 on success you could use it to initialize sent here.
> 
> However at minimum, I want a label here to jump at the end of the
> function in error case. I like to keep the number of lock_sock +
> release_sock balanced.

Makes sense. I'll do these improvements to the next version.

> Which is something that needs to be fixed for l2cap_sock_recvmsg()
> actually.

I'll see if I can roll up a separate patch for that.

Johan

Re: [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag

[Marcel Holtmann]

Hi Johan,

>>> @@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
>>> }
>>> EXPORT_SYMBOL(bt_sock_wait_state);
>>> 
>> 
>> add a small comment here that this needs to be called with sk lock held.
> 
> I suppose I should do that to the old bt_sock_wait_state function too?
> (which btw my wait_ready function is essentially a copy of, with the
> exception of which condition it looks for)

good idea.

>>> +int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
>>> +{
>>> +	DECLARE_WAITQUEUE(wait, current);
>>> +	unsigned long timeo;
>>> +	int err = 0;
>>> +
>>> +	BT_DBG("sk %p", sk);
>>> +
>>> +	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
>>> +
>>> +	add_wait_queue(sk_sleep(sk), &wait);
>>> +	set_current_state(TASK_INTERRUPTIBLE);
>>> +	while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
>>> +		if (!timeo) {
>>> +			err = -EAGAIN;
>>> +			break;
>>> +		}
>>> +
>>> +		if (signal_pending(current)) {
>>> +			err = sock_intr_errno(timeo);
>>> +			break;
>>> +		}
>>> +
>>> +		release_sock(sk);
>>> +		timeo = schedule_timeout(timeo);
>>> +		lock_sock(sk);
>>> +		set_current_state(TASK_INTERRUPTIBLE);
>>> +
>>> +		err = sock_error(sk);
>>> +		if (err)
>>> +			break;
>>> +	}
>>> +	__set_current_state(TASK_RUNNING);
>>> +	remove_wait_queue(sk_sleep(sk), &wait);
>>> +
>>> +	return err;
>>> +}
>>> +EXPORT_SYMBOL(bt_sock_wait_ready);
>>> +
>>> #ifdef CONFIG_PROC_FS
>>> struct bt_seq_state {
>>> 	struct bt_sock_list *l;
>>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>>> index 0098af8..ad95b42 100644
>>> --- a/net/bluetooth/l2cap_sock.c
>>> +++ b/net/bluetooth/l2cap_sock.c
>>> @@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
>>> 	if (sk->sk_state != BT_CONNECTED)
>>> 		return -ENOTCONN;
>>> 
>>> +	lock_sock(sk);
>>> +	err = bt_sock_wait_ready(sk, msg->msg_flags);
>>> +	release_sock(sk);
>>> +	if (err)
>>> +		return err;
>>> +
>> 
>> After starting to look into this now, I am not sure we have proper
>> locking in that function in the first place. Can you check that it is
>> actually fine not holding the socket lock for the send operation
>> itself.
> 
> What procedure did you have in mind for checking what's fine and what's
> not? Some quick googling and trying to look at the code of other address
> families didn't help me getting much wiser about this.
> 
> This is the way that RFCOMM and L2CAP sockets seem to always have done
> locking and we haven't seen any obvious breakage because of it. The
> RFCOMM side does indeed use lock_sock for the send operation and L2CAP
> doesn't. However, I think the reason it works is that l2cap_chan_lock
> (which is used before calling l2cap_chan_send) serves a similar purpose.

Then lets leave it as is.

Regards

Marcel