Bluetooth: L2CAP updates for PSM validation and ERTM

This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16 with L2CAP modes other than ERTM or streaming. At present, this only affects the FCS value shown with getsockopt() for basic mode.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

net/bluetooth/l2cap.c +0 -0

index 3e3cd9d..bc10be8 100644
@@ -3072,6 +3072,16 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	return 0;
 }

Hi Mat,

Quoted 16 lines

This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16 with L2CAP modes other than ERTM or streaming. At present, this only affects the FCS value shown with getsockopt() for basic mode.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

net/bluetooth/l2cap.c +0 -0

index 3e3cd9d..bc10be8 100644
@@ -3072,6 +3072,16 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	return 0;
 }

net/bluetooth/l2cap.c static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd · line 3083

Quoted 10 lines

net/bluetooth/l2cap.c +9 -0

 
+static inline int l2cap_fcs_needed(struct l2cap_pinfo *pi)
+{
+	if (pi->mode != L2CAP_MODE_ERTM && pi->mode != L2CAP_MODE_STREAMING)
+		return 0;
+
+	/* FCS is enabled if one or both sides request it. */
+	return !(pi->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+		pi->fcs == L2CAP_FCS_CRC16;
+}

so I think a switch statement is easier to read:

	switch (pi->mode) {
	case L2CAP_MODE_ERTM:
	case L2CAP_MODE_STREAMING:
		return !(pi->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
					pi->fcs == L2CAP_FCS_CRC16;
	}

	return 0;

net/bluetooth/l2cap.c static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr · line 3150

Quoted 11 lines

net/bluetooth/l2cap.c +1 -2

 static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, u16 cmd_len, u8 *data)
 {
 	struct l2cap_conf_req *req = (struct l2cap_conf_req *) data;

@@ -3136,8 +3146,7 @@ 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_fcs_needed(l2cap_pi(sk)))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;

While this is just fine, I think we might should make this a lot simpler:

	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
		set_default_fcs(pi);

		...
	}

With then set_default_fcs() doing all the checks etc. and setting either FCS_NONE or FCS_CRC16.

Would this work or do we have to deal with some funny default behavior?

Hi Marcel,

Quoted 61 lines · Marcel Holtmann

Hi Mat,

Quoted 16 lines

This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16 with L2CAP modes other than ERTM or streaming. At present, this only affects the FCS value shown with getsockopt() for basic mode.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

net/bluetooth/l2cap.c +0 -0

index 3e3cd9d..bc10be8 100644
@@ -3072,6 +3072,16 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	return 0;
 }

net/bluetooth/l2cap.c static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd · line 3083

Quoted 10 lines

net/bluetooth/l2cap.c +9 -0

 
+static inline int l2cap_fcs_needed(struct l2cap_pinfo *pi)
+{
+	if (pi->mode != L2CAP_MODE_ERTM && pi->mode != L2CAP_MODE_STREAMING)
+		return 0;
+
+	/* FCS is enabled if one or both sides request it. */
+	return !(pi->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+		pi->fcs == L2CAP_FCS_CRC16;
+}

so I think a switch statement is easier to read:

	switch (pi->mode) {
	case L2CAP_MODE_ERTM:
	case L2CAP_MODE_STREAMING:
		return !(pi->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
					pi->fcs == L2CAP_FCS_CRC16;
	}

	return 0;

net/bluetooth/l2cap.c static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr · line 3150

Quoted 11 lines

net/bluetooth/l2cap.c +1 -2

 static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, u16 cmd_len, u8 *data)
 {
 	struct l2cap_conf_req *req = (struct l2cap_conf_req *) data;

@@ -3136,8 +3146,7 @@ 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_fcs_needed(l2cap_pi(sk)))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;

While this is just fine, I think we might should make this a lot simpler:

	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
		set_default_fcs(pi);

		...
	}

Quoted 4 lines

With then set_default_fcs() doing all the checks etc. and setting either FCS_NONE or FCS_CRC16.

Would this work or do we have to deal with some funny default behavior?

That will work. ;)

The L2CAP spec requires that the ERTM retransmit timeout be at least 2 seconds for BR/EDR connections.

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Valid L2CAP PSMs are odd numbers, and the least significant bit of the most significant byte must be 0.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

net/bluetooth/l2cap.c +0 -0

index bc10be8..77ba106 100644
@@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 		goto done;
 	}

Incoming configuration values must be converted to native CPU order before use. This fixes a bug where a little-endian MPS value is compared to a native CPU value. On big-endian processors, this can cause ERTM and streaming mode segmentation to produce PDUs that are larger than the remote stack is expecting, or that would produce fragmented skbs that the current FCS code cannot handle.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

remote_tx_win is intended to be set on receipt of an L2CAP configuration request. The value is used to determine the size of the transmit window on the remote side of an ERTM connection, so L2CAP can stop sending frames when that remote window is full.

An incorrect remote_tx_win value will cause the stack to not fully utilize the tx window (performance impact), or to overfill the remote tx window (causing dropped frames or a disconnect).

This patch removes an extra setting of remote_tx_win when a configuration response is received. The transmit window has a different meaning in a response - it is an informational value less than or equal to the local tx_win.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

L2CAP ERTM sockets can be opened with the SOCK_STREAM socket type, which is a mandatory request for ERTM mode.

However, these sockets still have SOCK_SEQPACKET read semantics when bt_sock_recvmsg() is used to pull data from the receive queue. If the application is only reading part of a frame, then the unread portion of the frame is discarded. If the application requests more bytes than are in the current frame, only the current frame's data is returned.

This patch utilizes common code derived from RFCOMM's recvmsg() function to make L2CAP SOCK_STREAM reads behave like RFCOMM reads (and other SOCK_STREAM sockets in general). The application may read one byte at a time from the input stream and not lose any data, and may also read across L2CAP frame boundaries.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

net/bluetooth/l2cap.c +0 -0

index 43e0eae..cd0e150 100644
@@ -1956,7 +1956,10 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
 

The previous value of 672 for L2CAP_DEFAULT_MAX_PDU_SIZE is based on the default L2CAP MTU. That default MTU is calculated from the size of two DH5 packets, minus ACL and L2CAP b-frame header overhead.

ERTM is used with newer basebands that typically support larger 3-DH5 packets, and i-frames and s-frames have more header overhead. With clean RF conditions, basebands will typically attempt to use 1021-byte 3-DH5 packets for maximum throughput. Adjusting for 2 bytes of ACL headers plus 10 bytes of worst-case L2CAP headers yields 1009 bytes of payload.

This PDU size imposes less overhead for header bytes and gives the baseband the option to choose 3-DH5 packets, but is small enough for ERTM traffic to interleave well with other L2CAP or SCO data. 672-byte payloads do not allow the most efficient over-the-air packet choice, and cannot achieve maximum throughput over BR/EDR.

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)