can: support CAN XL

RFC PATCH Superseded Oliver Hartkopp 3 participants 17 messages Jul 14, 2022 – Jul 17, 2022

Subsystems (3)

Network drivers CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2022-07-14 16:06 UTC #0 lore raw

v2 cover can: support CAN XL

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

Oliver Hartkopp (5):
can: canxl: introduce CAN XL data structure
can: canxl: introduce ETH_P_CANXL ethernet protocol handling
can: dev: add CAN XL support
can: vcan: add CAN XL support
can: raw: add CAN XL support

 drivers/net/can/dev/rx-offload.c |  2 +-
 drivers/net/can/dev/skb.c        | 55 ++++++++++++++++++++++++++------
 drivers/net/can/vcan.c           | 11 +++----
 include/linux/can/skb.h          | 45 +++++++++++++++++++++++++-
 include/uapi/linux/can.h         | 50 +++++++++++++++++++++++++++++
 include/uapi/linux/can/raw.h     |  1 +
 include/uapi/linux/if_ether.h    |  1 +
 net/can/af_can.c                 | 36 +++++++++++++++++----
 net/can/raw.c                    | 27 +++++++++++++++-
 9 files changed, 203 insertions(+), 25 deletions(-)

Signature · 1 line

2.30.2

Oliver Hartkopp

author maintainer

2022-07-14 16:06 UTC #1 lore raw

RFC PATCH v2 1/5 can: canxl: introduce CAN XL data structure

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 <[email protected]>

---
 include/uapi/linux/can.h | 50 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

include/uapi/linux/can.h +50 -0

index 90801ada2bbe..d5c99ead8df0 100644
@@ -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,49 @@ 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 (1 .. CANXL_MAX_DLEN)
+ * @af:    acceptance field
+ * @data:  CAN XL frame payload (up to 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_HEAD_SZ	(offsetof(struct canxl_frame, data))
+#define CANXL_MINTU	(CANXL_HEAD_SZ + CANXL_MIN_DLEN)
 
 /* 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 */

Signature · 1 line

2.30.2

Marc Kleine-Budde

maintainer

2022-07-14 19:37 UTC #2 lore raw

Re: [RFC PATCH v2 1/5] can: canxl: introduce CAN XL data structure

Quoted 6 lines · 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:

Quoted 7 lines

- 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)

Where do we put the ADS bit (Arbitration Data Sequence Bit Rate Switching from Arbitration to Data Phase)?

Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Oliver Hartkopp

author maintainer

2022-07-14 20:27 UTC #3 lore raw

Re: [RFC PATCH v2 1/5] can: canxl: introduce CAN XL data structure

Quoted 15 lines · Marc Kleine-Budde

Quoted 6 lines · 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:
Quoted 7 lines
- 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)

Quoted 2 lines

Where do we put the ADS bit (Arbitration Data Sequence Bit Rate Switching from Arbitration to Data Phase)?

ADS (arbitration to data sequence)

DAS (data to arbitration sequence)

These sections inside the CAN XL frame bitstream are used to switch the bitrate and the transceiver mode (when a switchable CAN XL transceiver is attached).

AFAICS there is no 'per frame' switching of the data bitrate as we know it from CAN FD.

The bitrate and transceiver settings are done once in the CAN controller setup.

Best,
Oliver

Oliver Hartkopp

author maintainer

2022-07-14 16:06 UTC #4 lore raw

RFC PATCH v2 2/5 can: canxl: introduce ETH_P_CANXL ethernet protocol handling

In reply to Oliver Hartkopp

Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL frame data structure.

As the length information is now a uint16 value for CAN XL three new helper functions have been introduced to retrieve the data length from all types of CAN frames.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 include/linux/can/skb.h       | 39 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/if_ether.h |  1 +
 net/can/af_can.c              | 36 +++++++++++++++++++++++++-------
 3 files changed, 69 insertions(+), 7 deletions(-)

include/linux/can/skb.h +39 -0

index 182749e858b3..87698eac2f15 100644
@@ -101,6 +101,45 @@ 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;
 }
 
+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_MINTU || skb->len > CANXL_MTU)
+		return false;
+
+	/* this also checks valid CAN XL data length boundaries */
+	if (skb->len != CANXL_HEAD_SZ + 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 of 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 (skb->len == CAN_MTU && cf->can_id & CAN_RTR_FLAG)
+		return 0;
+
+	return len;
+}
+
 #endif /* !_CAN_SKB_H */

include/uapi/linux/if_ether.h +1 -0

index d370165bc621..69e0457eb200 100644
@@ -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 ([email protected])	*/
 #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
 #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/

net/can/af_can.c +29 -7

index 1fb49d51b25d..979b1c481dbb 100644
@@ -199,29 +199,30 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
 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;
+	unsigned int mtu = skb->len;
 	int err = -EINVAL;
 
-	if (skb->len == CAN_MTU) {
+	if (can_is_canxl_skb(skb)) {
+		skb->protocol = htons(ETH_P_CANXL);
+		mtu = CANXL_MTU;
+	} else if (mtu == CAN_MTU) {
 		skb->protocol = htons(ETH_P_CAN);
 		if (unlikely(cfd->len > CAN_MAX_DLEN))
 			goto inval_skb;
-	} else if (skb->len == CANFD_MTU) {
+	} else if (mtu == CANFD_MTU) {
 		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(mtu > skb->dev->mtu)) {
 		err = -EMSGSIZE;
 		goto inval_skb;
 	}
 
 	if (unlikely(skb->dev->type != ARPHRD_CAN)) {

@@ -725,10 +726,25 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 free_skb:
 	kfree_skb(skb);
 	return NET_RX_DROP;
 }
 
+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

@@ -849,10 +865,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,
 };

@@ -888,10 +909,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);

Signature · 1 line

2.30.2

Oliver Hartkopp

author maintainer

2022-07-14 16:06 UTC #5 lore raw

RFC PATCH v2 3/5 can: dev: add CAN XL support

In reply to Oliver Hartkopp

Extend the CAN device driver infrastructure to handle CAN XL frames. This especially addresses the increased data length which is extended to uint16 for CAN XL.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/dev/rx-offload.c |  2 +-
 drivers/net/can/dev/skb.c        | 55 ++++++++++++++++++++++++++------
 include/linux/can/skb.h          |  6 +++-
 3 files changed, 52 insertions(+), 11 deletions(-)

drivers/net/can/dev/rx-offload.c +1 -1

index a32a01c172d4..8505e547e922 100644
@@ -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;

drivers/net/can/dev/skb.c +46 -9

117 lines not on this page · raw message

include/linux/can/skb.h +5 -1

index 87698eac2f15..cf26eddaba55 100644
@@ -18,19 +18,23 @@
 
 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);
 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 datalen);
 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);
 
 /*

Signature · 1 line

2.30.2

Marc Kleine-Budde

maintainer

2022-07-14 20:06 UTC #6 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 116 lines · Oliver Hartkopp

Extend the CAN device driver infrastructure to handle CAN XL frames. This especially addresses the increased data length which is extended to uint16 for CAN XL.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/dev/rx-offload.c |  2 +-
 drivers/net/can/dev/skb.c        | 55 ++++++++++++++++++++++++++------
 include/linux/can/skb.h          |  6 +++-
 3 files changed, 52 insertions(+), 11 deletions(-)

drivers/net/can/dev/rx-offload.c +1 -1

index a32a01c172d4..8505e547e922 100644
@@ -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;

drivers/net/can/dev/skb.c +23 -9

index 8bb62dd864c8..c4283fa680be 100644
@@ -53,11 +53,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	BUG_ON(idx >= priv->echo_skb_max);
 
 	/* check flag whether this packet has to be looped back */
 	if (!(dev->flags & IFF_ECHO) ||
 	    (skb->protocol != htons(ETH_P_CAN) &&
-	     skb->protocol != htons(ETH_P_CANFD))) {
+	     skb->protocol != htons(ETH_P_CANFD) &&
+	     skb->protocol != htons(ETH_P_CANXL))) {
 		kfree_skb(skb);
 		return 0;
 	}
 
 	if (!priv->echo_skb[idx]) {

@@ -86,12 +87,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 +104,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 +136,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;

@@ -244,10 +241,47 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 
 	return skb;
 }
 EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+				struct canxl_frame **cfx,
+				unsigned int datalen)
+{
+	struct sk_buff *skb;
+
+	if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN)
+		goto out_error;
+
+	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+			       CANXL_HEAD_SZ + datalen);
+	if (unlikely(!skb))
+		goto out_error;
+
+	skb->protocol = htons(ETH_P_CANXL);
+	skb->pkt_type = PACKET_BROADCAST;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;

drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 272

Quoted 10 lines

drivers/net/can/dev/skb.c +10 -0

+
+	skb_reset_mac_header(skb);
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
+
+	can_skb_reserve(skb);
+	can_skb_prv(skb)->ifindex = dev->ifindex;
+	can_skb_prv(skb)->skbcnt = 0;
+
+	*cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);

Should the CANXL_XLF be set here?

I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data.....

For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length:

https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216

Maybe we should talk to Kees Cook what's best to use here.

Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Oliver Hartkopp

author maintainer

2022-07-14 20:59 UTC #7 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 1 line · Marc Kleine-Budde

On 14.07.2022 18:05:39, Oliver Hartkopp wrote:

(..)

Quoted 21 lines

drivers/net/can/dev/skb.c +21 -0

+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+				struct canxl_frame **cfx,
+				unsigned int datalen)
+{
+	struct sk_buff *skb;
+
+	if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN)
+		goto out_error;
+
+	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+			       CANXL_HEAD_SZ + datalen);
+	if (unlikely(!skb))
+		goto out_error;
+
+	skb->protocol = htons(ETH_P_CANXL);
+	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);

Quoted 8 lines

drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 272
Quoted 6 lines
drivers/net/can/dev/skb.c +6 -0
+
+	can_skb_reserve(skb);
+	can_skb_prv(skb)->ifindex = dev->ifindex;
+	can_skb_prv(skb)->skbcnt = 0;
+
+	*cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);
Should the CANXL_XLF be set here?

Yes, we can set that bit here directly - for convenience reasons ;-)

Quoted 9 lines

I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data.....
For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216
Maybe we should talk to Kees Cook what's best to use here.

I see this struct canxl_frame with 2048 byte of data more as a user space thing.

You can simply read() from the CAN_RAW socket into this struct (as you know it from CAN/CANFD) and it works safely.

That we optimize the length to the really needed length inside the skb and for CAN XL socket read/write operations is on another page for me.

If we *only* had the canxl data structure inside the kernel I would be definitely ok with flexible arrays. The current implementation indeed never allocates space with the sizeof(struct canxl_frame) ...

But I tend to maintain the pattern we introduced for CAN and CAN FD for the user space visible data structures. That is clearer and safe to use by default instead of reading documentation about flexible arrays and how to build some data structure on your own.

Regards,
Oliver

Vincent Mailhol

2022-07-15 03:53 UTC #8 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 5 lines · Oliver Hartkopp

Quoted 1 line · Marc Kleine-Budde
On 14.07.2022 18:05:39, Oliver Hartkopp wrote:
(..)

Quoted 11 lines

drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 256

Quoted 11 lines

drivers/net/can/dev/skb.c +11 -0

+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+                            struct canxl_frame **cfx,
+                            unsigned int datalen)
+{
+    struct sk_buff *skb;
+
+    if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN)
+            goto out_error;
+
+    skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+                           CANXL_HEAD_SZ + datalen);

If usings the flexible array member, this would become:

skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
sizeof(struct canxl_frame) + datalen);

or even:

skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
struct_size(*cxl, data, datalen));

This is an illustration of my point that flexible data arrays are more idiomatic. I find it weird to have to mix sizeof(struct can_skb_priv) and CANXL_HEAD_SZ in the same expression...

Quoted 19 lines

Quoted 10 lines

drivers/net/can/dev/skb.c +10 -0

+    if (unlikely(!skb))
+            goto out_error;
+
+    skb->protocol = htons(ETH_P_CANXL);
+    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);

Quoted 8 lines

drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 272
Quoted 6 lines
drivers/net/can/dev/skb.c +6 -0
+
+    can_skb_reserve(skb);
+    can_skb_prv(skb)->ifindex = dev->ifindex;
+    can_skb_prv(skb)->skbcnt = 0;
+
+    *cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);
Should the CANXL_XLF be set here?

Quoted 4 lines

Yes, we can set that bit here directly - for convenience reasons ;-)
Quoted 2 lines
I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data.....

I tried to think hard of what could go wrong with the data[CANXL_MAX_DLEN] declaration.

The worst I could think of would be some:

        int datalen = 64; /* or anything less than CANXL_MAX_DLEN */
        struct canxl_frame *cxl1 = malloc(CANXL_HEAD_SZ + datalen);
        struct canxl_frame *cxl2 = malloc(CANXL_HEAD_SZ + datalen);

        memcpy(cxl1, cxl2, sizeof(*cxl1));

But that example is a bit convoluted. That's why I wrote in my previous message that I saw no killer arguments against it.

Quoted 21 lines

Quoted 6 lines
For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216
Maybe we should talk to Kees Cook what's best to use here.
I see this struct canxl_frame with 2048 byte of data more as a user space thing.
You can simply read() from the CAN_RAW socket into this struct (as you know it from CAN/CANFD) and it works safely.
That we optimize the length to the really needed length inside the skb and for CAN XL socket read/write operations is on another page for me.
If we *only* had the canxl data structure inside the kernel I would be definitely ok with flexible arrays.
The current implementation indeed never allocates space with the sizeof(struct canxl_frame) ...

Quoted 4 lines

But I tend to maintain the pattern we introduced for CAN and CAN FD for the user space visible data structures. That is clearer and safe to use by default instead of reading documentation about flexible arrays and how to build some data structure on your own.

Here, you are making the assumption that the end user will only be familiar with the CAN(-FD) and not with other concepts.

Building data structures on your own is fairly common, the best example being the struct iphdr or the struct tcphdr for TCP/IP:
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L86
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/tcp.h#L25
(in those examples, it is not a flexible array member but the concept is basically the same).

I think it is fair to expect from a developer using Berkeley sockets (what SocketCAN is) to be familiar with this.

In the worst case, the developper who still completely ignore the documentation and just do sed "s/canfd/canxl/g" on their existing code base will eventually do this:

        write(sock, &cxl, sizeof(canxl));

And that would fail immediately (because sizeof(canxl) < CANXL_MIN_TU). So I think it is still safe. The real foot gun is when you can write incorrect code that still works (e.g. buffer overflow). If it directly fails, people will copy/paste the accepted answer on stackoverflow and will eventually do the correct:

        write(sock, &cxl, sizeof(cxl) + cxl.len);

Finally, for both solutions, user can not do this anymore:

        assert(read(sock, &cxl, sizeof(cxl)) == sizeof(cxl));
But instead should do:
        assert(read(sock, &cxl, sizeof(cxl)) >= CANXL_MINTU);

So regardless of the solution we use, the developer needs to be aware to some extent of the variable size (and ignoring the return value of read() is a bad practice so I won't accept this as a counterargument).

The debate is really on "reusing CAN(-FD) patterns" vs. "doing idiomatic C". I will not veto the data[CANXL_MAX_DLEN], but I vote for the flexible array member for the reasons listed here.

Yours sincerely,
Vincent Mailhol

Oliver Hartkopp

author maintainer

2022-07-15 06:51 UTC #9 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 98 lines · Vincent Mailhol

Quoted 5 lines · Oliver Hartkopp
Quoted 1 line · Marc Kleine-Budde
On 14.07.2022 18:05:39, Oliver Hartkopp wrote:
(..)
Quoted 11 lines
Quoted 11 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 256
Quoted 11 lines
drivers/net/can/dev/skb.c +11 -0
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+                            struct canxl_frame **cfx,
+                            unsigned int datalen)
+{
+    struct sk_buff *skb;
+
+    if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN)
+            goto out_error;
+
+    skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+                           CANXL_HEAD_SZ + datalen);
If usings the flexible array member, this would become:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
sizeof(struct canxl_frame) + datalen);
or even:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
struct_size(*cxl, data, datalen));
This is an illustration of my point that flexible data arrays are more idiomatic. I find it weird to have to mix sizeof(struct can_skb_priv) and CANXL_HEAD_SZ in the same expression...
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/net/can/dev/skb.c +10 -0
+    if (unlikely(!skb))
+            goto out_error;
+
+    skb->protocol = htons(ETH_P_CANXL);
+    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);
Quoted 8 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 272
Quoted 6 lines
drivers/net/can/dev/skb.c +6 -0
+
+    can_skb_reserve(skb);
+    can_skb_prv(skb)->ifindex = dev->ifindex;
+    can_skb_prv(skb)->skbcnt = 0;
+
+    *cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);
Should the CANXL_XLF be set here?
Quoted 4 lines
Yes, we can set that bit here directly - for convenience reasons ;-)
Quoted 2 lines
I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data.....
I tried to think hard of what could go wrong with the data[CANXL_MAX_DLEN] declaration.
The worst I could think of would be some:
         int datalen = 64; /* or anything less than CANXL_MAX_DLEN */
         struct canxl_frame *cxl1 = malloc(CANXL_HEAD_SZ + datalen);
         struct canxl_frame *cxl2 = malloc(CANXL_HEAD_SZ + datalen);

         memcpy(cxl1, cxl2, sizeof(*cxl1));
But that example is a bit convoluted. That's why I wrote in my previous message that I saw no killer arguments against it.
Quoted 21 lines
Quoted 6 lines
For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216
Maybe we should talk to Kees Cook what's best to use here.
I see this struct canxl_frame with 2048 byte of data more as a user space thing.
You can simply read() from the CAN_RAW socket into this struct (as you know it from CAN/CANFD) and it works safely.
That we optimize the length to the really needed length inside the skb and for CAN XL socket read/write operations is on another page for me.
If we *only* had the canxl data structure inside the kernel I would be definitely ok with flexible arrays.
The current implementation indeed never allocates space with the sizeof(struct canxl_frame) ...
Quoted 4 lines
But I tend to maintain the pattern we introduced for CAN and CAN FD for the user space visible data structures. That is clearer and safe to use by default instead of reading documentation about flexible arrays and how to build some data structure on your own.
Here, you are making the assumption that the end user will only be familiar with the CAN(-FD) and not with other concepts.

Quoted 6 lines

Building data structures on your own is fairly common, the best example being the struct iphdr or the struct tcphdr for TCP/IP:
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L86
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/tcp.h#L25
(in those examples, it is not a flexible array member but the concept is basically the same).

But then you would have to name it struct canxlhdr or canxl_hdr to follow this pattern, right?

And this is my other problem. The struct canxl_frame should be able to contain a CAN XL frame (as can[fd]_frame do).

I'm fine with introducing e.g. a

struct canxl_hdr {
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[];
};

and propose the suggested use-patterns.

But I just don't feel good to give up the struct canxl_frame analogue to can[fd]_frame.

Best regards,
Oliver

ps. can we perhaps put canxl_frame and canxl_hdr in some union structure that does not look ugly?

Quoted 29 lines

I think it is fair to expect from a developer using Berkeley sockets (what SocketCAN is) to be familiar with this.
In the worst case, the developper who still completely ignore the documentation and just do sed "s/canfd/canxl/g" on their existing code base will eventually do this:
         write(sock, &cxl, sizeof(canxl));
And that would fail immediately (because sizeof(canxl) < CANXL_MIN_TU). So I think it is still safe. The real foot gun is when you can write incorrect code that still works (e.g. buffer overflow). If it directly fails, people will copy/paste the accepted answer on stackoverflow and will eventually do the correct:
         write(sock, &cxl, sizeof(cxl) + cxl.len);
Finally, for both solutions, user can not do this anymore:
         assert(read(sock, &cxl, sizeof(cxl)) == sizeof(cxl));
But instead should do:
         assert(read(sock, &cxl, sizeof(cxl)) >= CANXL_MINTU);
So regardless of the solution we use, the developer needs to be aware to some extent of the variable size (and ignoring the return value of read() is a bad practice so I won't accept this as a counterargument).
The debate is really on "reusing CAN(-FD) patterns" vs. "doing idiomatic C". I will not veto the data[CANXL_MAX_DLEN], but I vote for the flexible array member for the reasons listed here.
Yours sincerely,
Vincent Mailhol

Vincent Mailhol

2022-07-15 08:28 UTC #10 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 123 lines · Oliver Hartkopp

Quoted 98 lines · Vincent Mailhol
Quoted 5 lines · Oliver Hartkopp
Quoted 1 line · Marc Kleine-Budde
On 14.07.2022 18:05:39, Oliver Hartkopp wrote:
(..)
Quoted 11 lines
Quoted 11 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 256
Quoted 11 lines
drivers/net/can/dev/skb.c +11 -0
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+                            struct canxl_frame **cfx,
+                            unsigned int datalen)
+{
+    struct sk_buff *skb;
+
+    if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN)
+            goto out_error;
+
+    skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+                           CANXL_HEAD_SZ + datalen);
If usings the flexible array member, this would become:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
sizeof(struct canxl_frame) + datalen);
or even:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
struct_size(*cxl, data, datalen));
This is an illustration of my point that flexible data arrays are more idiomatic. I find it weird to have to mix sizeof(struct can_skb_priv) and CANXL_HEAD_SZ in the same expression...
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/net/can/dev/skb.c +10 -0
+    if (unlikely(!skb))
+            goto out_error;
+
+    skb->protocol = htons(ETH_P_CANXL);
+    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);
Quoted 8 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 272
Quoted 6 lines
drivers/net/can/dev/skb.c +6 -0
+
+    can_skb_reserve(skb);
+    can_skb_prv(skb)->ifindex = dev->ifindex;
+    can_skb_prv(skb)->skbcnt = 0;
+
+    *cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);
Should the CANXL_XLF be set here?
Quoted 4 lines
Yes, we can set that bit here directly - for convenience reasons ;-)
Quoted 2 lines
I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data.....
I tried to think hard of what could go wrong with the data[CANXL_MAX_DLEN] declaration.
The worst I could think of would be some:
         int datalen = 64; /* or anything less than CANXL_MAX_DLEN */
         struct canxl_frame *cxl1 = malloc(CANXL_HEAD_SZ + datalen);
         struct canxl_frame *cxl2 = malloc(CANXL_HEAD_SZ + datalen);

         memcpy(cxl1, cxl2, sizeof(*cxl1));
But that example is a bit convoluted. That's why I wrote in my previous message that I saw no killer arguments against it.
Quoted 21 lines
Quoted 6 lines
For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216
Maybe we should talk to Kees Cook what's best to use here.
I see this struct canxl_frame with 2048 byte of data more as a user space thing.
You can simply read() from the CAN_RAW socket into this struct (as you know it from CAN/CANFD) and it works safely.
That we optimize the length to the really needed length inside the skb and for CAN XL socket read/write operations is on another page for me.
If we *only* had the canxl data structure inside the kernel I would be definitely ok with flexible arrays.
The current implementation indeed never allocates space with the sizeof(struct canxl_frame) ...
Quoted 4 lines
But I tend to maintain the pattern we introduced for CAN and CAN FD for the user space visible data structures. That is clearer and safe to use by default instead of reading documentation about flexible arrays and how to build some data structure on your own.
Here, you are making the assumption that the end user will only be familiar with the CAN(-FD) and not with other concepts.
Quoted 6 lines
Building data structures on your own is fairly common, the best example being the struct iphdr or the struct tcphdr for TCP/IP:
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L86
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/tcp.h#L25
(in those examples, it is not a flexible array member but the concept is basically the same).
But then you would have to name it struct canxlhdr or canxl_hdr to follow this pattern, right?
And this is my other problem. The struct canxl_frame should be able to contain a CAN XL frame (as can[fd]_frame do).
I'm fine with introducing e.g. a
struct canxl_hdr {
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[];
};

Quoted 1 line

and propose the suggested use-patterns.

No, you misunderstood my comment. The ipdhr and tcphdr was to illustrate the fact that it is not uncommon to build some data structure on your own. In those two examples, the hdr suffix is used because there is no payload (i.e. no data field in the struct).

In our case, it would be:

struct canxl {
        struct canxl_hdr hdr;
        __u8    data[];
}

Which is equivalent to:

struct canxl {
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[];
};

You can do:

$ git grep -E -B5 -A2 "data\[0\];|data\[\];" include/uapi/
to get some examples of flexible member arrays (those are probably
better examples than the iphdr and tcphdr, I should have started with
that).

NB: data[0] and data[] are basically equivalent with the nuance that data[0] is a GNU C extension whereas data[] is standard C99.

Quoted 38 lines

But I just don't feel good to give up the struct canxl_frame analogue to can[fd]_frame.
Best regards,
Oliver
ps. can we perhaps put canxl_frame and canxl_hdr in some union structure that does not look ugly?
Quoted 29 lines
I think it is fair to expect from a developer using Berkeley sockets (what SocketCAN is) to be familiar with this.
In the worst case, the developper who still completely ignore the documentation and just do sed "s/canfd/canxl/g" on their existing code base will eventually do this:
         write(sock, &cxl, sizeof(canxl));
And that would fail immediately (because sizeof(canxl) < CANXL_MIN_TU). So I think it is still safe. The real foot gun is when you can write incorrect code that still works (e.g. buffer overflow). If it directly fails, people will copy/paste the accepted answer on stackoverflow and will eventually do the correct:
         write(sock, &cxl, sizeof(cxl) + cxl.len);
Finally, for both solutions, user can not do this anymore:
         assert(read(sock, &cxl, sizeof(cxl)) == sizeof(cxl));
But instead should do:
         assert(read(sock, &cxl, sizeof(cxl)) >= CANXL_MINTU);
So regardless of the solution we use, the developer needs to be aware to some extent of the variable size (and ignoring the return value of read() is a bad practice so I won't accept this as a counterargument).
The debate is really on "reusing CAN(-FD) patterns" vs. "doing idiomatic C". I will not veto the data[CANXL_MAX_DLEN], but I vote for the flexible array member for the reasons listed here.
Yours sincerely,
Vincent Mailhol

Oliver Hartkopp

author maintainer

2022-07-17 13:07 UTC #11 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 43 lines · Vincent Mailhol

Quoted 17 lines · Oliver Hartkopp
But then you would have to name it struct canxlhdr or canxl_hdr to follow this pattern, right?
And this is my other problem. The struct canxl_frame should be able to contain a CAN XL frame (as can[fd]_frame do).
I'm fine with introducing e.g. a
struct canxl_hdr {
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[];
};
Quoted 1 line
and propose the suggested use-patterns.
No, you misunderstood my comment. The ipdhr and tcphdr was to illustrate the fact that it is not uncommon to build some data structure on your own. In those two examples, the hdr suffix is used because there is no payload (i.e. no data field in the struct).
In our case, it would be:
struct canxl_hdr {
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 */
};
struct canxl {
         struct canxl_hdr hdr;
         __u8    data[];
}
Which is equivalent to:

Quoted 9 lines

struct canxl {
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[];
};

Unfortunately not really.

You have the same memory layout but you can access the content in a different way:

struct canxl cxl1; (the first one)

cxl1.hdr.flags

struct canxl cxl2; (the second one)

cxl2.flags

:-/

It would be cool to handle and define

struct canxl_hdr

that could be used in

struct canxl_frame (containing data[2048])

and

struct canxl_msg (containing data[])

But this might confuse programmers even more.

Regards,
Oliver

Marc Kleine-Budde

maintainer

2022-07-15 07:37 UTC #12 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

In reply to Vincent Mailhol

Quoted 20 lines · Vincent Mailhol

Quoted 5 lines · Oliver Hartkopp
Quoted 1 line · Marc Kleine-Budde
On 14.07.2022 18:05:39, Oliver Hartkopp wrote:
(..)
Quoted 11 lines
Quoted 11 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 256
Quoted 11 lines
drivers/net/can/dev/skb.c +11 -0
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+                            struct canxl_frame **cfx,
+                            unsigned int datalen)
+{
+    struct sk_buff *skb;
+
+    if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN)
+            goto out_error;
+
+    skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+                           CANXL_HEAD_SZ + datalen);
If usings the flexible array member, this would become:

Quoted 7 lines

skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
sizeof(struct canxl_frame) + datalen);
or even:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
struct_size(*cxl, data, datalen));

ACK. I was thinking of the 2nd option with the struct_size().

Quoted 32 lines

This is an illustration of my point that flexible data arrays are more idiomatic. I find it weird to have to mix sizeof(struct can_skb_priv) and CANXL_HEAD_SZ in the same expression...
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/net/can/dev/skb.c +10 -0
+    if (unlikely(!skb))
+            goto out_error;
+
+    skb->protocol = htons(ETH_P_CANXL);
+    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);
Quoted 8 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 272
Quoted 6 lines
drivers/net/can/dev/skb.c +6 -0
+
+    can_skb_reserve(skb);
+    can_skb_prv(skb)->ifindex = dev->ifindex;
+    can_skb_prv(skb)->skbcnt = 0;
+
+    *cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);
Should the CANXL_XLF be set here?
Quoted 4 lines
Yes, we can set that bit here directly - for convenience reasons ;-)
Quoted 2 lines
I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data.....
I tried to think hard of what could go wrong with the data[CANXL_MAX_DLEN] declaration.
The worst I could think of would be some:

Quoted 8 lines

        int datalen = 64; /* or anything less than CANXL_MAX_DLEN */
        struct canxl_frame *cxl1 = malloc(CANXL_HEAD_SZ + datalen);
        struct canxl_frame *cxl2 = malloc(CANXL_HEAD_SZ + datalen);

        memcpy(cxl1, cxl2, sizeof(*cxl1));
But that example is a bit convoluted. That's why I wrote in my previous message that I saw no killer arguments against it.

It just feels wrong to have a pointer to a struct canxl_frame that's only backed half by memory. I haven't followed the flex array discussion in great detail, but it opens a whole class of errors if arbitrary kernel memory can be accessed with struct canxl_frame::data using seemingly valid array indices.

Quoted 10 lines

Quoted 10 lines
Quoted 6 lines
For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216
Maybe we should talk to Kees Cook what's best to use here.
I see this struct canxl_frame with 2048 byte of data more as a user space thing.

Quoted 8 lines

Quoted 8 lines
You can simply read() from the CAN_RAW socket into this struct (as you know it from CAN/CANFD) and it works safely.
That we optimize the length to the really needed length inside the skb and for CAN XL socket read/write operations is on another page for me.
If we *only* had the canxl data structure inside the kernel I would be definitely ok with flexible arrays.

We don't have to use the same data structure in user space and in the kernel.

Quoted 11 lines

Quoted 7 lines
The current implementation indeed never allocates space with the sizeof(struct canxl_frame) ...
But I tend to maintain the pattern we introduced for CAN and CAN FD for the user space visible data structures. That is clearer and safe to use by default instead of reading documentation about flexible arrays and how to build some data structure on your own.
Here, you are making the assumption that the end user will only be familiar with the CAN(-FD) and not with other concepts.

Quoted 6 lines

Building data structures on your own is fairly common, the best example being the struct iphdr or the struct tcphdr for TCP/IP:
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L86
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/tcp.h#L25
(in those examples, it is not a flexible array member but the concept is basically the same).

struct_size() is not exported to user space, but I think this could be changed.

Quoted 18 lines

I think it is fair to expect from a developer using Berkeley sockets (what SocketCAN is) to be familiar with this.
In the worst case, the developper who still completely ignore the documentation and just do sed "s/canfd/canxl/g" on their existing code base will eventually do this:
        write(sock, &cxl, sizeof(canxl));
And that would fail immediately (because sizeof(canxl) < CANXL_MIN_TU). So I think it is still safe. The real foot gun is when you can write incorrect code that still works (e.g. buffer overflow). If it directly fails, people will copy/paste the accepted answer on stackoverflow and will eventually do the correct:
        write(sock, &cxl, sizeof(cxl) + cxl.len);
Finally, for both solutions, user can not do this anymore:
        assert(read(sock, &cxl, sizeof(cxl)) == sizeof(cxl));
But instead should do:
        assert(read(sock, &cxl, sizeof(cxl)) >= CANXL_MINTU);

Quoted 7 lines

So regardless of the solution we use, the developer needs to be aware to some extent of the variable size (and ignoring the return value of read() is a bad practice so I won't accept this as a counterargument).
The debate is really on "reusing CAN(-FD) patterns" vs. "doing idiomatic C". I will not veto the data[CANXL_MAX_DLEN], but I vote for the flexible array member for the reasons listed here.

How are raw Ethernet frames handled in user space?

Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Vincent Mailhol

2022-07-15 08:48 UTC #13 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 67 lines · Marc Kleine-Budde

Quoted 20 lines · Vincent Mailhol
Quoted 5 lines · Oliver Hartkopp
Quoted 1 line · Marc Kleine-Budde
On 14.07.2022 18:05:39, Oliver Hartkopp wrote:
(..)
Quoted 11 lines
Quoted 11 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 256
Quoted 11 lines
drivers/net/can/dev/skb.c +11 -0
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+                            struct canxl_frame **cfx,
+                            unsigned int datalen)
+{
+    struct sk_buff *skb;
+
+    if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN)
+            goto out_error;
+
+    skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+                           CANXL_HEAD_SZ + datalen);
If usings the flexible array member, this would become:
Quoted 7 lines
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
sizeof(struct canxl_frame) + datalen);
or even:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
struct_size(*cxl, data, datalen));
ACK. I was thinking of the 2nd option with the struct_size().
Quoted 36 lines
This is an illustration of my point that flexible data arrays are more idiomatic. I find it weird to have to mix sizeof(struct can_skb_priv) and CANXL_HEAD_SZ in the same expression...
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/net/can/dev/skb.c +10 -0
+    if (unlikely(!skb))
+            goto out_error;
+
+    skb->protocol = htons(ETH_P_CANXL);
+    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);
Quoted 8 lines
drivers/net/can/dev/skb.c struct sk_buff *alloc_canfd_skb(struct net_device *dev, · line 272
Quoted 6 lines
drivers/net/can/dev/skb.c +6 -0
+
+    can_skb_reserve(skb);
+    can_skb_prv(skb)->ifindex = dev->ifindex;
+    can_skb_prv(skb)->skbcnt = 0;
+
+    *cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);
Should the CANXL_XLF be set here?
Quoted 4 lines
Yes, we can set that bit here directly - for convenience reasons ;-)
Quoted 2 lines
I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data.....
I tried to think hard of what could go wrong with the data[CANXL_MAX_DLEN] declaration.
The worst I could think of would be some:
        int datalen = 64; /* or anything less than CANXL_MAX_DLEN */
        struct canxl_frame *cxl1 = malloc(CANXL_HEAD_SZ + datalen);
        struct canxl_frame *cxl2 = malloc(CANXL_HEAD_SZ + datalen);

Quoted 7 lines

Quoted 4 lines
        memcpy(cxl1, cxl2, sizeof(*cxl1));
But that example is a bit convoluted. That's why I wrote in my previous message that I saw no killer arguments against it.
It just feels wrong to have a pointer to a struct canxl_frame that's only backed half by memory.

I follow you on this feeling. But have no concrete bad examples of how the data[CANXL_MAX_DLEN] could be misused.

Quoted 4 lines

I haven't followed the flex array discussion
in great detail, but it opens a whole class of errors if arbitrary kernel memory can be accessed with struct canxl_frame::data using seemingly valid array indices.

The point in the previous discussion is that the kernel should always crop the returned frames. If implemented correctly, such memory leaks should not occur.

Quoted 40 lines

Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216
Maybe we should talk to Kees Cook what's best to use here.
I see this struct canxl_frame with 2048 byte of data more as a user space thing.
Quoted 8 lines
Quoted 8 lines
You can simply read() from the CAN_RAW socket into this struct (as you know it from CAN/CANFD) and it works safely.
That we optimize the length to the really needed length inside the skb and for CAN XL socket read/write operations is on another page for me.
If we *only* had the canxl data structure inside the kernel I would be definitely ok with flexible arrays.
We don't have to use the same data structure in user space and in the kernel.
Quoted 11 lines
Quoted 7 lines
The current implementation indeed never allocates space with the sizeof(struct canxl_frame) ...
But I tend to maintain the pattern we introduced for CAN and CAN FD for the user space visible data structures. That is clearer and safe to use by default instead of reading documentation about flexible arrays and how to build some data structure on your own.
Here, you are making the assumption that the end user will only be familiar with the CAN(-FD) and not with other concepts.
Quoted 6 lines
Building data structures on your own is fairly common, the best example being the struct iphdr or the struct tcphdr for TCP/IP:
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L86
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/tcp.h#L25
(in those examples, it is not a flexible array member but the concept is basically the same).

Quoted 2 lines

struct_size() is not exported to user space, but I think this could be changed.

Good luck with that!

struct_size() as-is can not be exported as-is because it will break existing programs which already have the same declaration. So we would need to declare a __struct_size(). I would absolutely love to see this exported but I would be surprised if such discussion did not already occured in the past.

Quoted 22 lines

Quoted 22 lines
I think it is fair to expect from a developer using Berkeley sockets (what SocketCAN is) to be familiar with this.
In the worst case, the developper who still completely ignore the documentation and just do sed "s/canfd/canxl/g" on their existing code base will eventually do this:
        write(sock, &cxl, sizeof(canxl));
And that would fail immediately (because sizeof(canxl) < CANXL_MIN_TU). So I think it is still safe. The real foot gun is when you can write incorrect code that still works (e.g. buffer overflow). If it directly fails, people will copy/paste the accepted answer on stackoverflow and will eventually do the correct:
        write(sock, &cxl, sizeof(cxl) + cxl.len);
Finally, for both solutions, user can not do this anymore:
        assert(read(sock, &cxl, sizeof(cxl)) == sizeof(cxl));
But instead should do:
        assert(read(sock, &cxl, sizeof(cxl)) >= CANXL_MINTU);
So regardless of the solution we use, the developer needs to be aware to some extent of the variable size (and ignoring the return value of read() is a bad practice so I won't accept this as a counterargument).

Quoted 5 lines

Quoted 3 lines
The debate is really on "reusing CAN(-FD) patterns" vs. "doing idiomatic C". I will not veto the data[CANXL_MAX_DLEN], but I vote for the flexible array member for the reasons listed here.
How are raw Ethernet frames handled in user space?

The first example of a "iphdr c example" in Google gives me:
https://github.com/joshlong/interesting-native-code-examples/blob/master/packet_sniffer.c#L133

(I am not saying this is the best example, but have a clear example of buffer size manipulation).

Another example of flexible array member:
* kernel definition:
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1163
* userland code:
https://kernel.googlesource.com/pub/scm/network/ethtool/ethtool/+/refs/heads/ethtool-3.4.y/ethtool.c#2989

Yours sincerely,
Vincent Mailhol

Oliver Hartkopp

author maintainer

2022-07-17 13:23 UTC #14 lore raw

Re: [RFC PATCH v2 3/5] can: dev: add CAN XL support

Quoted 11 lines · Vincent Mailhol

Quoted 5 lines · Marc Kleine-Budde
Quoted 2 lines
But that example is a bit convoluted. That's why I wrote in my previous message that I saw no killer arguments against it.
It just feels wrong to have a pointer to a struct canxl_frame that's only backed half by memory.
I follow you on this feeling. But have no concrete bad examples of how the data[CANXL_MAX_DLEN] could be misused.

Quoted 8 lines

Quoted 4 lines
I haven't followed the flex array discussion
in great detail, but it opens a whole class of errors if arbitrary kernel memory can be accessed with struct canxl_frame::data using seemingly valid array indices.
The point in the previous discussion is that the kernel should always crop the returned frames. If implemented correctly, such memory leaks should not occur.

I prepared a V3 patch set for that discussion.

It ended up with the fixed struct canxl_frame analogue to the CAN[fd]_frame's.

So whenever we have an enabled CAN XL netdevice we create a sk_buff in raw.c which can potentially hold a struct canxl_frame.

Even when we send a CAN/CANFD frame to that CAN XL interface.

The distinction inside the kernel remains with skb->len being CAN[|FD|XL]_MTU.

But the CAN_RAW socket read/write operations can be switched to a dynamic (cropped) size or to a complete sizeof(struct canxl_frame) via enhanced sockopt().

As the sk_buffs are only allocated once and are not copied inside the kernel there should be no remarkable drawbacks whether we allocate CAN_MTU skbs or CANXL_MTU skbs.

AFAICS both skbs will fit into a single page.

Best regards,
Oliver

Oliver Hartkopp

author maintainer

2022-07-14 16:06 UTC #15 lore raw

RFC PATCH v2 4/5 can: vcan: add CAN XL support

In reply to Oliver Hartkopp

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/vcan.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/net/can/vcan.c +5 -6

index a15619d883ec..d72c0727b440 100644
@@ -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,11 @@ 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 && new_mtu != CANXL_MTU)
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
 	return 0;
 }

Signature · 1 line

2.30.2

Oliver Hartkopp

author maintainer

2022-07-14 16:06 UTC #16 lore raw

RFC PATCH v2 5/5 can: raw: add CAN XL support

In reply to 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.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 include/uapi/linux/can/raw.h |  1 +
 net/can/raw.c                | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

include/uapi/linux/can/raw.h +1 -0

index 3386aa81fdf2..ff12f525c37c 100644
@@ -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 */

net/can/raw.c +26 -1

index d1bd9cc51ebe..21f48190cdf9 100644
@@ -85,10 +85,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;

@@ -344,10 +345,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 +669,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 +760,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;

@@ -795,14 +812,22 @@ 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 (ro->xl_frames && dev->mtu == CANXL_MTU) {
+		/* CAN XL, CAN FD and Classical CAN */
+		if (unlikely((size < CANXL_MINTU || size > CANXL_MTU) &&
+			     size != CANFD_MTU &&
+			     size != CAN_MTU))
+			goto put_dev;
+	} else if (ro->fd_frames && dev->mtu == CANFD_MTU) {
+		/* CAN FD and Classical CAN */
 		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
 			goto put_dev;
 	} else {
+		/* Classical CAN */
 		if (unlikely(size != CAN_MTU))
 			goto put_dev;
 	}
 
 	skb = sock_alloc_send_skb(sk, size + sizeof(struct can_skb_priv),

Signature · 1 line

2.30.2