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

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

[johan.hedberg]

Hi,

This set started off with just three patches but after looking into the
code based on feedback from Marcel there were several other issues that
were found which prompted for more patches.

Johan

----------------------------------------------------------------
Johan Hedberg (8):
      Bluetooth: Remove unused event mask struct
      Bluetooth: Fix L2CAP command reject reason
      Bluetooth: Fix double error response for l2cap_create_chan_req
      Bluetooth: Fix L2CAP Disconnect response for unknown CID
      Bluetooth: Fix error code for invalid CID in l2cap_config_req
      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        | 48 +++++++++++++++++++++++++++----------
 net/bluetooth/l2cap_sock.c        |  6 +++++
 net/bluetooth/rfcomm/sock.c       |  9 +++++--
 6 files changed, 88 insertions(+), 18 deletions(-)

[PATCH v3 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>
---
 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 v3 2/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 | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b3bb7bc..83acb28 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 u16 l2cap_err_to_reason(int err)
+{
+	switch (err) {
+	case -EFAULT:
+		return L2CAP_REJ_INVALID_CID;
+	case -EMSGSIZE:
+		return L2CAP_REJ_MTU_EXCEEDED;
+	case -EINVAL:
+	case -EPROTO:
+	default:
+		return L2CAP_REJ_NOT_UNDERSTOOD;
+	}
+}
+
 static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 					struct sk_buff *skb)
 {
@@ -5323,11 +5337,11 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 		err = l2cap_le_sig_cmd(conn, &cmd, data);
 		if (err) {
 			struct l2cap_cmd_rej_unk rej;
+			u16 reason = l2cap_err_to_reason(err);
 
 			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 = cpu_to_le16(reason);
 			l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
 				       sizeof(rej), &rej);
 		}
@@ -5368,11 +5382,11 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 		err = l2cap_bredr_sig_cmd(conn, &cmd, cmd_len, data);
 		if (err) {
 			struct l2cap_cmd_rej_unk rej;
+			u16 reason = l2cap_err_to_reason(err);
 
 			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 = cpu_to_le16(reason);
 			l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
 				       sizeof(rej), &rej);
 		}
-- 
1.8.4.rc3

[PATCH v3 3/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 83acb28..6515804 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 v3 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 6515804..9ec1561f 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 -EFAULT;
 	}
 
 	l2cap_chan_lock(chan);
-- 
1.8.4.rc3

[PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[johan.hedberg]

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

The convention of the L2CAP code is to return EFAULT when a CID in the
request packet is invalid. This patch fixes the l2cap_config_req to use
that error code instead of ENOENT.

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 9ec1561f..d134501 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
 
 	chan = l2cap_get_chan_by_scid(conn, dcid);
 	if (!chan)
-		return -ENOENT;
+		return -EFAULT;
 
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
 		struct l2cap_cmd_rej_cid rej;
-- 
1.8.4.rc3

[PATCH v3 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>
---
 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 d134501..7d36051 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 v3 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 7d36051..247a955 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5311,6 +5311,7 @@ static u16 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);
@@ -5356,6 +5360,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;
@@ -5363,6 +5368,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 v3 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>
---
v3: Use wait_ready instead of wait_unsuspend

 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 v3 1/8] Bluetooth: Remove unused event mask struct

[Marcel Holtmann]

Hi Johan,

> 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>
> ---
> include/net/bluetooth/hci.h | 3 ---
> 1 file changed, 3 deletions(-)

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

Regards

Marcel

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Marcel Holtmann]

Hi Johan,

> The convention of the L2CAP code is to return EFAULT when a CID in the
> request packet is invalid. This patch fixes the l2cap_config_req to use
> that error code instead of ENOENT.
> 
> 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 9ec1561f..d134501 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> 
> 	chan = l2cap_get_chan_by_scid(conn, dcid);
> 	if (!chan)
> -		return -ENOENT;
> +		return -EFAULT;

do we really want to use EFAULT. That is normally only used if a memory copy from userspace to kernel or vice versa fails.

Regards

Marcel

Re: [PATCH v3 2/8] Bluetooth: Fix L2CAP command reject reason

[Marcel Holtmann]

Hi Johan,

> 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 | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index b3bb7bc..83acb28 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 u16 l2cap_err_to_reason(int err)

what we could to is return __le16 here. And then keep using __constant_cpu_to_le16.


> +{
> +	switch (err) {
> +	case -EFAULT:
> +		return L2CAP_REJ_INVALID_CID;
> +	case -EMSGSIZE:
> +		return L2CAP_REJ_MTU_EXCEEDED;
> +	case -EINVAL:
> +	case -EPROTO:
> +	default:
> +		return L2CAP_REJ_NOT_UNDERSTOOD;
> +	}
> +}

Is there really a point in listing EINVAL and EPROTO here.

> +
> static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
> 					struct sk_buff *skb)
> {
> @@ -5323,11 +5337,11 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
> 		err = l2cap_le_sig_cmd(conn, &cmd, data);
> 		if (err) {
> 			struct l2cap_cmd_rej_unk rej;
> +			u16 reason = l2cap_err_to_reason(err);
> 
> 			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 = cpu_to_le16(reason);
> 			l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
> 				       sizeof(rej), &rej);
> 		}
> @@ -5368,11 +5382,11 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
> 		err = l2cap_bredr_sig_cmd(conn, &cmd, cmd_len, data);
> 		if (err) {
> 			struct l2cap_cmd_rej_unk rej;
> +			u16 reason = l2cap_err_to_reason(err);
> 
> 			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 = cpu_to_le16(reason);
> 			l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
> 				       sizeof(rej), &rej);
> 		}

Regards

Marcel

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

[Marcel Holtmann]

Hi Johan,

> 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>
> ---
> net/bluetooth/l2cap_core.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)

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

Regards

Marcel

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Johan Hedberg]

Hi Marcel,

On Sat, Sep 14, 2013, Marcel Holtmann wrote:
> > The convention of the L2CAP code is to return EFAULT when a CID in the
> > request packet is invalid. This patch fixes the l2cap_config_req to use
> > that error code instead of ENOENT.
> > 
> > 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 9ec1561f..d134501 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> > 
> > 	chan = l2cap_get_chan_by_scid(conn, dcid);
> > 	if (!chan)
> > -		return -ENOENT;
> > +		return -EFAULT;
> 
> do we really want to use EFAULT. That is normally only used if a
> memory copy from userspace to kernel or vice versa fails.

EFAULT seemed to be the most common error code in this case, but now
that I did some research it seems it's mostly from newish AMP related
code. Would you be ok if I just switch all places to use ENOENT then?

Johan

Re: [PATCH v3 2/8] Bluetooth: Fix L2CAP command reject reason

[Johan Hedberg]

Hi Marcel,

On Sat, Sep 14, 2013, Marcel Holtmann wrote:
> > 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 | 22 ++++++++++++++++++----
> > 1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index b3bb7bc..83acb28 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 u16 l2cap_err_to_reason(int err)
> 
> what we could to is return __le16 here. And then keep using
> __constant_cpu_to_le16.

I was mainly trying to follow the principle of using host endianness
until the very moment when we actually encode into the protocol struct.
However, I agree that we could be more pragmatic here since the only
users of this function are places where we're directly encoding into the
response PDU.

> > +{
> > +	switch (err) {
> > +	case -EFAULT:
> > +		return L2CAP_REJ_INVALID_CID;
> > +	case -EMSGSIZE:
> > +		return L2CAP_REJ_MTU_EXCEEDED;
> > +	case -EINVAL:
> > +	case -EPROTO:
> > +	default:
> > +		return L2CAP_REJ_NOT_UNDERSTOOD;
> > +	}
> > +}
> 
> Is there really a point in listing EINVAL and EPROTO here.

I thought it would make it clear which error codes are recommended for
parsing errors but I can remove them and just rely on default.

Johan

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Marcel Holtmann]

Hi Johan,

>>> The convention of the L2CAP code is to return EFAULT when a CID in the
>>> request packet is invalid. This patch fixes the l2cap_config_req to use
>>> that error code instead of ENOENT.
>>> 
>>> 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 9ec1561f..d134501 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>>> 
>>> 	chan = l2cap_get_chan_by_scid(conn, dcid);
>>> 	if (!chan)
>>> -		return -ENOENT;
>>> +		return -EFAULT;
>> 
>> do we really want to use EFAULT. That is normally only used if a
>> memory copy from userspace to kernel or vice versa fails.
> 
> EFAULT seemed to be the most common error code in this case, but now
> that I did some research it seems it's mostly from newish AMP related
> code. Would you be ok if I just switch all places to use ENOENT then?

EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.

I am fine with keep using ENOENT if we used that before. If that is what the original code used, then lets keep using it. Otherwise we better find a more descriptive error.

Regards

Marcel

Re: [PATCH v3 2/8] Bluetooth: Fix L2CAP command reject reason

[Marcel Holtmann]

Hi Johan,

>>> 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 | 22 ++++++++++++++++++----
>>> 1 file changed, 18 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index b3bb7bc..83acb28 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 u16 l2cap_err_to_reason(int err)
>> 
>> what we could to is return __le16 here. And then keep using
>> __constant_cpu_to_le16.
> 
> I was mainly trying to follow the principle of using host endianness
> until the very moment when we actually encode into the protocol struct.
> However, I agree that we could be more pragmatic here since the only
> users of this function are places where we're directly encoding into the
> response PDU.

the thing is that within the kernel you can nicely mark the endian of the return value with __le16. So tools like sparse will warn you if you mess it up.

>>> +{
>>> +	switch (err) {
>>> +	case -EFAULT:
>>> +		return L2CAP_REJ_INVALID_CID;
>>> +	case -EMSGSIZE:
>>> +		return L2CAP_REJ_MTU_EXCEEDED;
>>> +	case -EINVAL:
>>> +	case -EPROTO:
>>> +	default:
>>> +		return L2CAP_REJ_NOT_UNDERSTOOD;
>>> +	}
>>> +}
>> 
>> Is there really a point in listing EINVAL and EPROTO here.
> 
> I thought it would make it clear which error codes are recommended for
> parsing errors but I can remove them and just rely on default.

I am fine keeping them. Just asking.

Regards

Marcel

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Johan Hedberg]

Hi Marcel,

On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> >>> The convention of the L2CAP code is to return EFAULT when a CID in the
> >>> request packet is invalid. This patch fixes the l2cap_config_req to use
> >>> that error code instead of ENOENT.
> >>> 
> >>> 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 9ec1561f..d134501 100644
> >>> --- a/net/bluetooth/l2cap_core.c
> >>> +++ b/net/bluetooth/l2cap_core.c
> >>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> >>> 
> >>> 	chan = l2cap_get_chan_by_scid(conn, dcid);
> >>> 	if (!chan)
> >>> -		return -ENOENT;
> >>> +		return -EFAULT;
> >> 
> >> do we really want to use EFAULT. That is normally only used if a
> >> memory copy from userspace to kernel or vice versa fails.
> > 
> > EFAULT seemed to be the most common error code in this case, but now
> > that I did some research it seems it's mostly from newish AMP related
> > code. Would you be ok if I just switch all places to use ENOENT then?
> 
> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.
> 
> I am fine with keep using ENOENT if we used that before. If that is
> what the original code used, then lets keep using it. Otherwise we
> better find a more descriptive error.

If we ignore new AMP code there's only one "legacy" reference for this
kind of situation which is l2cap_config_req. Since it has always used
ENOENT for a failed CID lookup I'll follow that lead and use the same
error for all places that need it.

Johan

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Marcel Holtmann]

Hi Johan,

>>>>> The convention of the L2CAP code is to return EFAULT when a CID in the
>>>>> request packet is invalid. This patch fixes the l2cap_config_req to use
>>>>> that error code instead of ENOENT.
>>>>> 
>>>>> 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 9ec1561f..d134501 100644
>>>>> --- a/net/bluetooth/l2cap_core.c
>>>>> +++ b/net/bluetooth/l2cap_core.c
>>>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>>>>> 
>>>>> 	chan = l2cap_get_chan_by_scid(conn, dcid);
>>>>> 	if (!chan)
>>>>> -		return -ENOENT;
>>>>> +		return -EFAULT;
>>>> 
>>>> do we really want to use EFAULT. That is normally only used if a
>>>> memory copy from userspace to kernel or vice versa fails.
>>> 
>>> EFAULT seemed to be the most common error code in this case, but now
>>> that I did some research it seems it's mostly from newish AMP related
>>> code. Would you be ok if I just switch all places to use ENOENT then?
>> 
>> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.
>> 
>> I am fine with keep using ENOENT if we used that before. If that is
>> what the original code used, then lets keep using it. Otherwise we
>> better find a more descriptive error.
> 
> If we ignore new AMP code there's only one "legacy" reference for this
> kind of situation which is l2cap_config_req. Since it has always used
> ENOENT for a failed CID lookup I'll follow that lead and use the same
> error for all places that need it.

have a look at EBADRQC, EBADSLT, EBADR or EBADE. They might be a better choice.

Regards

Marcel

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Johan Hedberg]

Hi Marcel,

On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> >>>>> The convention of the L2CAP code is to return EFAULT when a CID in the
> >>>>> request packet is invalid. This patch fixes the l2cap_config_req to use
> >>>>> that error code instead of ENOENT.
> >>>>> 
> >>>>> 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 9ec1561f..d134501 100644
> >>>>> --- a/net/bluetooth/l2cap_core.c
> >>>>> +++ b/net/bluetooth/l2cap_core.c
> >>>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> >>>>> 
> >>>>> 	chan = l2cap_get_chan_by_scid(conn, dcid);
> >>>>> 	if (!chan)
> >>>>> -		return -ENOENT;
> >>>>> +		return -EFAULT;
> >>>> 
> >>>> do we really want to use EFAULT. That is normally only used if a
> >>>> memory copy from userspace to kernel or vice versa fails.
> >>> 
> >>> EFAULT seemed to be the most common error code in this case, but now
> >>> that I did some research it seems it's mostly from newish AMP related
> >>> code. Would you be ok if I just switch all places to use ENOENT then?
> >> 
> >> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.
> >> 
> >> I am fine with keep using ENOENT if we used that before. If that is
> >> what the original code used, then lets keep using it. Otherwise we
> >> better find a more descriptive error.
> > 
> > If we ignore new AMP code there's only one "legacy" reference for this
> > kind of situation which is l2cap_config_req. Since it has always used
> > ENOENT for a failed CID lookup I'll follow that lead and use the same
> > error for all places that need it.
> 
> have a look at EBADRQC, EBADSLT, EBADR or EBADE. They might be a
> better choice.

I'd rather have a consensus here first before resending the entire patch
set yet another time because of this issue (which, let's face it, is not
about any public API but consistency inside a single c-file).

As quite often with trying to map POSIX error codes to our own domain
we've ended up here with trying to find the "least bad" option in the
absence of a single obviously correct one.

EBADRQC sounds like it's not related to PDU parameters but the request
code of the PDU. EBADSLT is "invalid slot", so if we consider the CID to
be a "slot" this might be an option. EBADR talks about a "request
descriptor" which I don't quite understand how it's different from a
"request code". EBADE means "invalid exchange" whereas this isn't really
about an exchange but just an incorrect CID in a PDU.

I agree that ENOENT isn't perfect either since, even though internally
this is about a failed lookup, externally this is just the peer sending
us an invalid parameter value. Therefore, we could even consider EINVAL
if it wasn't for the fact that it's already used e.g. when we receive an
unknown opcode.

In the end I'm willing to accept pretty much any of the proposed options
since there really isn't a single right answer to this and since this is
not about public API that we really need to stick to once we decide
which value to use. The important thing is that the code is kept
consistent regarding the use of this error.

Johan

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Marcel Holtmann]

Hi Johan,

>>>>>>> The convention of the L2CAP code is to return EFAULT when a CID in the
>>>>>>> request packet is invalid. This patch fixes the l2cap_config_req to use
>>>>>>> that error code instead of ENOENT.
>>>>>>> 
>>>>>>> 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 9ec1561f..d134501 100644
>>>>>>> --- a/net/bluetooth/l2cap_core.c
>>>>>>> +++ b/net/bluetooth/l2cap_core.c
>>>>>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>>>>>>> 
>>>>>>> 	chan = l2cap_get_chan_by_scid(conn, dcid);
>>>>>>> 	if (!chan)
>>>>>>> -		return -ENOENT;
>>>>>>> +		return -EFAULT;
>>>>>> 
>>>>>> do we really want to use EFAULT. That is normally only used if a
>>>>>> memory copy from userspace to kernel or vice versa fails.
>>>>> 
>>>>> EFAULT seemed to be the most common error code in this case, but now
>>>>> that I did some research it seems it's mostly from newish AMP related
>>>>> code. Would you be ok if I just switch all places to use ENOENT then?
>>>> 
>>>> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.
>>>> 
>>>> I am fine with keep using ENOENT if we used that before. If that is
>>>> what the original code used, then lets keep using it. Otherwise we
>>>> better find a more descriptive error.
>>> 
>>> If we ignore new AMP code there's only one "legacy" reference for this
>>> kind of situation which is l2cap_config_req. Since it has always used
>>> ENOENT for a failed CID lookup I'll follow that lead and use the same
>>> error for all places that need it.
>> 
>> have a look at EBADRQC, EBADSLT, EBADR or EBADE. They might be a
>> better choice.
> 
> I'd rather have a consensus here first before resending the entire patch
> set yet another time because of this issue (which, let's face it, is not
> about any public API but consistency inside a single c-file).
> 
> As quite often with trying to map POSIX error codes to our own domain
> we've ended up here with trying to find the "least bad" option in the
> absence of a single obviously correct one.
> 
> EBADRQC sounds like it's not related to PDU parameters but the request
> code of the PDU. EBADSLT is "invalid slot", so if we consider the CID to
> be a "slot" this might be an option. EBADR talks about a "request
> descriptor" which I don't quite understand how it's different from a
> "request code". EBADE means "invalid exchange" whereas this isn't really
> about an exchange but just an incorrect CID in a PDU.
> 
> I agree that ENOENT isn't perfect either since, even though internally
> this is about a failed lookup, externally this is just the peer sending
> us an invalid parameter value. Therefore, we could even consider EINVAL
> if it wasn't for the fact that it's already used e.g. when we receive an
> unknown opcode.
> 
> In the end I'm willing to accept pretty much any of the proposed options
> since there really isn't a single right answer to this and since this is
> not about public API that we really need to stick to once we decide
> which value to use. The important thing is that the code is kept
> consistent regarding the use of this error.

I have nothing against ENOENT. I however am a bit concerned about that we accidentally use ENOENT in some other case and end up overloading its meaning.

So just for the sake of making this unique, I would consider going with EBADSLT. I am not married to this at all. However I wanted to have this talked through so that we understand the whole picture. If I would know the perfect answer, I would have just forced you to use it ;)

If you want to keep using ENOENT, then I will sign off your current patches.

Regards

Marcel

Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req

[Johan Hedberg]

Hi Marcel,

On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> I have nothing against ENOENT. I however am a bit concerned about that
> we accidentally use ENOENT in some other case and end up overloading
> its meaning.
> 
> So just for the sake of making this unique, I would consider going
> with EBADSLT. I am not married to this at all. However I wanted to
> have this talked through so that we understand the whole picture. If I
> would know the perfect answer, I would have just forced you to use it
> ;)

I really don't have a strong opinion one way or another, both seem good
enough to me, but since ENOENT has you feeling a bit uneasy I'll change
this to EBADSLT for the next revision (which will hopefully be the last
one for this error code issue).

Johan