can: support CAN XL

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 | 51 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

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

index 7b23eeeb3273..dd645ea72306 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,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)
+ *

include/uapi/linux/can.h +4 -1

 #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

Quoted 3 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.

        ^^^^^^^

inside

Quoted 106 lines

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 | 51 ++++++++++++++++++++++++++++++++++++++++
  1 file changed, 51 insertions(+)

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

index 7b23eeeb3273..dd645ea72306 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,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)
+ *

include/uapi/linux/can.h struct canfd_frame { · line 214

Quoted 10 lines

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

+ * @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.

Quoted 14 lines

include/uapi/linux/can.h +4 -1

  #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

Hi Vincent,

Quoted 6 lines · Vincent Mailhol

Quoted 3 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.

        ^^^^^^^

inside

ok. Thx.

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

Quoted 139 lines

Quoted 106 lines

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 | 51 ++++++++++++++++++++++++++++++++++++++++
  1 file changed, 51 insertions(+)

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

index 7b23eeeb3273..dd645ea72306 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,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)
+ *

include/uapi/linux/can.h struct canfd_frame { · line 214

Quoted 10 lines

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

+ * @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 */

Quoted 3 lines

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.

Quoted 3 lines

     /* can.fd.flags can collide with can.xl.sdt,  */
     /* nbytes == CANFD_MTU would also collide if the CAN XL frame

length is 32 bytes */

Quoted 24 lines

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];
};

Quoted 3 lines

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?

Quoted 13 lines

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.

Quoted 4 lines

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

Quoted 1 line

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

Quoted 21 lines

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.

Quoted 11 lines

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

  #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 */

Quoted 18 lines · Oliver Hartkopp

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 20 lines · Vincent Mailhol

Quoted 18 lines · Oliver Hartkopp

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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!

Quoted 3 lines

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.

Quoted 3 lines

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 */

Quoted 65 lines · Oliver Hartkopp

Quoted 20 lines · Vincent Mailhol

Quoted 18 lines · Oliver Hartkopp

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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!

Quoted 3 lines

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.

Quoted 3 lines

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:

Quoted 8 lines

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.