[PATCH v7 0/7] can: support CAN XL

[PATCH v7 0/7] can: support CAN XL

[Oliver Hartkopp]

The CAN with eXtended data Length (CAN XL) is a new CAN protocol with a
10Mbit/s data transfer with a new physical layer transceiver (for this
data section). CAN XL allows up to 2048 byte of payload and shares the
arbitration principle (11 bit priority) known from Classical CAN and
CAN FD. RTR and 29 bit identifiers are not implemented in CAN XL.

A short introdution to CAN XL can be found here:
https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf

V2: Major rework after discussion and feedback on Linux-CAN ML

- rework of struct canxl_frame
- CANXL_XLF flag is now the switch between CAN XL and CAN/CANFD
- variable length in r/w operations for CAN XL frames
- write CAN XL frame to raw socket enforces size <-> canxl_frame.len sync

V3: Fix length for CAN XL frames inside the sk_buff

- extend the CAN_RAW sockopt to handle fixed/truncated read/write operations

V4: Fix patch 5 (can: raw: add CAN XL support)

- fix return value (move 'err = -EINVAL' in raw_sendmsg())
- add CAN XL frame handling in can_rcv()
- change comment for CAN_RAW_XL_[RT]X_DYN definition (allow -> enable)

V5: Remove CAN_RAW_XL_[RT]X_DYN definition again

- CAN_RAW_XL_[RT]X_DYN (truncated data) feature is now enabled by default
- use CANXL_MIN_DLEN instead of '1' in canxl_frame definition
- add missing 'err = -EINVAL' initialization in raw_sendmsg())

V6:

- rework an separate skb identification and length helpers
- add CANFD_FDF flag in all CAN FD frame structures
- simplify patches for infrastructure and raw sockets
- add vxcan support in virtual CAN interface patch

V7:

- fixed indention as remarked by Marc
- set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
- Allow to use variable CAN XL MTU sizes to enforce real time requirements
  on CAN XL segments (e.g. to support of CAN CiA segmentation concept)

Oliver Hartkopp (7):
  can: skb: unify skb CAN frame identification helpers
  can: skb: add skb CAN frame data length helpers
  can: set CANFD_FDF flag in all CAN FD frame structures
  can: canxl: introduce CAN XL data structure
  can: canxl: update CAN infrastructure for CAN XL frames
  can: dev: add CAN XL support to virtual CAN
  can: raw: add CAN XL support

 drivers/net/can/ctucanfd/ctucanfd_base.c |   1 -
 drivers/net/can/dev/rx-offload.c         |   2 +-
 drivers/net/can/dev/skb.c                | 113 ++++++++++++++++-------
 drivers/net/can/vcan.c                   |  12 +--
 drivers/net/can/vxcan.c                  |   8 +-
 include/linux/can/dev.h                  |   5 +
 include/linux/can/skb.h                  |  57 +++++++++++-
 include/uapi/linux/can.h                 |  55 ++++++++++-
 include/uapi/linux/can/raw.h             |   1 +
 include/uapi/linux/if_ether.h            |   1 +
 net/can/af_can.c                         |  76 ++++++++-------
 net/can/bcm.c                            |   9 +-
 net/can/gw.c                             |   4 +-
 net/can/isotp.c                          |   2 +-
 net/can/j1939/main.c                     |   4 +
 net/can/raw.c                            |  55 ++++++++---
 16 files changed, 299 insertions(+), 106 deletions(-)

-- 
2.30.2

[PATCH v7 1/7] can: skb: unify skb CAN frame identification helpers

[Oliver Hartkopp]

Replace open coded checks for sk_buffs containing Classical CAN and
CAN FD frame structures as a preparation for CAN XL support.

With the added length check the unintended processing of CAN XL frames
having the CANXL_XLF bit set can be suppressed even when the skb->len
fits to non CAN XL frames.

The CAN_RAW socket needs a rework to use these helpers. Therefore the
use of these helpers is postponed to the CAN_RAW CAN XL integration.

The J1939 protocol gets a check for Classical CAN frames too.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev/skb.c | 18 +++++++-------
 include/linux/can/skb.h   | 12 +++++++++-
 net/can/af_can.c          | 50 ++++++++-------------------------------
 net/can/bcm.c             |  9 +++++--
 net/can/gw.c              |  4 ++--
 net/can/isotp.c           |  2 +-
 net/can/j1939/main.c      |  4 ++++
 7 files changed, 45 insertions(+), 54 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 8bb62dd864c8..45313b90005c 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -291,22 +291,24 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
 }
 
 /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
 bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
 {
-	const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct can_priv *priv = netdev_priv(dev);
 
-	if (skb->protocol == htons(ETH_P_CAN)) {
-		if (unlikely(skb->len != CAN_MTU ||
-			     cfd->len > CAN_MAX_DLEN))
+	switch (ntohs(skb->protocol)) {
+	case ETH_P_CAN:
+		if (!can_is_can_skb(skb))
 			goto inval_skb;
-	} else if (skb->protocol == htons(ETH_P_CANFD)) {
-		if (unlikely(skb->len != CANFD_MTU ||
-			     cfd->len > CANFD_MAX_DLEN))
+		break;
+
+	case ETH_P_CANFD:
+		if (!can_is_canfd_skb(skb))
 			goto inval_skb;
-	} else {
+		break;
+
+	default:
 		goto inval_skb;
 	}
 
 	if (!can_skb_headroom_valid(dev, skb)) {
 		goto inval_skb;
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 182749e858b3..27ebfc28510f 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -95,12 +95,22 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
 	can_skb_set_owner(nskb, skb->sk);
 	consume_skb(skb);
 	return nskb;
 }
 
+static inline bool can_is_can_skb(const struct sk_buff *skb)
+{
+	struct can_frame *cf = (struct can_frame *)skb->data;
+
+	/* the CAN specific type of skb is identified by its data length */
+	return (skb->len == CAN_MTU && cf->len <= CAN_MAX_DLEN);
+}
+
 static inline bool can_is_canfd_skb(const struct sk_buff *skb)
 {
+	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
 	/* the CAN specific type of skb is identified by its data length */
-	return skb->len == CANFD_MTU;
+	return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
 }
 
 #endif /* !_CAN_SKB_H */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 1fb49d51b25d..afa6c2151bc4 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -197,31 +197,23 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
  *  -EINVAL when the skb->data does not contain a valid CAN frame
  */
 int can_send(struct sk_buff *skb, int loop)
 {
 	struct sk_buff *newskb = NULL;
-	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
 	int err = -EINVAL;
 
-	if (skb->len == CAN_MTU) {
+	if (can_is_can_skb(skb)) {
 		skb->protocol = htons(ETH_P_CAN);
-		if (unlikely(cfd->len > CAN_MAX_DLEN))
-			goto inval_skb;
-	} else if (skb->len == CANFD_MTU) {
+	} else if (can_is_canfd_skb(skb)) {
 		skb->protocol = htons(ETH_P_CANFD);
-		if (unlikely(cfd->len > CANFD_MAX_DLEN))
-			goto inval_skb;
 	} else {
 		goto inval_skb;
 	}
 
-	/* Make sure the CAN frame can pass the selected CAN netdevice.
-	 * As structs can_frame and canfd_frame are similar, we can provide
-	 * CAN FD frames to legacy CAN drivers as long as the length is <= 8
-	 */
-	if (unlikely(skb->len > skb->dev->mtu && cfd->len > CAN_MAX_DLEN)) {
+	/* Make sure the CAN frame can pass the selected CAN netdevice. */
+	if (unlikely(skb->len > skb->dev->mtu)) {
 		err = -EMSGSIZE;
 		goto inval_skb;
 	}
 
 	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
@@ -676,57 +668,35 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 }
 
 static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev)
 {
-	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
-
-	if (unlikely(dev->type != ARPHRD_CAN || skb->len != CAN_MTU)) {
+	if (unlikely(dev->type != ARPHRD_CAN || (!can_is_can_skb(skb)))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
-		goto free_skb;
-	}
 
-	/* This check is made separately since cfd->len would be uninitialized if skb->len = 0. */
-	if (unlikely(cfd->len > CAN_MAX_DLEN)) {
-		pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d, datalen %d\n",
-			     dev->type, skb->len, cfd->len);
-		goto free_skb;
+		kfree_skb(skb);
+		return NET_RX_DROP;
 	}
 
 	can_receive(skb, dev);
 	return NET_RX_SUCCESS;
-
-free_skb:
-	kfree_skb(skb);
-	return NET_RX_DROP;
 }
 
 static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
-	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
-
-	if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU)) {
+	if (unlikely(dev->type != ARPHRD_CAN || (!can_is_canfd_skb(skb)))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
-		goto free_skb;
-	}
 
-	/* This check is made separately since cfd->len would be uninitialized if skb->len = 0. */
-	if (unlikely(cfd->len > CANFD_MAX_DLEN)) {
-		pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d, datalen %d\n",
-			     dev->type, skb->len, cfd->len);
-		goto free_skb;
+		kfree_skb(skb);
+		return NET_RX_DROP;
 	}
 
 	can_receive(skb, dev);
 	return NET_RX_SUCCESS;
-
-free_skb:
-	kfree_skb(skb);
-	return NET_RX_DROP;
 }
 
 /* af_can protocol functions */
 
 /**
diff --git a/net/can/bcm.c b/net/can/bcm.c
index e60161bec850..e5d179ba6f7d 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -646,12 +646,17 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 
 	if (op->can_id != rxframe->can_id)
 		return;
 
 	/* make sure to handle the correct frame type (CAN / CAN FD) */
-	if (skb->len != op->cfsiz)
-		return;
+	if (op->flags & CAN_FD_FRAME) {
+		if (!can_is_canfd_skb(skb))
+			return;
+	} else {
+		if (!can_is_can_skb(skb))
+			return;
+	}
 
 	/* disable timeout */
 	hrtimer_cancel(&op->timer);
 
 	/* save rx timestamp */
diff --git a/net/can/gw.c b/net/can/gw.c
index 1ea4cc527db3..23a3d89cad81 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -461,14 +461,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 	struct sk_buff *nskb;
 	int modidx = 0;
 
 	/* process strictly Classic CAN or CAN FD frames */
 	if (gwj->flags & CGW_FLAGS_CAN_FD) {
-		if (skb->len != CANFD_MTU)
+		if (!can_is_canfd_skb(skb))
 			return;
 	} else {
-		if (skb->len != CAN_MTU)
+		if (!can_is_can_skb(skb))
 			return;
 	}
 
 	/* Do not handle CAN frames routed more than 'max_hops' times.
 	 * In general we should never catch this delimiter which is intended
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 43a27d19cdac..a9d1357f8489 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -667,11 +667,11 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
 		sf_dl = cf->data[ae] & 0x0F;
 
 		if (cf->len <= CAN_MAX_DLEN) {
 			isotp_rcv_sf(sk, cf, SF_PCI_SZ4 + ae, skb, sf_dl);
 		} else {
-			if (skb->len == CANFD_MTU) {
+			if (can_is_canfd_skb(skb)) {
 				/* We have a CAN FD frame and CAN_DL is greater than 8:
 				 * Only frames with the SF_DL == 0 ESC value are valid.
 				 *
 				 * If so take care of the increased SF PCI size
 				 * (SF_PCI_SZ8) to point to the message content behind
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 8452b0fbb78c..144c86b0e3ff 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -40,10 +40,14 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
 	struct j1939_priv *priv = data;
 	struct sk_buff *skb;
 	struct j1939_sk_buff_cb *skcb, *iskcb;
 	struct can_frame *cf;
 
+	/* make sure we only get Classical CAN frames */
+	if (!can_is_can_skb(iskb))
+		return;
+
 	/* create a copy of the skb
 	 * j1939 only delivers the real data bytes,
 	 * the header goes into sockaddr.
 	 * j1939 may not touch the incoming skb in such way
 	 */
-- 
2.30.2

[PATCH v7 2/7] can: skb: add skb CAN frame data length helpers

[Oliver Hartkopp]

Add two helpers to retrieve the data length from CAN sk_buffs and prepare
the length information to be a uint16 value for the CAN XL support.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev/rx-offload.c |  2 +-
 drivers/net/can/dev/skb.c        | 12 ++++--------
 include/linux/can/skb.h          | 24 +++++++++++++++++++++++-
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index a32a01c172d4..8505e547e922 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -245,11 +245,11 @@ unsigned int can_rx_offload_get_echo_skb(struct can_rx_offload *offload,
 					 unsigned int *frame_len_ptr)
 {
 	struct net_device *dev = offload->dev;
 	struct net_device_stats *stats = &dev->stats;
 	struct sk_buff *skb;
-	u8 len;
+	unsigned int len;
 	int err;
 
 	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
 	if (!skb)
 		return 0;
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 45313b90005c..7b369383416b 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -86,12 +86,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(can_put_echo_skb);
 
 struct sk_buff *
-__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
-		   unsigned int *frame_len_ptr)
+__can_get_echo_skb(struct net_device *dev, unsigned int idx,
+		   unsigned int *len_ptr, unsigned int *frame_len_ptr)
 {
 	struct can_priv *priv = netdev_priv(dev);
 
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
@@ -103,17 +103,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
 		/* Using "struct canfd_frame::len" for the frame
 		 * length is supported on both CAN and CANFD frames.
 		 */
 		struct sk_buff *skb = priv->echo_skb[idx];
 		struct can_skb_priv *can_skb_priv = can_skb_prv(skb);
-		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
 
 		/* get the real payload length for netdev statistics */
-		if (cf->can_id & CAN_RTR_FLAG)
-			*len_ptr = 0;
-		else
-			*len_ptr = cf->len;
+		*len_ptr = can_skb_get_data_len(skb);
 
 		if (frame_len_ptr)
 			*frame_len_ptr = can_skb_priv->frame_len;
 
 		priv->echo_skb[idx] = NULL;
@@ -139,11 +135,11 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
  */
 unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx,
 			      unsigned int *frame_len_ptr)
 {
 	struct sk_buff *skb;
-	u8 len;
+	unsigned int len;
 
 	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
 	if (!skb)
 		return 0;
 
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 27ebfc28510f..ddffc2fc008c 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -18,11 +18,12 @@
 
 void can_flush_echo_skb(struct net_device *dev);
 int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 		     unsigned int idx, unsigned int frame_len);
 struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx,
-				   u8 *len_ptr, unsigned int *frame_len_ptr);
+				   unsigned int *len_ptr,
+				   unsigned int *frame_len_ptr);
 unsigned int __must_check can_get_echo_skb(struct net_device *dev,
 					   unsigned int idx,
 					   unsigned int *frame_len_ptr);
 void can_free_echo_skb(struct net_device *dev, unsigned int idx,
 		       unsigned int *frame_len_ptr);
@@ -111,6 +112,27 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
 
 	/* the CAN specific type of skb is identified by its data length */
 	return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
 }
 
+/* get length element value from can[fd]_frame structure */
+static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
+{
+	const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+	return cfd->len;
+}
+
+/* get needed data length inside CAN frame for all frame types (RTR aware) */
+static inline unsigned int can_skb_get_data_len(struct sk_buff *skb)
+{
+	unsigned int len = can_skb_get_len_val(skb);
+	const struct can_frame *cf = (struct can_frame *)skb->data;
+
+	/* RTR frames have an actual length of zero */
+	if (can_is_can_skb(skb) && cf->can_id & CAN_RTR_FLAG)
+		return 0;
+
+	return len;
+}
+
 #endif /* !_CAN_SKB_H */
-- 
2.30.2

[PATCH v7 3/7] can: set CANFD_FDF flag in all CAN FD frame structures

[Oliver Hartkopp]

To simplify the testing in user space all struct canfd_frame's provided by
the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in
canfd_frame::flags.

NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might
not set this bit correctly. During the check for sufficient headroom in
PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled.
In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.

As the CAN frame content is already zero initialized in alloc_canfd_skb()
the obsolete initialization of cf->flags in the CTU CAN FD driver has been
removed as it would overwrite the already set CANFD_FDF flag.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/ctucanfd/ctucanfd_base.c |  1 -
 drivers/net/can/dev/skb.c                | 11 +++++++++++
 include/uapi/linux/can.h                 |  4 ++--
 net/can/af_can.c                         |  5 +++++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
index 6b281f6eb9b4..7eb9e4badedf 100644
--- a/drivers/net/can/ctucanfd/ctucanfd_base.c
+++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
@@ -654,11 +654,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
 		cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
 		cf->can_id = (idw >> 18) & CAN_SFF_MASK;
 
 	/* BRS, ESI, RTR Flags */
-	cf->flags = 0;
 	if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
 		if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
 			cf->flags |= CANFD_BRS;
 		if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
 			cf->flags |= CANFD_ESI;
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 7b369383416b..dd3854a56af5 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -236,10 +236,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 	can_skb_prv(skb)->ifindex = dev->ifindex;
 	can_skb_prv(skb)->skbcnt = 0;
 
 	*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
 
+	/* set CAN FD flag by default */
+	(*cfd)->flags = CANFD_FDF;
+
 	return skb;
 }
 EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
 struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
@@ -279,10 +282,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
 			skb->pkt_type = PACKET_HOST;
 
 		skb_reset_mac_header(skb);
 		skb_reset_network_header(skb);
 		skb_reset_transport_header(skb);
+
+		/* set CANFD_FDF flag for CAN FD frames */
+		if (can_is_canfd_skb(skb)) {
+			struct canfd_frame *cfd;
+
+			cfd = (struct canfd_frame *)skb->data;
+			cfd->flags |= CANFD_FDF;
+		}
 	}
 
 	return true;
 }
 
diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 90801ada2bbe..7b23eeeb3273 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -139,12 +139,12 @@ struct can_frame {
  * The struct can_frame and struct canfd_frame intentionally share the same
  * layout to be able to write CAN frame content into a CAN FD frame structure.
  * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
  * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
  * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
- * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
- * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
+ * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
+ * frame structures provided by the CAN subsystem of the Linux kernel.
  */
 #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
 #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
 #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
 
diff --git a/net/can/af_can.c b/net/can/af_can.c
index afa6c2151bc4..072a6a5c9dd1 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
 	int err = -EINVAL;
 
 	if (can_is_can_skb(skb)) {
 		skb->protocol = htons(ETH_P_CAN);
 	} else if (can_is_canfd_skb(skb)) {
+		struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
 		skb->protocol = htons(ETH_P_CANFD);
+
+		/* set CAN FD flag for CAN FD frames by default */
+		cfd->flags |= CANFD_FDF;
 	} else {
 		goto inval_skb;
 	}
 
 	/* Make sure the CAN frame can pass the selected CAN netdevice. */
-- 
2.30.2

[PATCH v7 4/7] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

This patch adds defines for data structures and length information for
CAN XL (CAN with eXtended data Length) which can transfer up to 2048
byte insinde a single frame.

Notable changes from CAN FD:

- the 11 bit arbitration field is now named 'priority' instead of 'can_id'
  (there are no 29 bit identifiers nor RTR frames anymore)
- the data length needs a uint16 value to cover up to 2048 byte
  (the length element position is different to struct can[fd]_frame)
- new fields (SDT, AF) and a SEC bit have been introduced
- the virtual CAN interface identifier is not part if the CAN XL frame
  struct as this VCID value is stored in struct skbuff (analog to vlan id)

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 7b23eeeb3273..dd645ea72306 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -46,10 +46,11 @@
 #ifndef _UAPI_CAN_H
 #define _UAPI_CAN_H
 
 #include <linux/types.h>
 #include <linux/socket.h>
+#include <linux/stddef.h> /* for offsetof */
 
 /* controller area network (CAN) kernel definitions */
 
 /* special address description flags for the CAN_ID */
 #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
@@ -58,10 +59,11 @@
 
 /* valid bits in CAN ID for frame formats */
 #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
 #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
 #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
+#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
 
 /*
  * Controller Area Network Identifier structure
  *
  * bit 0-28	: CAN identifier (11/29 bit)
@@ -71,10 +73,11 @@
  */
 typedef __u32 canid_t;
 
 #define CAN_SFF_ID_BITS		11
 #define CAN_EFF_ID_BITS		29
+#define CANXL_PRIO_BITS		CAN_SFF_ID_BITS
 
 /*
  * Controller Area Network Error Message Frame Mask structure
  *
  * bit 0-28	: error class mask (see include/uapi/linux/can/error.h)
@@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
 
 /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
 #define CANFD_MAX_DLC 15
 #define CANFD_MAX_DLEN 64
 
+/*
+ * CAN XL payload length and DLC definitions according to ISO 11898-1
+ * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
+ */
+#define CANXL_MIN_DLC 0
+#define CANXL_MAX_DLC 2047
+#define CANXL_MAX_DLC_MASK 0x07FF
+#define CANXL_MIN_DLEN 1
+#define CANXL_MAX_DLEN 2048
+
 /**
  * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
  * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
  * @len:      CAN frame payload length in byte (0 .. 8)
  * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
@@ -164,12 +177,50 @@ struct canfd_frame {
 	__u8    __res0;  /* reserved / padding */
 	__u8    __res1;  /* reserved / padding */
 	__u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
 };
 
+/*
+ * defined bits for canxl_frame.flags
+ *
+ * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
+ * and shares the relative position of the struct can[fd]_frame.len element.
+ * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
+ * As a side effect setting this bit intentionally breaks the length checks
+ * for Classical CAN and CAN FD frames.
+ *
+ * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
+ */
+#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
+#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
+
+/**
+ * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
+ * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
+ * @flags: additional flags for CAN XL
+ * @sdt:   SDU (service data unit) type
+ * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
+ * @af:    acceptance field
+ * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
+ *
+ * @prio shares the same position as @can_id from struct can[fd]_frame.
+ */
+struct canxl_frame {
+	canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
+	__u8    flags; /* additional flags for CAN XL */
+	__u8    sdt;   /* SDU (service data unit) type */
+	__u16   len;   /* frame payload length in byte */
+	__u32   af;    /* acceptance field */
+	__u8    data[CANXL_MAX_DLEN];
+};
+
 #define CAN_MTU		(sizeof(struct can_frame))
 #define CANFD_MTU	(sizeof(struct canfd_frame))
+#define CANXL_MTU	(sizeof(struct canxl_frame))
+#define CANXL_HDR_SIZE	(offsetof(struct canxl_frame, data))
+#define CANXL_MIN_MTU	(CANXL_HDR_SIZE + 64)
+#define CANXL_MAX_MTU	CANXL_MTU
 
 /* particular protocols of the protocol family PF_CAN */
 #define CAN_RAW		1 /* RAW sockets */
 #define CAN_BCM		2 /* Broadcast Manager */
 #define CAN_TP16	3 /* VAG Transport Protocol v1.6 */
-- 
2.30.2

[PATCH v7 5/7] can: canxl: update CAN infrastructure for CAN XL frames

[Oliver Hartkopp]

- add new ETH_P_CANXL ethernet protocol type
- update skb checks for CAN XL
- add alloc_canxl_skb() which now needs a data length parameter
- introduce init_can_skb_reserve() to reduce code duplication

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev/skb.c     | 72 ++++++++++++++++++++++++++---------
 include/linux/can/skb.h       | 23 ++++++++++-
 include/uapi/linux/if_ether.h |  1 +
 net/can/af_can.c              | 25 +++++++++++-
 4 files changed, 101 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index dd3854a56af5..7c39b6de66ca 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -179,10 +179,24 @@ void can_free_echo_skb(struct net_device *dev, unsigned int idx,
 		priv->echo_skb[idx] = NULL;
 	}
 }
 EXPORT_SYMBOL_GPL(can_free_echo_skb);
 
+/* fill common values for CAN sk_buffs */
+static void init_can_skb_reserve(struct sk_buff *skb)
+{
+	skb->pkt_type = PACKET_BROADCAST;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	skb_reset_mac_header(skb);
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
+
+	can_skb_reserve(skb);
+	can_skb_prv(skb)->skbcnt = 0;
+}
+
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 {
 	struct sk_buff *skb;
 
 	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
@@ -192,20 +206,12 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 
 		return NULL;
 	}
 
 	skb->protocol = htons(ETH_P_CAN);
-	skb->pkt_type = PACKET_BROADCAST;
-	skb->ip_summed = CHECKSUM_UNNECESSARY;
-
-	skb_reset_mac_header(skb);
-	skb_reset_network_header(skb);
-	skb_reset_transport_header(skb);
-
-	can_skb_reserve(skb);
+	init_can_skb_reserve(skb);
 	can_skb_prv(skb)->ifindex = dev->ifindex;
-	can_skb_prv(skb)->skbcnt = 0;
 
 	*cf = skb_put_zero(skb, sizeof(struct can_frame));
 
 	return skb;
 }
@@ -223,30 +229,55 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 
 		return NULL;
 	}
 
 	skb->protocol = htons(ETH_P_CANFD);
-	skb->pkt_type = PACKET_BROADCAST;
-	skb->ip_summed = CHECKSUM_UNNECESSARY;
-
-	skb_reset_mac_header(skb);
-	skb_reset_network_header(skb);
-	skb_reset_transport_header(skb);
-
-	can_skb_reserve(skb);
+	init_can_skb_reserve(skb);
 	can_skb_prv(skb)->ifindex = dev->ifindex;
-	can_skb_prv(skb)->skbcnt = 0;
 
 	*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
 
 	/* set CAN FD flag by default */
 	(*cfd)->flags = CANFD_FDF;
 
 	return skb;
 }
 EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+				struct canxl_frame **cfx,
+				unsigned int data_len)
+{
+	struct sk_buff *skb;
+
+	if (data_len < CANXL_MIN_DLEN || data_len > CANXL_MAX_DLEN)
+		goto out_error;
+
+	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+			       CANXL_HDR_SIZE + data_len);
+	if (unlikely(!skb))
+		goto out_error;
+
+	skb->protocol = htons(ETH_P_CANXL);
+	init_can_skb_reserve(skb);
+	can_skb_prv(skb)->ifindex = dev->ifindex;
+
+	*cfx = skb_put_zero(skb, CANXL_HDR_SIZE + data_len);
+
+	/* set CAN XL flag and length information by default */
+	(*cfx)->flags = CANXL_XLF;
+	(*cfx)->len = data_len;
+
+	return skb;
+
+out_error:
+	*cfx = NULL;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(alloc_canxl_skb);
+
 struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
 {
 	struct sk_buff *skb;
 
 	skb = alloc_can_skb(dev, cf);
@@ -311,10 +342,15 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
 	case ETH_P_CANFD:
 		if (!can_is_canfd_skb(skb))
 			goto inval_skb;
 		break;
 
+	case ETH_P_CANXL:
+		if (!can_is_canxl_skb(skb))
+			goto inval_skb;
+		break;
+
 	default:
 		goto inval_skb;
 	}
 
 	if (!can_skb_headroom_valid(dev, skb)) {
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index ddffc2fc008c..01c01b1261fa 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -28,10 +28,13 @@ unsigned int __must_check can_get_echo_skb(struct net_device *dev,
 void can_free_echo_skb(struct net_device *dev, unsigned int idx,
 		       unsigned int *frame_len_ptr);
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
 struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 				struct canfd_frame **cfd);
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+				struct canxl_frame **cfx,
+				unsigned int data_len);
 struct sk_buff *alloc_can_err_skb(struct net_device *dev,
 				  struct can_frame **cf);
 bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
 
 /*
@@ -112,15 +115,33 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
 
 	/* the CAN specific type of skb is identified by its data length */
 	return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
 }
 
-/* get length element value from can[fd]_frame structure */
+static inline bool can_is_canxl_skb(const struct sk_buff *skb)
+{
+	const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
+
+	if (skb->len < CANXL_HDR_SIZE + CANXL_MIN_DLEN || skb->len > CANXL_MTU)
+		return false;
+
+	/* this also checks valid CAN XL data length boundaries */
+	if (skb->len != CANXL_HDR_SIZE + cfx->len)
+		return false;
+
+	return cfx->flags & CANXL_XLF;
+}
+
+/* get length element value from can[|fd|xl]_frame structure */
 static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
 {
+	const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
 	const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
+	if (can_is_canxl_skb(skb))
+		return cfx->len;
+
 	return cfd->len;
 }
 
 /* get needed data length inside CAN frame for all frame types (RTR aware) */
 static inline unsigned int can_skb_get_data_len(struct sk_buff *skb)
diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index d370165bc621..69e0457eb200 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -136,10 +136,11 @@
 #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
 #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
 #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
 #define ETH_P_CAN	0x000C		/* CAN: Controller Area Network */
 #define ETH_P_CANFD	0x000D		/* CANFD: CAN flexible data rate*/
+#define ETH_P_CANXL	0x000E		/* CANXL: eXtended frame Length */
 #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
 #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
 #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
 #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
 #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 072a6a5c9dd1..9503ab10f9b8 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -200,11 +200,13 @@ int can_send(struct sk_buff *skb, int loop)
 {
 	struct sk_buff *newskb = NULL;
 	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
 	int err = -EINVAL;
 
-	if (can_is_can_skb(skb)) {
+	if (can_is_canxl_skb(skb)) {
+		skb->protocol = htons(ETH_P_CANXL);
+	} else if (can_is_can_skb(skb)) {
 		skb->protocol = htons(ETH_P_CAN);
 	} else if (can_is_canfd_skb(skb)) {
 		struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
 		skb->protocol = htons(ETH_P_CANFD);
@@ -700,10 +702,25 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	can_receive(skb, dev);
 	return NET_RX_SUCCESS;
 }
 
+static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
+		     struct packet_type *pt, struct net_device *orig_dev)
+{
+	if (unlikely(dev->type != ARPHRD_CAN || (!can_is_canxl_skb(skb)))) {
+		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
+			     dev->type, skb->len);
+
+		kfree_skb(skb);
+		return NET_RX_DROP;
+	}
+
+	can_receive(skb, dev);
+	return NET_RX_SUCCESS;
+}
+
 /* af_can protocol functions */
 
 /**
  * can_proto_register - register CAN transport protocol
  * @cp: pointer to CAN protocol structure
@@ -824,10 +841,15 @@ static struct packet_type can_packet __read_mostly = {
 static struct packet_type canfd_packet __read_mostly = {
 	.type = cpu_to_be16(ETH_P_CANFD),
 	.func = canfd_rcv,
 };
 
+static struct packet_type canxl_packet __read_mostly = {
+	.type = cpu_to_be16(ETH_P_CANXL),
+	.func = canxl_rcv,
+};
+
 static const struct net_proto_family can_family_ops = {
 	.family = PF_CAN,
 	.create = can_create,
 	.owner  = THIS_MODULE,
 };
@@ -863,10 +885,11 @@ static __init int can_init(void)
 	if (err)
 		goto out_sock;
 
 	dev_add_pack(&can_packet);
 	dev_add_pack(&canfd_packet);
+	dev_add_pack(&canxl_packet);
 
 	return 0;
 
 out_sock:
 	unregister_pernet_subsys(&can_pernet_ops);
-- 
2.30.2

[PATCH v7 6/7] can: dev: add CAN XL support to virtual CAN

[Oliver Hartkopp]

Make use of new can_skb_get_data_len() helper.
Add support for variable CANXL MTU using the new can_is_canxl_dev_mtu().

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/vcan.c  | 12 ++++++------
 drivers/net/can/vxcan.c |  8 ++++----
 include/linux/can/dev.h |  5 +++++
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index a15619d883ec..6c4d4ef93e80 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -68,33 +68,32 @@ static bool echo; /* echo testing. Default: 0 (Off) */
 module_param(echo, bool, 0444);
 MODULE_PARM_DESC(echo, "Echo sent frames (for testing). Default: 0 (Off)");
 
 static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 {
-	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct net_device_stats *stats = &dev->stats;
 
 	stats->rx_packets++;
-	stats->rx_bytes += cfd->len;
+	stats->rx_bytes += can_skb_get_data_len(skb);
 
 	skb->pkt_type  = PACKET_BROADCAST;
 	skb->dev       = dev;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	netif_rx(skb);
 }
 
 static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 {
-	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct net_device_stats *stats = &dev->stats;
-	int loop, len;
+	unsigned int len;
+	int loop;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
 
-	len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
+	len = can_skb_get_data_len(skb);
 	stats->tx_packets++;
 	stats->tx_bytes += len;
 
 	/* set flag whether this packet has to be looped back */
 	loop = skb->pkt_type == PACKET_LOOPBACK;
@@ -132,11 +131,12 @@ static int vcan_change_mtu(struct net_device *dev, int new_mtu)
 {
 	/* Do not allow changing the MTU while running */
 	if (dev->flags & IFF_UP)
 		return -EBUSY;
 
-	if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU)
+	if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU &&
+	    !can_is_canxl_dev_mtu(new_mtu))
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
 	return 0;
 }
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 577a80300514..189f51428c24 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -35,14 +35,13 @@ struct vxcan_priv {
 
 static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
 {
 	struct vxcan_priv *priv = netdev_priv(dev);
 	struct net_device *peer;
-	struct canfd_frame *cfd = (struct canfd_frame *)oskb->data;
 	struct net_device_stats *peerstats, *srcstats = &dev->stats;
 	struct sk_buff *skb;
-	u8 len;
+	unsigned int len;
 
 	if (can_dropped_invalid_skb(dev, oskb))
 		return NETDEV_TX_OK;
 
 	rcu_read_lock();
@@ -65,11 +64,11 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
 	skb->csum_start = 0;
 	skb->pkt_type   = PACKET_BROADCAST;
 	skb->dev        = peer;
 	skb->ip_summed  = CHECKSUM_UNNECESSARY;
 
-	len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
+	len = can_skb_get_data_len(skb);
 	if (netif_rx(skb) == NET_RX_SUCCESS) {
 		srcstats->tx_packets++;
 		srcstats->tx_bytes += len;
 		peerstats = &peer->stats;
 		peerstats->rx_packets++;
@@ -127,11 +126,12 @@ static int vxcan_change_mtu(struct net_device *dev, int new_mtu)
 {
 	/* Do not allow changing the MTU while running */
 	if (dev->flags & IFF_UP)
 		return -EBUSY;
 
-	if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU)
+	if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU &&
+	    !can_is_canxl_dev_mtu(new_mtu))
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
 	return 0;
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index e22dc03c850e..8245ec7d2667 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -144,10 +144,15 @@ static inline int __must_check can_set_static_ctrlmode(struct net_device *dev,
 static inline u32 can_get_static_ctrlmode(struct can_priv *priv)
 {
 	return priv->ctrlmode & ~priv->ctrlmode_supported;
 }
 
+static inline bool can_is_canxl_dev_mtu(unsigned int mtu)
+{
+	return (mtu >= CANXL_MIN_MTU && mtu <= CANXL_MAX_MTU);
+}
+
 void can_setup(struct net_device *dev);
 
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
 				    unsigned int txqs, unsigned int rxqs);
 #define alloc_candev(sizeof_priv, echo_skb_max) \
-- 
2.30.2

[PATCH v7 7/7] can: raw: add CAN XL support

[Oliver Hartkopp]

Enable CAN_RAW sockets to read and write CAN XL frames analogue to the
CAN FD extension (new CAN_RAW_XL_FRAMES sockopt).

A CAN XL network interface is capable to handle Classical CAN, CAN FD and
CAN XL frames. When CAN_RAW_XL_FRAMES is enabled, the CAN_RAW socket checks
whether the addressed CAN network interface is capable to handle the
provided CAN frame.

In opposite to the fixed number of bytes for
- CAN frames (CAN_MTU = sizeof(struct can_frame))
- CAN FD frames (CANFD_MTU = sizeof(struct can_frame))
the number of bytes when reading/writing CAN XL frames depends on the
number of data bytes. For efficiency reasons the length of the struct
canxl_frame is truncated to the needed size for read/write operations.
This leads to a calculated size of CANXL_HDR_SIZE + canxl_frame::len which
is enforced on write() operations and guaranteed on read() operations.

NB: Valid length values are 1 .. 2048 (CANXL_MIN_DLEN .. CANXL_MAX_DLEN).

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/uapi/linux/can/raw.h |  1 +
 net/can/raw.c                | 55 ++++++++++++++++++++++++++++--------
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 3386aa81fdf2..ff12f525c37c 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -60,8 +60,9 @@ enum {
 	CAN_RAW_ERR_FILTER,	/* set filter for error frames       */
 	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
 	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
 	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
 	CAN_RAW_JOIN_FILTERS,	/* all filters must match to trigger */
+	CAN_RAW_XL_FRAMES,	/* allow CAN XL frames (default:off) */
 };
 
 #endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/raw.c b/net/can/raw.c
index d1bd9cc51ebe..c6859a061a7c 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -48,10 +48,11 @@
 #include <linux/socket.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
 #include <linux/can/core.h>
+#include <linux/can/dev.h> /* for can_is_canxl_dev_mtu() */
 #include <linux/can/skb.h>
 #include <linux/can/raw.h>
 #include <net/sock.h>
 #include <net/net_namespace.h>
 
@@ -85,10 +86,11 @@ struct raw_sock {
 	int ifindex;
 	struct list_head notifier;
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int xl_frames;
 	int join_filters;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
 	can_err_mask_t err_mask;
@@ -127,12 +129,13 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 
 	/* check the received tx sock reference */
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
-	/* do not pass non-CAN2.0 frames to a legacy socket */
-	if (!ro->fd_frames && oskb->len != CAN_MTU)
+	/* make sure to not pass oversized frames to the socket */
+	if ((can_is_canfd_skb(oskb) && !ro->fd_frames && !ro->xl_frames) ||
+	    (can_is_canxl_skb(oskb) && !ro->xl_frames))
 		return;
 
 	/* eliminate multiple filter matches for the same skb */
 	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
 	    this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
@@ -344,10 +347,11 @@ static int raw_init(struct sock *sk)
 
 	/* set default loopback behaviour */
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->xl_frames        = 0;
 	ro->join_filters     = 0;
 
 	/* alloc_percpu provides zero'ed memory */
 	ro->uniq = alloc_percpu(struct uniqframe);
 	if (unlikely(!ro->uniq))
@@ -667,10 +671,19 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (copy_from_sockptr(&ro->fd_frames, optval, optlen))
 			return -EFAULT;
 
 		break;
 
+	case CAN_RAW_XL_FRAMES:
+		if (optlen != sizeof(ro->xl_frames))
+			return -EINVAL;
+
+		if (copy_from_sockptr(&ro->xl_frames, optval, optlen))
+			return -EFAULT;
+
+		break;
+
 	case CAN_RAW_JOIN_FILTERS:
 		if (optlen != sizeof(ro->join_filters))
 			return -EINVAL;
 
 		if (copy_from_sockptr(&ro->join_filters, optval, optlen))
@@ -749,10 +762,16 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		if (len > sizeof(int))
 			len = sizeof(int);
 		val = &ro->fd_frames;
 		break;
 
+	case CAN_RAW_XL_FRAMES:
+		if (len > sizeof(int))
+			len = sizeof(int);
+		val = &ro->xl_frames;
+		break;
+
 	case CAN_RAW_JOIN_FILTERS:
 		if (len > sizeof(int))
 			len = sizeof(int);
 		val = &ro->join_filters;
 		break;
@@ -774,11 +793,15 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	struct raw_sock *ro = raw_sk(sk);
 	struct sockcm_cookie sockc;
 	struct sk_buff *skb;
 	struct net_device *dev;
 	int ifindex;
-	int err;
+	int err = -EINVAL;
+
+	/* check for valid CAN frame sizes */
+	if (size < CANXL_HDR_SIZE + CANXL_MIN_DLEN || size > CANXL_MTU)
+		return -EINVAL;
 
 	if (msg->msg_name) {
 		DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
 
 		if (msg->msg_namelen < RAW_MIN_NAMELEN)
@@ -794,32 +817,40 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 	dev = dev_get_by_index(sock_net(sk), ifindex);
 	if (!dev)
 		return -ENXIO;
 
-	err = -EINVAL;
-	if (ro->fd_frames && dev->mtu == CANFD_MTU) {
-		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
-			goto put_dev;
-	} else {
-		if (unlikely(size != CAN_MTU))
-			goto put_dev;
-	}
-
 	skb = sock_alloc_send_skb(sk, size + sizeof(struct can_skb_priv),
 				  msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
 		goto put_dev;
 
 	can_skb_reserve(skb);
 	can_skb_prv(skb)->ifindex = dev->ifindex;
 	can_skb_prv(skb)->skbcnt = 0;
 
+	/* fill the skb before testing for valid CAN frames */
 	err = memcpy_from_msg(skb_put(skb, size), msg, size);
 	if (err < 0)
 		goto free_skb;
 
+	err = -EINVAL;
+	if (ro->xl_frames && can_is_canxl_dev_mtu(dev->mtu)) {
+		/* CAN XL, CAN FD and Classical CAN */
+		if (!can_is_canxl_skb(skb) && !can_is_canfd_skb(skb) &&
+		    !can_is_can_skb(skb))
+			goto free_skb;
+	} else if (ro->fd_frames && dev->mtu == CANFD_MTU) {
+		/* CAN FD and Classical CAN */
+		if (!can_is_canfd_skb(skb) && !can_is_can_skb(skb))
+			goto free_skb;
+	} else {
+		/* Classical CAN */
+		if (!can_is_can_skb(skb))
+			goto free_skb;
+	}
+
 	sockcm_init(&sockc, sk);
 	if (msg->msg_controllen) {
 		err = sock_cmsg_send(sk, msg, &sockc);
 		if (unlikely(err))
 			goto free_skb;
-- 
2.30.2

Re: [PATCH v7 4/7] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Sat. 30 Jul. 2022 at 00:42, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> This patch adds defines for data structures and length information for
> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
> byte insinde a single frame.
       ^^^^^^^
inside

> Notable changes from CAN FD:
>
> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
>   (there are no 29 bit identifiers nor RTR frames anymore)
> - the data length needs a uint16 value to cover up to 2048 byte
>   (the length element position is different to struct can[fd]_frame)
> - new fields (SDT, AF) and a SEC bit have been introduced
> - the virtual CAN interface identifier is not part if the CAN XL frame
>   struct as this VCID value is stored in struct skbuff (analog to vlan id)
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 7b23eeeb3273..dd645ea72306 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -46,10 +46,11 @@
>  #ifndef _UAPI_CAN_H
>  #define _UAPI_CAN_H
>
>  #include <linux/types.h>
>  #include <linux/socket.h>
> +#include <linux/stddef.h> /* for offsetof */
>
>  /* controller area network (CAN) kernel definitions */
>
>  /* special address description flags for the CAN_ID */
>  #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
> @@ -58,10 +59,11 @@
>
>  /* valid bits in CAN ID for frame formats */
>  #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
>  #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
>  #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>
>  /*
>   * Controller Area Network Identifier structure
>   *
>   * bit 0-28    : CAN identifier (11/29 bit)
> @@ -71,10 +73,11 @@
>   */
>  typedef __u32 canid_t;
>
>  #define CAN_SFF_ID_BITS                11
>  #define CAN_EFF_ID_BITS                29
> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
>
>  /*
>   * Controller Area Network Error Message Frame Mask structure
>   *
>   * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
> @@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
>
>  /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>  #define CANFD_MAX_DLC 15
>  #define CANFD_MAX_DLEN 64
>
> +/*
> + * CAN XL payload length and DLC definitions according to ISO 11898-1
> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
> + */
> +#define CANXL_MIN_DLC 0
> +#define CANXL_MAX_DLC 2047
> +#define CANXL_MAX_DLC_MASK 0x07FF
> +#define CANXL_MIN_DLEN 1
> +#define CANXL_MAX_DLEN 2048
> +
>  /**
>   * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
>   * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>   * @len:      CAN frame payload length in byte (0 .. 8)
>   * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
> @@ -164,12 +177,50 @@ struct canfd_frame {
>         __u8    __res0;  /* reserved / padding */
>         __u8    __res1;  /* reserved / padding */
>         __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>  };
>
> +/*
> + * defined bits for canxl_frame.flags
> + *
> + * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
> + * and shares the relative position of the struct can[fd]_frame.len element.
> + * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
> + * As a side effect setting this bit intentionally breaks the length checks
> + * for Classical CAN and CAN FD frames.
> + *
> + * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
> + */
> +#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
> +#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
> +
> +/**
> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> + * @flags: additional flags for CAN XL
> + * @sdt:   SDU (service data unit) type
> + * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
> + * @af:    acceptance field
> + * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
> + *
> + * @prio shares the same position as @can_id from struct can[fd]_frame.
> + */
> +struct canxl_frame {
> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> +       __u8    flags; /* additional flags for CAN XL */
> +       __u8    sdt;   /* SDU (service data unit) type */
> +       __u16   len;   /* frame payload length in byte */
> +       __u32   af;    /* acceptance field */
> +       __u8    data[CANXL_MAX_DLEN];
> +};

I read the previous threads from the start and there is one point
which, I think, did not get enough attention.

With the current proposal, there is one pitfall on the user side. A
correct way to differentiate between the Classical CAN, CAN-FD and
CAN-XL would be, as discussed:

union {
         struct can_frame cc;
         struct canfd_frame fd;
         struct canxl_frame xl;
} can;

nbytes = read(s, &can.xl, sizeof(struct canxl_frame));

if (can.xl.flags & CANXL_XLF)
    /* Handle CAN XL frame */
else if (can.fd.flags & CANFD_FDF) /* or equivalent: if (nbytes == CANFD_MTU) */
    /* Handle CAN FD frame */
else
       /* Handle Classic CAN frame */

However, it becomes incorrect if the order is changed:

if (can.fd.flags & CANFD_FDF) /* or equivalent: if (nbytes == CANFD_MTU) */
    /* can.fd.flags can collide with can.xl.sdt,  */
    /* nbytes == CANFD_MTU would also collide if the CAN XL frame
length is 32 bytes  */
else if (can.xl.flags & CANXL_XLF)
    /* Handle CAN XL frame */
else
       /* Handle Classic CAN frame */

I see this as a big pitfall.



A solution (or rather a compromise) would be to align the flags fields
between the canfd_frame and canxl_frame.

#define CANXL_XLF 0x08 /* mandatory CAN XL frame flag (must always be set!) */
#define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */

struct canxl_frame {
       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
       __u8    sdt;   /* SDU (service data unit) type */
       __u8    flags; /* flags for CAN FD and CAN XL */
       __u16   len;   /* frame payload length in byte */
       __u32   af;    /* acceptance field */
       __u8    data[CANXL_MAX_DLEN];
};

This way, the check between CANFD_FDF and CANXL_XLF can be done in any
order. A side benefit is that you do not need a union anymore to check
the frame type.


I say this is a compromise because it adds back the collision between
can{fd,xl}_frame::flags and can_frame::__pad.

Existing applications are exempted from this risk: the
CAN_RAW_XL_FRAMES socket option will prevent the collision from
happening. To summarise, the collision will only occur if below
criteria are met:

  * The user allocates a can_frame without zeroing it (typical use
cases are stack declaration or malloc()).
  * The user activates the CAN_RAW_XL_FRAMES socket option.
  * Garbage value results in can_frame::__pad & CANXL_XLF being set.

So if going in that direction, we would have to update the
documentation (header files and kernel doc) to point at this pitfall
and specify that can_frame::__pad shall always be zeroed for mix
usages.

Personally, I prefer to have the can{fd,xl}_frame::flags aligned. I
foresee that misuses due to bad "if" ordering are more likely to
happen. Zeroing your stack variable is a best practice, developers
with good hygiene are safe. Requiring the two "if" checks in the
correct order is more obscure and more luckily to be misused even with
best intentions.

So here are two solutions, each of them has their benefits and
drawbacks. I vote for aligning the flags. Pick your poison.


>  #define CAN_MTU                (sizeof(struct can_frame))
>  #define CANFD_MTU      (sizeof(struct canfd_frame))
> +#define CANXL_MTU      (sizeof(struct canxl_frame))
> +#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
> +#define CANXL_MIN_MTU  (CANXL_HDR_SIZE + 64)
> +#define CANXL_MAX_MTU  CANXL_MTU
>
>  /* particular protocols of the protocol family PF_CAN */
>  #define CAN_RAW                1 /* RAW sockets */
>  #define CAN_BCM                2 /* Broadcast Manager */
>  #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */
> --
> 2.30.2
>

Re: [PATCH v7 4/7] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

Hi Vincent,

On 30.07.22 04:13, Vincent Mailhol wrote:
> On Sat. 30 Jul. 2022 at 00:42, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> This patch adds defines for data structures and length information for
>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
>> byte insinde a single frame.
>         ^^^^^^^
> inside

ok. Thx.

@Marc: Should I send an updated series or would you fix this typo while 
committing the patch?

> 
>> Notable changes from CAN FD:
>>
>> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
>>    (there are no 29 bit identifiers nor RTR frames anymore)
>> - the data length needs a uint16 value to cover up to 2048 byte
>>    (the length element position is different to struct can[fd]_frame)
>> - new fields (SDT, AF) and a SEC bit have been introduced
>> - the virtual CAN interface identifier is not part if the CAN XL frame
>>    struct as this VCID value is stored in struct skbuff (analog to vlan id)
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>
>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
>> index 7b23eeeb3273..dd645ea72306 100644
>> --- a/include/uapi/linux/can.h
>> +++ b/include/uapi/linux/can.h
>> @@ -46,10 +46,11 @@
>>   #ifndef _UAPI_CAN_H
>>   #define _UAPI_CAN_H
>>
>>   #include <linux/types.h>
>>   #include <linux/socket.h>
>> +#include <linux/stddef.h> /* for offsetof */
>>
>>   /* controller area network (CAN) kernel definitions */
>>
>>   /* special address description flags for the CAN_ID */
>>   #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
>> @@ -58,10 +59,11 @@
>>
>>   /* valid bits in CAN ID for frame formats */
>>   #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
>>   #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
>>   #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>>
>>   /*
>>    * Controller Area Network Identifier structure
>>    *
>>    * bit 0-28    : CAN identifier (11/29 bit)
>> @@ -71,10 +73,11 @@
>>    */
>>   typedef __u32 canid_t;
>>
>>   #define CAN_SFF_ID_BITS                11
>>   #define CAN_EFF_ID_BITS                29
>> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
>>
>>   /*
>>    * Controller Area Network Error Message Frame Mask structure
>>    *
>>    * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
>> @@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
>>
>>   /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>>   #define CANFD_MAX_DLC 15
>>   #define CANFD_MAX_DLEN 64
>>
>> +/*
>> + * CAN XL payload length and DLC definitions according to ISO 11898-1
>> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
>> + */
>> +#define CANXL_MIN_DLC 0
>> +#define CANXL_MAX_DLC 2047
>> +#define CANXL_MAX_DLC_MASK 0x07FF
>> +#define CANXL_MIN_DLEN 1
>> +#define CANXL_MAX_DLEN 2048
>> +
>>   /**
>>    * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
>>    * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>>    * @len:      CAN frame payload length in byte (0 .. 8)
>>    * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
>> @@ -164,12 +177,50 @@ struct canfd_frame {
>>          __u8    __res0;  /* reserved / padding */
>>          __u8    __res1;  /* reserved / padding */
>>          __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>>   };
>>
>> +/*
>> + * defined bits for canxl_frame.flags
>> + *
>> + * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
>> + * and shares the relative position of the struct can[fd]_frame.len element.
>> + * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
>> + * As a side effect setting this bit intentionally breaks the length checks
>> + * for Classical CAN and CAN FD frames.
>> + *
>> + * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
>> + */
>> +#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
>> +#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
>> +
>> +/**
>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
>> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
>> + * @flags: additional flags for CAN XL
>> + * @sdt:   SDU (service data unit) type
>> + * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
>> + * @af:    acceptance field
>> + * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
>> + *
>> + * @prio shares the same position as @can_id from struct can[fd]_frame.
>> + */
>> +struct canxl_frame {
>> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>> +       __u8    flags; /* additional flags for CAN XL */
>> +       __u8    sdt;   /* SDU (service data unit) type */
>> +       __u16   len;   /* frame payload length in byte */
>> +       __u32   af;    /* acceptance field */
>> +       __u8    data[CANXL_MAX_DLEN];
>> +};
> 
> I read the previous threads from the start and there is one point
> which, I think, did not get enough attention.
> 
> With the current proposal, there is one pitfall on the user side. A
> correct way to differentiate between the Classical CAN, CAN-FD and
> CAN-XL would be, as discussed:
> 
> union {
>           struct can_frame cc;
>           struct canfd_frame fd;
>           struct canxl_frame xl;
> } can;
> 
> nbytes = read(s, &can.xl, sizeof(struct canxl_frame));
> 
> if (can.xl.flags & CANXL_XLF)
>      /* Handle CAN XL frame */
> else if (can.fd.flags & CANFD_FDF) /* or equivalent: if (nbytes == CANFD_MTU) */
>      /* Handle CAN FD frame */
> else
>         /* Handle Classic CAN frame */
> 
> However, it becomes incorrect if the order is changed:
> 
> if (can.fd.flags & CANFD_FDF) /* or equivalent: if (nbytes == CANFD_MTU) */

No. The point is, that you might have a CAN XL frame with a data length 
of 60 which becomes (when adding the CANXL_HDR_SIZE) 72 which is CANFD_MTU.

For that reason the distinction for CAN XL frames (and the 'old' stuff) is

     if (can.xl.flags & CANXL_XLF) {}

in the first place.

>      /* can.fd.flags can collide with can.xl.sdt,  */
>      /* nbytes == CANFD_MTU would also collide if the CAN XL frame
> length is 32 bytes  */
> else if (can.xl.flags & CANXL_XLF)
>      /* Handle CAN XL frame */
> else
>         /* Handle Classic CAN frame */
> 
> I see this as a big pitfall.
> 
> 
> 
> A solution (or rather a compromise) would be to align the flags fields
> between the canfd_frame and canxl_frame.
> 
> #define CANXL_XLF 0x08 /* mandatory CAN XL frame flag (must always be set!) */
> #define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
> 
> struct canxl_frame {
>         canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>         __u8    sdt;   /* SDU (service data unit) type */
>         __u8    flags; /* flags for CAN FD and CAN XL */
>         __u16   len;   /* frame payload length in byte */
>         __u32   af;    /* acceptance field */
>         __u8    data[CANXL_MAX_DLEN];
> };
> 
> This way, the check between CANFD_FDF and CANXL_XLF can be done in any
> order. A side benefit is that you do not need a union anymore to check
> the frame type.

This has no impact on the union. When you write code that is able to 
cope with all three CAN frame types you need this kind of union, right?

> I say this is a compromise because it adds back the collision between
> can{fd,xl}_frame::flags and can_frame::__pad.
> 
> Existing applications are exempted from this risk: the
> CAN_RAW_XL_FRAMES socket option will prevent the collision from
> happening. To summarise, the collision will only occur if below
> criteria are met:
> 
>    * The user allocates a can_frame without zeroing it (typical use
> cases are stack declaration or malloc()).
>    * The user activates the CAN_RAW_XL_FRAMES socket option.
>    * Garbage value results in can_frame::__pad & CANXL_XLF being set.
> 
> So if going in that direction, we would have to update the
> documentation (header files and kernel doc) to point at this pitfall
> and specify that can_frame::__pad shall always be zeroed for mix
> usages.

But you would add this recommendation to a *very late point* - namely 
after 14 years of SocketCAN in the mainline kernel.

If you think about the rule "never break user space" I wonder if the 
risk turns out to be higher when people enable CAN_RAW_XL_FRAMES ("as 
seen on Stackoverflow") and not read the kernel doc recommendations to 
review/update their existing code.

With the current layout it can be clearly stated that after enabling 
CAN_RAW_XL_FRAMES you will be able to detect CAN XL frames with

if (can.xl.flags & CANXL_XLF) {}

And if this bit is not set you can check for the old stuff (CAN FD and 
Classical CAN).

The good thing about having can[fd]_frame::len and canxl_frame::flags at 
the same location is that this field is (and was!) *always* set (== 
initialized) properly. No potential risks with existing implementations 
that probably did not initialize reserved or padding bytes.

And the fact that CANXL_XLF with its value of 128 intentionally breaks 
the CAN frame length rules for CAN/CANFD can be used too.

E.g. in include/linux/can/skb.h

static inline bool can_is_can_skb(const struct sk_buff *skb)
{
         struct can_frame *cf = (struct can_frame *)skb->data;

         /* the CAN specific type of skb is identified by its data length */
         return (skb->len == CAN_MTU && cf->len <= CAN_MAX_DLEN);
}

static inline bool can_is_canfd_skb(const struct sk_buff *skb)
{
         struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

         /* the CAN specific type of skb is identified by its data length */
         return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
}

Using this simple and efficient check makes it comparably easy to detect 
CAN/CANFD/CANXL content inside the Linux kernel (and it can also be used 
in user space).

> Personally, I prefer to have the can{fd,xl}_frame::flags aligned.

Summing up I see much more risks of uninitialized content and potential 
misuse from existing code when the can{fd,xl}_frame::flags were aligned 
according to your suggestion.

I would prefer having can[fd]_frame::len and canxl_frame::flags at the 
same (always initialized) location for the reasons described above.

Best regards,
Oliver


> I foresee that misuses due to bad "if" ordering are more likely to
> happen. Zeroing your stack variable is a best practice, developers
> with good hygiene are safe. Requiring the two "if" checks in the
> correct order is more obscure and more luckily to be misused even with
> best intentions.
> 
> So here are two solutions, each of them has their benefits and
> drawbacks. I vote for aligning the flags. Pick your poison.
> 
> 
>>   #define CAN_MTU                (sizeof(struct can_frame))
>>   #define CANFD_MTU      (sizeof(struct canfd_frame))
>> +#define CANXL_MTU      (sizeof(struct canxl_frame))
>> +#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
>> +#define CANXL_MIN_MTU  (CANXL_HDR_SIZE + 64)
>> +#define CANXL_MAX_MTU  CANXL_MTU
>>
>>   /* particular protocols of the protocol family PF_CAN */
>>   #define CAN_RAW                1 /* RAW sockets */
>>   #define CAN_BCM                2 /* Broadcast Manager */
>>   #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */
>> --
>> 2.30.2
>>

Re: [PATCH v7 4/7] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Sat. 30 juil. 2022 at 21:06, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Vincent,
>
> On 30.07.22 04:13, Vincent Mailhol wrote:
> > On Sat. 30 Jul. 2022 at 00:42, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> This patch adds defines for data structures and length information for
> >> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
> >> byte insinde a single frame.
> >         ^^^^^^^
> > inside
>
> ok. Thx.
>
> @Marc: Should I send an updated series or would you fix this typo while
> committing the patch?
>
> >
> >> Notable changes from CAN FD:
> >>
> >> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
> >>    (there are no 29 bit identifiers nor RTR frames anymore)
> >> - the data length needs a uint16 value to cover up to 2048 byte
> >>    (the length element position is different to struct can[fd]_frame)
> >> - new fields (SDT, AF) and a SEC bit have been introduced
> >> - the virtual CAN interface identifier is not part if the CAN XL frame
> >>    struct as this VCID value is stored in struct skbuff (analog to vlan id)
> >>
> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >> ---
> >>   include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 51 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> >> index 7b23eeeb3273..dd645ea72306 100644
> >> --- a/include/uapi/linux/can.h
> >> +++ b/include/uapi/linux/can.h
> >> @@ -46,10 +46,11 @@
> >>   #ifndef _UAPI_CAN_H
> >>   #define _UAPI_CAN_H
> >>
> >>   #include <linux/types.h>
> >>   #include <linux/socket.h>
> >> +#include <linux/stddef.h> /* for offsetof */
> >>
> >>   /* controller area network (CAN) kernel definitions */
> >>
> >>   /* special address description flags for the CAN_ID */
> >>   #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
> >> @@ -58,10 +59,11 @@
> >>
> >>   /* valid bits in CAN ID for frame formats */
> >>   #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
> >>   #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
> >>   #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
> >> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
> >>
> >>   /*
> >>    * Controller Area Network Identifier structure
> >>    *
> >>    * bit 0-28    : CAN identifier (11/29 bit)
> >> @@ -71,10 +73,11 @@
> >>    */
> >>   typedef __u32 canid_t;
> >>
> >>   #define CAN_SFF_ID_BITS                11
> >>   #define CAN_EFF_ID_BITS                29
> >> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
> >>
> >>   /*
> >>    * Controller Area Network Error Message Frame Mask structure
> >>    *
> >>    * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
> >> @@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
> >>
> >>   /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
> >>   #define CANFD_MAX_DLC 15
> >>   #define CANFD_MAX_DLEN 64
> >>
> >> +/*
> >> + * CAN XL payload length and DLC definitions according to ISO 11898-1
> >> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
> >> + */
> >> +#define CANXL_MIN_DLC 0
> >> +#define CANXL_MAX_DLC 2047
> >> +#define CANXL_MAX_DLC_MASK 0x07FF
> >> +#define CANXL_MIN_DLEN 1
> >> +#define CANXL_MAX_DLEN 2048
> >> +
> >>   /**
> >>    * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
> >>    * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> >>    * @len:      CAN frame payload length in byte (0 .. 8)
> >>    * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
> >> @@ -164,12 +177,50 @@ struct canfd_frame {
> >>          __u8    __res0;  /* reserved / padding */
> >>          __u8    __res1;  /* reserved / padding */
> >>          __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
> >>   };
> >>
> >> +/*
> >> + * defined bits for canxl_frame.flags
> >> + *
> >> + * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
> >> + * and shares the relative position of the struct can[fd]_frame.len element.
> >> + * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
> >> + * As a side effect setting this bit intentionally breaks the length checks
> >> + * for Classical CAN and CAN FD frames.
> >> + *
> >> + * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
> >> + */
> >> +#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
> >> +#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
> >> +
> >> +/**
> >> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> >> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> >> + * @flags: additional flags for CAN XL
> >> + * @sdt:   SDU (service data unit) type
> >> + * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
> >> + * @af:    acceptance field
> >> + * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
> >> + *
> >> + * @prio shares the same position as @can_id from struct can[fd]_frame.
> >> + */
> >> +struct canxl_frame {
> >> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >> +       __u8    flags; /* additional flags for CAN XL */
> >> +       __u8    sdt;   /* SDU (service data unit) type */
> >> +       __u16   len;   /* frame payload length in byte */
> >> +       __u32   af;    /* acceptance field */
> >> +       __u8    data[CANXL_MAX_DLEN];
> >> +};
> >
> > I read the previous threads from the start and there is one point
> > which, I think, did not get enough attention.
> >
> > With the current proposal, there is one pitfall on the user side. A
> > correct way to differentiate between the Classical CAN, CAN-FD and
> > CAN-XL would be, as discussed:
> >
> > union {
> >           struct can_frame cc;
> >           struct canfd_frame fd;
> >           struct canxl_frame xl;
> > } can;
> >
> > nbytes = read(s, &can.xl, sizeof(struct canxl_frame));
> >
> > if (can.xl.flags & CANXL_XLF)
> >      /* Handle CAN XL frame */
> > else if (can.fd.flags & CANFD_FDF) /* or equivalent: if (nbytes == CANFD_MTU) */
> >      /* Handle CAN FD frame */
> > else
> >         /* Handle Classic CAN frame */
> >
> > However, it becomes incorrect if the order is changed:
> >
> > if (can.fd.flags & CANFD_FDF) /* or equivalent: if (nbytes == CANFD_MTU) */
>
> No. The point is, that you might have a CAN XL frame with a data length
> of 60 which becomes (when adding the CANXL_HDR_SIZE) 72 which is CANFD_MTU.
>
> For that reason the distinction for CAN XL frames (and the 'old' stuff) is
>
>      if (can.xl.flags & CANXL_XLF) {}
>
> in the first place.

When I write "it becomes incorrect if..." the following example is
obviously incorrect. No need to point out that my misuse illustration
is a misuse.

I fully understand the nuance as I am the one who initially proposed
to use the 0x80 flag to differentiate between CAN XL and CAN(-FD):
https://lore.kernel.org/linux-can/CAMZ6RqLSHAdB-ocj7=74zJqWbv-EH9x8X5ARPK6Gv+FFiTZ7Lg@mail.gmail.com/
(The only difference was that I proposed to name it
CANXL_DISCRIMINATOR instead of CANXL_XLF)

But will all the users understand this nuance? What if someone doesn't
put the CANXL_XLF check in the first place? It is a bug.

> >      /* can.fd.flags can collide with can.xl.sdt,  */
> >      /* nbytes == CANFD_MTU would also collide if the CAN XL frame
> > length is 32 bytes  */
              ^^
Here, I meant 60 bytes.
> > else if (can.xl.flags & CANXL_XLF)
> >      /* Handle CAN XL frame */
> > else
> >         /* Handle Classic CAN frame */
> >
> > I see this as a big pitfall.
> >
> >
> >
> > A solution (or rather a compromise) would be to align the flags fields
> > between the canfd_frame and canxl_frame.
> >
> > #define CANXL_XLF 0x08 /* mandatory CAN XL frame flag (must always be set!) */
> > #define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
> >
> > struct canxl_frame {
> >         canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >         __u8    sdt;   /* SDU (service data unit) type */
> >         __u8    flags; /* flags for CAN FD and CAN XL */
> >         __u16   len;   /* frame payload length in byte */
> >         __u32   af;    /* acceptance field */
> >         __u8    data[CANXL_MAX_DLEN];
> > };
> >
> > This way, the check between CANFD_FDF and CANXL_XLF can be done in any
> > order. A side benefit is that you do not need a union anymore to check
> > the frame type.
>
> This has no impact on the union. When you write code that is able to
> cope with all three CAN frame types you need this kind of union, right?

I am thinking of this:

void handle_cxl(canxl_frame *cxl)
{
    /* ... */
}

void handle_cfd(canfd_frame *cfd)
{
    /* ... */
}

void handle_cc(can_frame *cc)
{
    /* ... */
}

int main()
{
    /* ... */

    if (cxl.flags & CANXL_XLF)
        handle_cxl(&cxl);
    else if (cxl.flags & CANFD_FDF)
        handle_cfd((canfd_frame *)&cxl);
    else
        handle_cc((can_frame *)&cxl);

    /* ... */
}

No union needed.

> > I say this is a compromise because it adds back the collision between
> > can{fd,xl}_frame::flags and can_frame::__pad.
> >
> > Existing applications are exempted from this risk: the
> > CAN_RAW_XL_FRAMES socket option will prevent the collision from
> > happening. To summarise, the collision will only occur if below
> > criteria are met:
> >
> >    * The user allocates a can_frame without zeroing it (typical use
> > cases are stack declaration or malloc()).
> >    * The user activates the CAN_RAW_XL_FRAMES socket option.
> >    * Garbage value results in can_frame::__pad & CANXL_XLF being set.
> >
> > So if going in that direction, we would have to update the
> > documentation (header files and kernel doc) to point at this pitfall
> > and specify that can_frame::__pad shall always be zeroed for mix
> > usages.
>
> But you would add this recommendation to a *very late point* - namely
> after 14 years of SocketCAN in the mainline kernel.
>
> If you think about the rule "never break user space" I wonder if the
> risk turns out to be higher when people enable CAN_RAW_XL_FRAMES ("as
> seen on Stackoverflow") and not read the kernel doc recommendations to
> review/update their existing code.

In which aspect does this break the user space? It doesn't.
Enabling CAN_RAW_XL_FRAMES marks a clear transition between the 14
years old Classical CAN API and the new use case of mixed CAN(-FD) and
CAN XL frames.
For the issue to occur, developers have to incorrectly modify their
application, I do not call this a break in user space if only the
yet-to-be-created programs are impacted.

The argument that people do not read the kernel doc apply to both
cases: some people will not initialise variables, some people will not
do the "if" checks in the right order.

> With the current layout it can be clearly stated that after enabling
> CAN_RAW_XL_FRAMES you will be able to detect CAN XL frames with
>
> if (can.xl.flags & CANXL_XLF) {}
>
> And if this bit is not set you can check for the old stuff (CAN FD and
> Classical CAN).

What makes you think that people will fully understand this and not misuse it?

> The good thing about having can[fd]_frame::len and canxl_frame::flags at
> the same location is that this field is (and was!) *always* set (==
> initialized) properly. No potential risks with existing implementations
> that probably did not initialize reserved or padding bytes.

Yes, I understand this. One more time, I was the first on the mailing
to point out that the 0x80 flag can be used to discriminate between
CAN XL and CAN(-FD).

> And the fact that CANXL_XLF with its value of 128 intentionally breaks
> the CAN frame length rules for CAN/CANFD can be used too.
>
> E.g. in include/linux/can/skb.h
>
> static inline bool can_is_can_skb(const struct sk_buff *skb)
> {
>          struct can_frame *cf = (struct can_frame *)skb->data;
>
>          /* the CAN specific type of skb is identified by its data length */
>          return (skb->len == CAN_MTU && cf->len <= CAN_MAX_DLEN);
> }
>
> static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> {
>          struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>
>          /* the CAN specific type of skb is identified by its data length */
>          return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
> }
>
> Using this simple and efficient check makes it comparably easy to detect
> CAN/CANFD/CANXL content inside the Linux kernel (and it can also be used
> in user space).

But it will not be exposed in the UAPI.

The point isn't whether or not this works: it does, no need to
convince me on it. The point is how easy it is to understand and how
many people will not get it and thus misuse it by putting the
CANFD_FDF check before the CANXL_XLF check?

Yours sincerely,
Vincent Mailhol

> > Personally, I prefer to have the can{fd,xl}_frame::flags aligned.
>
> Summing up I see much more risks of uninitialized content and potential
> misuse from existing code when the can{fd,xl}_frame::flags were aligned
> according to your suggestion.
>
> I would prefer having can[fd]_frame::len and canxl_frame::flags at the
> same (always initialized) location for the reasons described above.
>
> Best regards,
> Oliver
>
>
> > I foresee that misuses due to bad "if" ordering are more likely to
> > happen. Zeroing your stack variable is a best practice, developers
> > with good hygiene are safe. Requiring the two "if" checks in the
> > correct order is more obscure and more luckily to be misused even with
> > best intentions.
> >
> > So here are two solutions, each of them has their benefits and
> > drawbacks. I vote for aligning the flags. Pick your poison.
> >
> >
> >>   #define CAN_MTU                (sizeof(struct can_frame))
> >>   #define CANFD_MTU      (sizeof(struct canfd_frame))
> >> +#define CANXL_MTU      (sizeof(struct canxl_frame))
> >> +#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
> >> +#define CANXL_MIN_MTU  (CANXL_HDR_SIZE + 64)
> >> +#define CANXL_MAX_MTU  CANXL_MTU
> >>
> >>   /* particular protocols of the protocol family PF_CAN */
> >>   #define CAN_RAW                1 /* RAW sockets */
> >>   #define CAN_BCM                2 /* Broadcast Manager */
> >>   #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */

Re: [PATCH v7 4/7] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 30.07.22 15:16, Vincent Mailhol wrote:
> On Sat. 30 juil. 2022 at 21:06, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>>> Existing applications are exempted from this risk: the
>>> CAN_RAW_XL_FRAMES socket option will prevent the collision from
>>> happening. To summarise, the collision will only occur if below
>>> criteria are met:
>>>
>>>     * The user allocates a can_frame without zeroing it (typical use
>>> cases are stack declaration or malloc()).
>>>     * The user activates the CAN_RAW_XL_FRAMES socket option.
>>>     * Garbage value results in can_frame::__pad & CANXL_XLF being set.
>>>
>>> So if going in that direction, we would have to update the
>>> documentation (header files and kernel doc) to point at this pitfall
>>> and specify that can_frame::__pad shall always be zeroed for mix
>>> usages.
>>
>> But you would add this recommendation to a *very late point* - namely
>> after 14 years of SocketCAN in the mainline kernel.
>>
>> If you think about the rule "never break user space" I wonder if the
>> risk turns out to be higher when people enable CAN_RAW_XL_FRAMES ("as
>> seen on Stackoverflow") and not read the kernel doc recommendations to
>> review/update their existing code.
> 
> In which aspect does this break the user space? It doesn't.

It does in the way that you need to zero-initialize structures now.

> Enabling CAN_RAW_XL_FRAMES marks a clear transition between the 14
> years old Classical CAN API and the new use case of mixed CAN(-FD) and
> CAN XL frames.

You are seriously telling me that CAN_RAW_XL_FRAMES is a clear 
transition and people will automatically review and rework their former 
CAN/CANFD code to zero-initialize padding bytes?

There is a much higher risk that this does not happen than telling 
people to check for CANXL_XLF first after reading from the socket!

> For the issue to occur, developers have to incorrectly modify their
> application, I do not call this a break in user space if only the
> yet-to-be-created programs are impacted.

I don't know about your thresholds but I call this a break in user 
space. Yet-to-be-created programs will likely copy old code.

> The argument that people do not read the kernel doc apply to both
> cases: some people will not initialise variables, some people will not
> do the "if" checks in the right order.

To make it simple: The difference between CAN XL and (old) CAN/CANFD 
handling is CANXL_XLF.

I'm really tired about your constructed case that people would be too 
dumb to check for CANXL_XLF first.

This CANFD_FDF check is a new optional(!) possibility, which we do not 
need to promote. This could become a power user trick for people that 
are obviously "smart enough" to put if-statements in the right order.

The correct and safe check that should be documented and suggested looks 
like this:

if (can.xl.flags & CANXL_XLF)
      /* Handle CAN XL frame */
else if (nbytes == CANFD_MTU)
      /* Handle CAN FD frame */
else if (nbytes == CAN_MTU) /* paranoid */
      /* Handle Classic CAN frame */
else
      /* error */

Regards,
Oliver

Re: [PATCH v7 4/7] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Sun. 31 Jul. 2022 at 06:05, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 30.07.22 15:16, Vincent Mailhol wrote:
> > On Sat. 30 juil. 2022 at 21:06, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> >>> Existing applications are exempted from this risk: the
> >>> CAN_RAW_XL_FRAMES socket option will prevent the collision from
> >>> happening. To summarise, the collision will only occur if below
> >>> criteria are met:
> >>>
> >>>     * The user allocates a can_frame without zeroing it (typical use
> >>> cases are stack declaration or malloc()).
> >>>     * The user activates the CAN_RAW_XL_FRAMES socket option.
> >>>     * Garbage value results in can_frame::__pad & CANXL_XLF being set.
> >>>
> >>> So if going in that direction, we would have to update the
> >>> documentation (header files and kernel doc) to point at this pitfall
> >>> and specify that can_frame::__pad shall always be zeroed for mix
> >>> usages.
> >>
> >> But you would add this recommendation to a *very late point* - namely
> >> after 14 years of SocketCAN in the mainline kernel.
> >>
> >> If you think about the rule "never break user space" I wonder if the
> >> risk turns out to be higher when people enable CAN_RAW_XL_FRAMES ("as
> >> seen on Stackoverflow") and not read the kernel doc recommendations to
> >> review/update their existing code.
> >
> > In which aspect does this break the user space? It doesn't.
>
> It does in the way that you need to zero-initialize structures now.
>
> > Enabling CAN_RAW_XL_FRAMES marks a clear transition between the 14
> > years old Classical CAN API and the new use case of mixed CAN(-FD) and
> > CAN XL frames.
>
> You are seriously telling me that CAN_RAW_XL_FRAMES is a clear
> transition and people will automatically review and rework their former
> CAN/CANFD code to zero-initialize padding bytes?
>
> There is a much higher risk that this does not happen than telling
> people to check for CANXL_XLF first after reading from the socket!
>
> > For the issue to occur, developers have to incorrectly modify their
> > application, I do not call this a break in user space if only the
> > yet-to-be-created programs are impacted.
>
> I don't know about your thresholds but I call this a break in user
> space. Yet-to-be-created programs will likely copy old code.
>
> > The argument that people do not read the kernel doc apply to both
> > cases: some people will not initialise variables, some people will not
> > do the "if" checks in the right order.
>
> To make it simple: The difference between CAN XL and (old) CAN/CANFD
> handling is CANXL_XLF.
>
> I'm really tired about your constructed case that people would be too
> dumb to check for CANXL_XLF first.
>
> This CANFD_FDF check is a new optional(!) possibility, which we do not
> need to promote. This could become a power user trick for people that
> are obviously "smart enough" to put if-statements in the right order.
>
> The correct and safe check that should be documented and suggested looks
> like this:
>
> if (can.xl.flags & CANXL_XLF)
>       /* Handle CAN XL frame */
> else if (nbytes == CANFD_MTU)
>       /* Handle CAN FD frame */
> else if (nbytes == CAN_MTU) /* paranoid */
>       /* Handle Classic CAN frame */
> else
>       /* error */

After a good night of sleep, I found a use case which destroys my idea:

int main()
{
    static union can can;

    can.xl.flags = CANXL_XLF;
    /* set other fields, open socket... */
    write(s, &can.xl, CANXL_HD_SIZE + can.xl.len);

    can.cc.len = 8;
    /* set other field but leave can.cc._pad untouched */
    write(s, &can.cc, sizeof(can.cc));

    /* ... */
}

Here, we have a static declaration so the frame is guaranteed to be
zero initialized. The user then sends a CAN XL frame (and thus sets
the CANXL_XLF flag). Then the user sends a Classic CAN frame. However,
can.cc._pad still contains the CANXL_XLF flag set. I stay in my
position that asking to zero initialize variables for mix usage is
acceptable. But requiring to reset the can.cc._pad to zero in the
middle of the run time is too much.
This is a very bad foot gun. So let me withdraw my idea. Using the
0x80 flag on the can(fd)_frame.len is the best.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v7 4/7] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 31.07.22 05:19, Vincent Mailhol wrote:
> On Sun. 31 Jul. 2022 at 06:05, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 30.07.22 15:16, Vincent Mailhol wrote:
>>> On Sat. 30 juil. 2022 at 21:06, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>
>>>>> Existing applications are exempted from this risk: the
>>>>> CAN_RAW_XL_FRAMES socket option will prevent the collision from
>>>>> happening. To summarise, the collision will only occur if below
>>>>> criteria are met:
>>>>>
>>>>>      * The user allocates a can_frame without zeroing it (typical use
>>>>> cases are stack declaration or malloc()).
>>>>>      * The user activates the CAN_RAW_XL_FRAMES socket option.
>>>>>      * Garbage value results in can_frame::__pad & CANXL_XLF being set.
>>>>>
>>>>> So if going in that direction, we would have to update the
>>>>> documentation (header files and kernel doc) to point at this pitfall
>>>>> and specify that can_frame::__pad shall always be zeroed for mix
>>>>> usages.
>>>>
>>>> But you would add this recommendation to a *very late point* - namely
>>>> after 14 years of SocketCAN in the mainline kernel.
>>>>
>>>> If you think about the rule "never break user space" I wonder if the
>>>> risk turns out to be higher when people enable CAN_RAW_XL_FRAMES ("as
>>>> seen on Stackoverflow") and not read the kernel doc recommendations to
>>>> review/update their existing code.
>>>
>>> In which aspect does this break the user space? It doesn't.
>>
>> It does in the way that you need to zero-initialize structures now.
>>
>>> Enabling CAN_RAW_XL_FRAMES marks a clear transition between the 14
>>> years old Classical CAN API and the new use case of mixed CAN(-FD) and
>>> CAN XL frames.
>>
>> You are seriously telling me that CAN_RAW_XL_FRAMES is a clear
>> transition and people will automatically review and rework their former
>> CAN/CANFD code to zero-initialize padding bytes?
>>
>> There is a much higher risk that this does not happen than telling
>> people to check for CANXL_XLF first after reading from the socket!
>>
>>> For the issue to occur, developers have to incorrectly modify their
>>> application, I do not call this a break in user space if only the
>>> yet-to-be-created programs are impacted.
>>
>> I don't know about your thresholds but I call this a break in user
>> space. Yet-to-be-created programs will likely copy old code.
>>
>>> The argument that people do not read the kernel doc apply to both
>>> cases: some people will not initialise variables, some people will not
>>> do the "if" checks in the right order.
>>
>> To make it simple: The difference between CAN XL and (old) CAN/CANFD
>> handling is CANXL_XLF.
>>
>> I'm really tired about your constructed case that people would be too
>> dumb to check for CANXL_XLF first.
>>
>> This CANFD_FDF check is a new optional(!) possibility, which we do not
>> need to promote. This could become a power user trick for people that
>> are obviously "smart enough" to put if-statements in the right order.
>>
>> The correct and safe check that should be documented and suggested looks
>> like this:
>>
>> if (can.xl.flags & CANXL_XLF)
>>        /* Handle CAN XL frame */
>> else if (nbytes == CANFD_MTU)
>>        /* Handle CAN FD frame */
>> else if (nbytes == CAN_MTU) /* paranoid */
>>        /* Handle Classic CAN frame */
>> else
>>        /* error */
> 
> After a good night of sleep, I found a use case which destroys my idea:
> 
> int main()
> {
>      static union can can;
> 
>      can.xl.flags = CANXL_XLF;
>      /* set other fields, open socket... */
>      write(s, &can.xl, CANXL_HD_SIZE + can.xl.len);
> 
>      can.cc.len = 8;
>      /* set other field but leave can.cc._pad untouched */
>      write(s, &can.cc, sizeof(can.cc));
> 
>      /* ... */
> }
> 
> Here, we have a static declaration so the frame is guaranteed to be
> zero initialized. The user then sends a CAN XL frame (and thus sets
> the CANXL_XLF flag). Then the user sends a Classic CAN frame. However,
> can.cc._pad still contains the CANXL_XLF flag set. I stay in my
> position that asking to zero initialize variables for mix usage is
> acceptable. But requiring to reset the can.cc._pad to zero in the
> middle of the run time is too much.
> This is a very bad foot gun. So let me withdraw my idea. Using the
> 0x80 flag on the can(fd)_frame.len is the best.

Thanks.

Best regards,
Oliver