[PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Mat Martineau]

This is a collection of ERTM-related patches, including some L2CAP 
configuration fixes and allowing partial-frame reads from L2CAP
SOCK_STREAM sockets.  PSM validation is also included, although that
is not restricted to ERTM.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9ba1e8e..aed72f2 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		goto unlock;
 
 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
+			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
+			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
 
 		sk->sk_state = BT_CONNECTED;
@@ -3217,8 +3219,10 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	l2cap_pi(sk)->conf_state |= L2CAP_CONF_INPUT_DONE;
 
 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_OUTPUT_DONE) {
-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
+			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
+			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
 
 		sk->sk_state = BT_CONNECTED;
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 2/8] Bluetooth: Change default ERTM retransmit timeout to 2 seconds, as the spec requires.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 636724b..16e412f 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -33,7 +33,7 @@
 #define L2CAP_DEFAULT_FLUSH_TO		0xffff
 #define L2CAP_DEFAULT_TX_WINDOW		63
 #define L2CAP_DEFAULT_MAX_TX		3
-#define L2CAP_DEFAULT_RETRANS_TO	1000    /* 1 second */
+#define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
 #define L2CAP_DEFAULT_MAX_PDU_SIZE	672
 #define L2CAP_DEFAULT_ACK_TO		200
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index aed72f2..44bc6ee 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 		goto done;
 	}
 
+	/* If specified, PSM must be odd and lsb of upper byte must be 0 */
+	if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	if (la.l2_psm && __le16_to_cpu(la.l2_psm) < 0x1001 &&
 				!capable(CAP_NET_BIND_SERVICE)) {
 		err = -EACCES;
@@ -1190,6 +1196,12 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
 		goto done;
 	}
 
+	/* PSM must be odd and lsb of upper byte must be 0 */
+	if ((__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	/* Set destination address and psm */
 	bacpy(&bt_sk(sk)->dst, &la.l2_bdaddr);
 	l2cap_pi(sk)->psm = la.l2_psm;
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 4/8] Bluetooth: Do endianness conversion on MPS configuration value before doing comparisons.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 44bc6ee..9780ab0 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2708,10 +2708,10 @@ done:
 		case L2CAP_MODE_ERTM:
 			pi->remote_tx_win = rfc.txwin_size;
 			pi->remote_max_tx = rfc.max_transmit;
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
 
 			rfc.retrans_timeout =
 				le16_to_cpu(L2CAP_DEFAULT_RETRANS_TO);
@@ -2726,10 +2726,9 @@ done:
 			break;
 
 		case L2CAP_MODE_STREAMING:
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
-
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
 
 			pi->conf_state |= L2CAP_CONF_MODE_DONE;
 
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 5/8] Bluetooth: Don't modify remote_tx_win when receiving a config response. Only config requests should set remote_tx_win.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9780ab0..582975b 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2808,7 +2808,6 @@ static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data,
 	if (*result == L2CAP_CONF_SUCCESS) {
 		switch (rfc.mode) {
 		case L2CAP_MODE_ERTM:
-			pi->remote_tx_win   = rfc.txwin_size;
 			pi->retrans_timeout = le16_to_cpu(rfc.retrans_timeout);
 			pi->monitor_timeout = le16_to_cpu(rfc.monitor_timeout);
 			pi->mps    = le16_to_cpu(rfc.max_pdu_size);
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/bluetooth.h |    2 +
 net/bluetooth/af_bluetooth.c      |  107 +++++++++++++++++++++++++++++++++++++
 net/bluetooth/rfcomm/sock.c       |  104 ++---------------------------------
 3 files changed, 115 insertions(+), 98 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 27a902d..08b6c2a 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -126,6 +126,8 @@ int  bt_sock_unregister(int proto);
 void bt_sock_link(struct bt_sock_list *l, struct sock *s);
 void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
 int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
+int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
+			struct msghdr *msg, size_t len, int flags);
 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);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..73047f5 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 }
 EXPORT_SYMBOL(bt_sock_recvmsg);
 
+static long bt_sock_data_wait(struct sock *sk, long timeo)
+{
+	DECLARE_WAITQUEUE(wait, current);
+
+	add_wait_queue(sk_sleep(sk), &wait);
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		if (!skb_queue_empty(&sk->sk_receive_queue) ||
+		    sk->sk_err ||
+		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
+		    signal_pending(current) ||
+		    !timeo)
+			break;
+
+		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
+		release_sock(sk);
+		timeo = schedule_timeout(timeo);
+		lock_sock(sk);
+		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
+	}
+
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(sk_sleep(sk), &wait);
+	return timeo;
+}
+
+int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
+			       struct msghdr *msg, size_t size, int flags)
+{
+	struct sock *sk = sock->sk;
+	int err = 0;
+	size_t target, copied = 0;
+	long timeo;
+
+	if (flags & MSG_OOB)
+		return -EOPNOTSUPP;
+
+	msg->msg_namelen = 0;
+
+	BT_DBG("sk %p size %zu", sk, size);
+
+	lock_sock(sk);
+
+	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
+	timeo  = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+
+	do {
+		struct sk_buff *skb;
+		int chunk;
+
+		skb = skb_dequeue(&sk->sk_receive_queue);
+		if (!skb) {
+			if (copied >= target)
+				break;
+
+			if ((err = sock_error(sk)) != 0)
+				break;
+			if (sk->sk_shutdown & RCV_SHUTDOWN)
+				break;
+
+			err = -EAGAIN;
+			if (!timeo)
+				break;
+
+			timeo = bt_sock_data_wait(sk, timeo);
+
+			if (signal_pending(current)) {
+				err = sock_intr_errno(timeo);
+				goto out;
+			}
+			continue;
+		}
+
+		chunk = min_t(unsigned int, skb->len, size);
+		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
+			skb_queue_head(&sk->sk_receive_queue, skb);
+			if (!copied)
+				copied = -EFAULT;
+			break;
+		}
+		copied += chunk;
+		size   -= chunk;
+
+		sock_recv_ts_and_drops(msg, sk, skb);
+
+		if (!(flags & MSG_PEEK)) {
+			skb_pull(skb, chunk);
+			if (skb->len) {
+				skb_queue_head(&sk->sk_receive_queue, skb);
+				break;
+			}
+			kfree_skb(skb);
+
+		} else {
+			/* put message back and return */
+			skb_queue_head(&sk->sk_receive_queue, skb);
+			break;
+		}
+	} while (size);
+
+out:
+	release_sock(sk);
+	return copied ? : err;
+}
+EXPORT_SYMBOL(bt_sock_stream_recvmsg);
+
 static inline unsigned int bt_accept_poll(struct sock *parent)
 {
 	struct list_head *p, *n;
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 44a6232..5c92929 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -617,121 +617,29 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	return sent;
 }
 
-static long rfcomm_sock_data_wait(struct sock *sk, long timeo)
-{
-	DECLARE_WAITQUEUE(wait, current);
-
-	add_wait_queue(sk_sleep(sk), &wait);
-	for (;;) {
-		set_current_state(TASK_INTERRUPTIBLE);
-
-		if (!skb_queue_empty(&sk->sk_receive_queue) ||
-		    sk->sk_err ||
-		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
-		    signal_pending(current) ||
-		    !timeo)
-			break;
-
-		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
-		release_sock(sk);
-		timeo = schedule_timeout(timeo);
-		lock_sock(sk);
-		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
-	}
-
-	__set_current_state(TASK_RUNNING);
-	remove_wait_queue(sk_sleep(sk), &wait);
-	return timeo;
-}
-
 static int rfcomm_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 			       struct msghdr *msg, size_t size, int flags)
 {
 	struct sock *sk = sock->sk;
 	struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
-	int err = 0;
-	size_t target, copied = 0;
-	long timeo;
+	int len_or_err;
 
 	if (test_and_clear_bit(RFCOMM_DEFER_SETUP, &d->flags)) {
 		rfcomm_dlc_accept(d);
 		return 0;
 	}
 
-	if (flags & MSG_OOB)
-		return -EOPNOTSUPP;
-
-	msg->msg_namelen = 0;
-
-	BT_DBG("sk %p size %zu", sk, size);
+	len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, size, flags);
 
 	lock_sock(sk);
+	if (!(flags & MSG_PEEK) && len_or_err > 0)
+		atomic_sub(len_or_err, &sk->sk_rmem_alloc);
 
-	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
-	timeo  = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
-
-	do {
-		struct sk_buff *skb;
-		int chunk;
-
-		skb = skb_dequeue(&sk->sk_receive_queue);
-		if (!skb) {
-			if (copied >= target)
-				break;
-
-			if ((err = sock_error(sk)) != 0)
-				break;
-			if (sk->sk_shutdown & RCV_SHUTDOWN)
-				break;
-
-			err = -EAGAIN;
-			if (!timeo)
-				break;
-
-			timeo = rfcomm_sock_data_wait(sk, timeo);
-
-			if (signal_pending(current)) {
-				err = sock_intr_errno(timeo);
-				goto out;
-			}
-			continue;
-		}
-
-		chunk = min_t(unsigned int, skb->len, size);
-		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
-			skb_queue_head(&sk->sk_receive_queue, skb);
-			if (!copied)
-				copied = -EFAULT;
-			break;
-		}
-		copied += chunk;
-		size   -= chunk;
-
-		sock_recv_ts_and_drops(msg, sk, skb);
-
-		if (!(flags & MSG_PEEK)) {
-			atomic_sub(chunk, &sk->sk_rmem_alloc);
-
-			skb_pull(skb, chunk);
-			if (skb->len) {
-				skb_queue_head(&sk->sk_receive_queue, skb);
-				break;
-			}
-			kfree_skb(skb);
-
-		} else {
-			/* put message back and return */
-			skb_queue_head(&sk->sk_receive_queue, skb);
-			break;
-		}
-	} while (size);
-
-out:
 	if (atomic_read(&sk->sk_rmem_alloc) <= (sk->sk_rcvbuf >> 2))
 		rfcomm_dlc_unthrottle(rfcomm_pi(sk)->dlc);
-
 	release_sock(sk);
-	return copied ? : err;
+
+	return len_or_err;
 }
 
 static int rfcomm_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 7/8] Bluetooth: Use a stream-oriented recvmsg with SOCK_STREAM L2CAP sockets.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 582975b..8e9fa51 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1923,6 +1923,7 @@ done:
 static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags)
 {
 	struct sock *sk = sock->sk;
+	int len_or_err;
 
 	lock_sock(sk);
 
@@ -1956,7 +1957,13 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
 
 	release_sock(sk);
 
-	return bt_sock_recvmsg(iocb, sock, msg, len, flags);
+	if (sock->type == SOCK_STREAM)
+		len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, len,
+						flags);
+	else
+		len_or_err = bt_sock_recvmsg(iocb, sock, msg, len, flags);
+
+	return len_or_err;
 }
 
 static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 8/8] Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 16e412f..93aba17 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -35,7 +35,7 @@
 #define L2CAP_DEFAULT_MAX_TX		3
 #define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
-#define L2CAP_DEFAULT_MAX_PDU_SIZE	672
+#define L2CAP_DEFAULT_MAX_PDU_SIZE	1011    /* Sized for 3-DH5 packet */
 #define L2CAP_DEFAULT_ACK_TO		200
 #define L2CAP_LOCAL_BUSY_TRIES		12
 
-- 
1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 9ba1e8e..aed72f2 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  		goto unlock;
>  
>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;

this becomes unreadable and my brain starts to throw a core dump. So it
clearly needs to be put into a helper inline function.

Regards

Marcel

Re: [PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index aed72f2..44bc6ee 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
>  		goto done;
>  	}
>  
> +	/* If specified, PSM must be odd and lsb of upper byte must be 0 */
> +	if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
> +		err = -EINVAL;
> +		goto done;
> +	}
> +

this was a nice feature of L2CAP in BlueZ actually ;)

I am fine with forbidding this. However, please send a patch that fixes
l2test's default PSM first. Otherwise you gonna break that one.

Regards

Marcel

Re: [PATCH 2/8] Bluetooth: Change default ERTM retransmit timeout to 2 seconds, as the spec requires.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

I do expect some more details commit messages. The subject line is
suppose to around 50 chars and give an idea what the patch is about. The
actual commit message should have the full blown explanation why etc.

Regards

Marcel

Re: [PATCH 4/8] Bluetooth: Do endianness conversion on MPS configuration value before doing comparisons.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)

same for subject line vs commit message. And please explain why and if
this fixes a bug.

Regards

Marcel

Re: [PATCH 5/8] Bluetooth: Don't modify remote_tx_win when receiving a config response. Only config requests should set remote_tx_win.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)

please write a proper commit message. Your subject is not meant to be
used for explaining what is wrong and what is right and how you did it.
It is called a SUBJECT.

Regards

Marcel

Re: [PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/bluetooth.h |    2 +
>  net/bluetooth/af_bluetooth.c      |  107 +++++++++++++++++++++++++++++++++++++
>  net/bluetooth/rfcomm/sock.c       |  104 ++---------------------------------
>  3 files changed, 115 insertions(+), 98 deletions(-)

looks all like a good idea, but I really have to insist that the commit
message explain everything in detail. Give a reason why the code is
similar or the same and why this makes sense.

Also splitting these into two or more patches makes sense. One adding
the stream receive and the other modifying L2CAP and RFCOMM to use it.

> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 27a902d..08b6c2a 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -126,6 +126,8 @@ int  bt_sock_unregister(int proto);
>  void bt_sock_link(struct bt_sock_list *l, struct sock *s);
>  void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
>  int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
> +int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> +			struct msghdr *msg, size_t len, int flags);
>  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);
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 421c45b..73047f5 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
>  }
>  EXPORT_SYMBOL(bt_sock_recvmsg);
>  
> +static long bt_sock_data_wait(struct sock *sk, long timeo)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +
> +	add_wait_queue(sk_sleep(sk), &wait);
> +	for (;;) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +
> +		if (!skb_queue_empty(&sk->sk_receive_queue) ||
> +		    sk->sk_err ||
> +		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
> +		    signal_pending(current) ||
> +		    !timeo)
> +			break;

This makes my brain hurt. Please lets do this readable and with proper
coding style and/or a helper function.

Regards

Marcel

Re: [PATCH 8/8] Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

this should have a commit message with technical details to explain why
it is a good idea.

> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 16e412f..93aba17 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -35,7 +35,7 @@
>  #define L2CAP_DEFAULT_MAX_TX		3
>  #define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
>  #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
> -#define L2CAP_DEFAULT_MAX_PDU_SIZE	672
> +#define L2CAP_DEFAULT_MAX_PDU_SIZE	1011    /* Sized for 3-DH5 packet */
>  #define L2CAP_DEFAULT_ACK_TO		200
>  #define L2CAP_LOCAL_BUSY_TRIES		12

Also the default PDU size for basic mode should stay with 672. And only
for ERTM move it to 1010. Is this ensured?

And then again here. Is that explained in the commit message?

Regards

Marcel

 

Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Marcel Holtmann]

Hi Mat,

> This is a collection of ERTM-related patches, including some L2CAP 
> configuration fixes and allowing partial-frame reads from L2CAP
> SOCK_STREAM sockets.  PSM validation is also included, although that
> is not restricted to ERTM.

so the merge window for 2.6.36 is basically closed and these patches
came in too late. If they fix a bug, I like you to explain that in
detail in the commit message so I can pick them out and include them for
submission to John.

Regards

Marcel

Re: [PATCH 7/8] Bluetooth: Use a stream-oriented recvmsg with SOCK_STREAM L2CAP sockets.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 582975b..8e9fa51 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1923,6 +1923,7 @@ done:
>  static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags)
>  {
>  	struct sock *sk = sock->sk;
> +	int len_or_err;
>  
>  	lock_sock(sk);
>  
> @@ -1956,7 +1957,13 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
>  
>  	release_sock(sk);
>  
> -	return bt_sock_recvmsg(iocb, sock, msg, len, flags);
> +	if (sock->type == SOCK_STREAM)
> +		len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, len,
> +						flags);
> +	else
> +		len_or_err = bt_sock_recvmsg(iocb, sock, msg, len, flags);
> +
> +	return len_or_err;
>  }

I don't like this variable name. Just call it "len" or just use "ret"
here. Check what other parts of net/bluetooth/ are using.

Also since you are not checking is value anyway, why not return right
away from the if.

	if (sock->type == SOCK_STREAM)
		return bt_sock_stream_recvmsg(...);

	return bt_sock_recvmsg(...);

That way the compiler should also not complain. And in addition your
patch looks dead simple ;)

Regards

Marcel

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Gustavo F. Padovan]

Hi Mat,

* Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:38:32 -0700]:

> Hi Mat,
> 
> > Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> > ---
> >  net/bluetooth/l2cap.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index 9ba1e8e..aed72f2 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >  		goto unlock;
> >  
> >  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> > -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> > -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> > +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> > +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> > +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> > +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> 
> this becomes unreadable and my brain starts to throw a core dump. So it
> clearly needs to be put into a helper inline function.

Actually we don't need that, since the code that deals with Basic Mode
never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
value in the Basic Mode.


-- 
Gustavo F. Padovan
http://padovan.org

Re: [PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().

[Gustavo F. Padovan]

Hi Mat,

* Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:39:51 -0700]:

> Hi Mat,
> 
> > Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> > ---
> >  net/bluetooth/l2cap.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index aed72f2..44bc6ee 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> >  		goto done;
> >  	}
> >  
> > +	/* If specified, PSM must be odd and lsb of upper byte must be 0 */
> > +	if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
> > +		err = -EINVAL;
> > +		goto done;
> > +	}
> > +
> 
> this was a nice feature of L2CAP in BlueZ actually ;)

And please add a commit message for this patch.


-- 
Gustavo F. Padovan
http://padovan.org

Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Mat Martineau]

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,
>
>> This is a collection of ERTM-related patches, including some L2CAP
>> configuration fixes and allowing partial-frame reads from L2CAP
>> SOCK_STREAM sockets.  PSM validation is also included, although that
>> is not restricted to ERTM.
>
> so the merge window for 2.6.36 is basically closed and these patches
> came in too late. If they fix a bug, I like you to explain that in
> detail in the commit message so I can pick them out and include them for
> submission to John.

Marcel -

I'll fix up my commit messages and make the other changes you 
recommended.  I don't have a specific need for these to be in 2.6.36, 
but with more detail on the bug fixes you can decide if they're urgent 
enough.

Thanks for the feedback.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Marcel Holtmann]

Hi Mat,

> >> This is a collection of ERTM-related patches, including some L2CAP
> >> configuration fixes and allowing partial-frame reads from L2CAP
> >> SOCK_STREAM sockets.  PSM validation is also included, although that
> >> is not restricted to ERTM.
> >
> > so the merge window for 2.6.36 is basically closed and these patches
> > came in too late. If they fix a bug, I like you to explain that in
> > detail in the commit message so I can pick them out and include them for
> > submission to John.
> 
> I'll fix up my commit messages and make the other changes you 
> recommended.  I don't have a specific need for these to be in 2.6.36, 
> but with more detail on the bug fixes you can decide if they're urgent 
> enough.

since 2.6.36 will be the first kernel we enable ERTM by default, I
prefer that we have known bugs/regressions actually fixed.

Regards

Marcel

Re: [PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.

[Mat Martineau]

Marcel,

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,
>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/bluetooth.h |    2 +
>>  net/bluetooth/af_bluetooth.c      |  107 +++++++++++++++++++++++++++++++++++++
>>  net/bluetooth/rfcomm/sock.c       |  104 ++---------------------------------
>>  3 files changed, 115 insertions(+), 98 deletions(-)
>
> looks all like a good idea, but I really have to insist that the commit
> message explain everything in detail. Give a reason why the code is
> similar or the same and why this makes sense.
>
> Also splitting these into two or more patches makes sense. One adding
> the stream receive and the other modifying L2CAP and RFCOMM to use it.

Agreed.


>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 27a902d..08b6c2a 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -126,6 +126,8 @@ int  bt_sock_unregister(int proto);
>>  void bt_sock_link(struct bt_sock_list *l, struct sock *s);
>>  void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
>>  int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
>> +int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>> +			struct msghdr *msg, size_t len, int flags);
>>  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);
>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>> index 421c45b..73047f5 100644
>> --- a/net/bluetooth/af_bluetooth.c
>> +++ b/net/bluetooth/af_bluetooth.c
>> @@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
>>  }
>>  EXPORT_SYMBOL(bt_sock_recvmsg);
>>
>> +static long bt_sock_data_wait(struct sock *sk, long timeo)
>> +{
>> +	DECLARE_WAITQUEUE(wait, current);
>> +
>> +	add_wait_queue(sk_sleep(sk), &wait);
>> +	for (;;) {
>> +		set_current_state(TASK_INTERRUPTIBLE);
>> +
>> +		if (!skb_queue_empty(&sk->sk_receive_queue) ||
>> +		    sk->sk_err ||
>> +		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
>> +		    signal_pending(current) ||
>> +		    !timeo)
>> +			break;
>
> This makes my brain hurt. Please lets do this readable and with proper
> coding style and/or a helper function.

That's unchanged code from rfcomm_sock_data_wait() in rfcomm/sock.c - 
but I'll go ahead and break it up using multiple 'if' statements.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 8/8] Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.

[Mat Martineau]

Marcel,

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,
>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> this should have a commit message with technical details to explain why
> it is a good idea.
>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 16e412f..93aba17 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -35,7 +35,7 @@
>>  #define L2CAP_DEFAULT_MAX_TX		3
>>  #define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
>>  #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
>> -#define L2CAP_DEFAULT_MAX_PDU_SIZE	672
>> +#define L2CAP_DEFAULT_MAX_PDU_SIZE	1011    /* Sized for 3-DH5 packet */
>>  #define L2CAP_DEFAULT_ACK_TO		200
>>  #define L2CAP_LOCAL_BUSY_TRIES		12
>
> Also the default PDU size for basic mode should stay with 672. And only
> for ERTM move it to 1010. Is this ensured?
>
> And then again here. Is that explained in the commit message?

It is ensured that the max PDU size is only used for ERTM and 
streaming mode.  The default MTU for basic mode and ERTM/streaming 
remains unchanged.

I will update the commit message to fully describe this.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum