[PATCH] canxl: add virtual CAN network identifier support

[PATCH] canxl: add virtual CAN network identifier support

[Oliver Hartkopp]

CAN XL data frames contain an 8-bit virtual CAN network identifier (VCID).
A VCID value of zero represents an 'untagged' CAN XL frame.

To receive and send these optional VCIDs via CAN_RAW sockets a new socket
option CAN_RAW_XL_VCID_OPTS is introduced to define/access VCID content:

- tx: set the outgoing VCID value by the kernel (one fixed 8-bit value)
- tx: pass through VCID values from the user space (e.g. for traffic replay)
- rx: apply VCID receive filter (value/mask) to be passed to the user space

With the 'tx pass through' option CAN_RAW_XL_VCID_TX_PASS all valid VCID
values can be send, e.g. to replay full qualified CAN XL traffic.

The VCID value provided for the CAN_RAW_XL_VCID_TX_SET option will
override the VCID value in the struct canxl_frame.prio defined for
CAN_RAW_XL_VCID_TX_PASS when both flags are set.

With a rx_vcid_mask of zero all possible VCID values (0x00 - 0xFF) are passed
to the user space when the CAN_RAW_XL_VCID_RX_FILTER flag is set. Without this
flag only untagged CAN XL frames (VCID = 0x00, CANXL_VCID flag unset) are
delivered to the user space.

The 8-bit VCID is stored inside the CAN XL prio element (only in CAN XL
frames!) to not interfere with other CAN content or the CAN filters
provided by the CAN_RAW sockets and kernel infrastruture.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/uapi/linux/can.h     | 14 +++++--
 include/uapi/linux/can/raw.h | 14 +++++++
 net/can/raw.c                | 79 ++++++++++++++++++++++++++++++++++--
 3 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 939db2388208..30453905fbd2 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -180,34 +180,40 @@ struct canfd_frame {
 };
 
 /*
  * 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_frame.flags element contains CAN XL frame specific bits 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) */
+#define CANXL_VCID 0x02 /* prio contains the virtual CAN network identifier */
+
+/* the 8-bit VCID is optionally placed in the canxl_frame.prio element */
+#define CANXL_VCID_OFFSET 16 /* bit offset of VCID in prio element */
+#define CANXL_VCID_VAL_MASK 0xFFUL /* VCID is an 8-bit value */
+#define CANXL_VCID_MASK (CANXL_VCID_VAL_MASK << CANXL_VCID_OFFSET)
 
 /**
  * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
- * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
+ * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags / VCID
  * @flags: additional flags for CAN XL
  * @sdt:   SDU (service data unit) type
  * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
  * @af:    acceptance field
  * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
  *
  * @prio shares the same position as @can_id from struct can[fd]_frame.
  */
 struct canxl_frame {
-	canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
+	canid_t prio;  /* 11 bit priority for arbitration / 8 bit VCID */
 	__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];
diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 31622c9b7988..8890b0d2fd48 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -63,8 +63,22 @@ enum {
 	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) */
+	CAN_RAW_XL_VCID_OPTS,	/* CAN XL VCID configuration options */
 };
 
+struct can_raw_vcid_options {
+
+	__u8 flags;		/* flags for vcid (filter) behaviour */
+	__u8 tx_vcid;		/* VCID value set into canxl_frame.prio */
+	__u8 rx_vcid;		/* VCID value for VCID filter */
+	__u8 rx_vcid_mask;	/* VCID mask for VCID filter */
+
+};
+
+#define CAN_RAW_XL_VCID_TX_SET		0x01
+#define CAN_RAW_XL_VCID_TX_PASS		0x02
+#define CAN_RAW_XL_VCID_RX_FILTER	0x04
+
 #endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/raw.c b/net/can/raw.c
index e6b822624ba2..a6f0b6a0a934 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -89,10 +89,14 @@ struct raw_sock {
 	struct list_head notifier;
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
 	int xl_frames;
+	struct can_raw_vcid_options raw_vcid_opts;
+	canid_t tx_vcid_shifted;
+	canid_t rx_vcid_shifted;
+	canid_t rx_vcid_mask_shifted;
 	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;
@@ -132,14 +136,33 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	/* check the received tx sock reference */
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
 	/* make sure to not pass oversized frames to the socket */
-	if ((!ro->fd_frames && can_is_canfd_skb(oskb)) ||
-	    (!ro->xl_frames && can_is_canxl_skb(oskb)))
+	if (!ro->fd_frames && can_is_canfd_skb(oskb))
 		return;
 
+	if (can_is_canxl_skb(oskb)) {
+		struct canxl_frame *cxl = (struct canxl_frame *)oskb->data;
+
+		/* make sure to not pass oversized frames to the socket */
+		if (!ro->xl_frames)
+			return;
+
+		/* filter CAN XL VCID content */
+		if (ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_RX_FILTER) {
+			/* apply VCID filter if user enabled the filter */
+			if ((cxl->prio & ro->rx_vcid_mask_shifted) !=
+			    (ro->rx_vcid_shifted & ro->rx_vcid_mask_shifted))
+				return;
+		} else {
+			/* no filter => do not forward VCID tagged frames */
+			if (cxl->prio & CANXL_VCID_MASK)
+				return;
+		}
+	}
+
 	/* eliminate multiple filter matches for the same skb */
 	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
 	    this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
 		if (!ro->join_filters)
 			return;
@@ -696,10 +719,23 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		/* Enabling CAN XL includes CAN FD */
 		if (ro->xl_frames)
 			ro->fd_frames = ro->xl_frames;
 		break;
 
+	case CAN_RAW_XL_VCID_OPTS:
+		if (optlen != sizeof(ro->raw_vcid_opts))
+			return -EINVAL;
+
+		if (copy_from_sockptr(&ro->raw_vcid_opts, optval, optlen))
+			return -EFAULT;
+
+		/* prepare 32 bit values for handling in hot path */
+		ro->tx_vcid_shifted = ro->raw_vcid_opts.tx_vcid << CANXL_VCID_OFFSET;
+		ro->rx_vcid_shifted = ro->raw_vcid_opts.rx_vcid << CANXL_VCID_OFFSET;
+		ro->rx_vcid_mask_shifted = ro->raw_vcid_opts.rx_vcid_mask << CANXL_VCID_OFFSET;
+		break;
+
 	case CAN_RAW_JOIN_FILTERS:
 		if (optlen != sizeof(ro->join_filters))
 			return -EINVAL;
 
 		if (copy_from_sockptr(&ro->join_filters, optval, optlen))
@@ -784,10 +820,25 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		if (len > sizeof(int))
 			len = sizeof(int);
 		val = &ro->xl_frames;
 		break;
 
+	case CAN_RAW_XL_VCID_OPTS:
+		/* user space buffer to small for VCID opts? */
+		if (len < sizeof(ro->raw_vcid_opts)) {
+			/* return -ERANGE and needed space in optlen */
+			err = -ERANGE;
+			if (put_user(sizeof(ro->raw_vcid_opts), optlen))
+				err = -EFAULT;
+		} else {
+			if (len > sizeof(ro->raw_vcid_opts))
+				len = sizeof(ro->raw_vcid_opts);
+			if (copy_to_user(optval, &ro->raw_vcid_opts, len))
+				err = -EFAULT;
+		}
+		break;
+
 	case CAN_RAW_JOIN_FILTERS:
 		if (len > sizeof(int))
 			len = sizeof(int);
 		val = &ro->join_filters;
 		break;
@@ -814,12 +865,34 @@ static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
 	    (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
 		return false;
 
 	/* CAN XL -> needs to be enabled and a CAN XL device */
 	if (ro->xl_frames && can_is_canxl_skb(skb) &&
-	    can_is_canxl_dev_mtu(mtu))
+	    can_is_canxl_dev_mtu(mtu)) {
+		struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
+
+		/* sanitize non CAN XL bits */
+		cxl->prio &= (CANXL_PRIO_MASK | CANXL_VCID_MASK);
+
+		/* clear VCID in CAN XL frame if pass through is disabled */
+		if (!(ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_TX_PASS))
+			cxl->prio &= CANXL_PRIO_MASK;
+
+		/* set VCID in CAN XL frame if enabled */
+		if (ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_TX_SET) {
+			cxl->prio &= CANXL_PRIO_MASK;
+			cxl->prio |= ro->tx_vcid_shifted;
+		}
+
+		/* set flag only for 'tagged' VCID values */
+		if (cxl->prio & CANXL_VCID_MASK)
+			cxl->flags |= CANXL_VCID;
+		else
+			cxl->flags &= ~CANXL_VCID;
+
 		return false;
+	}
 
 	return true;
 }
 
 static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
-- 
2.34.1

Re: [PATCH] canxl: add virtual CAN network identifier support

[Oliver Hartkopp]

Hi all,

I've been working some time how to integrate the CAN XL VCID stuff into 
the Linux CAN infrastructure.

Going for a complete IEEE 802.1Q VLAN-style was not possible due to 
several reasons. Getting the VCID from the system via cmsg could have 
been done with a new extended cmsg analogue to the packet socket. But 
'sending' different VCIDs is not intended and would really lead to a 
complete mess.

To be able to read/write complete CAN XL frames and to store and replay 
the entire CAN traffic (with candump/canplayer) the VCID is now stored 
in an unused space in the prio element. Looks a bit ugly but is handy to 
use and does not interfere the filter infrastructure and CAN/CANFD frames.

Defining VLAN-like CAN network devices like canxl0.123 could still be 
done later. Same applies to Ethernet-like software interfaces to carry 
IP over CAN XL.

Some PoC tools and a WireShark plugin to 'decode' CAN XL traffic can be 
found here:

https://github.com/hartkopp/canxl-utils

Thanks for review and feedback.

Best regards,
Oliver

On 06.01.24 20:28, Oliver Hartkopp wrote:
> CAN XL data frames contain an 8-bit virtual CAN network identifier (VCID).
> A VCID value of zero represents an 'untagged' CAN XL frame.
> 
> To receive and send these optional VCIDs via CAN_RAW sockets a new socket
> option CAN_RAW_XL_VCID_OPTS is introduced to define/access VCID content:
> 
> - tx: set the outgoing VCID value by the kernel (one fixed 8-bit value)
> - tx: pass through VCID values from the user space (e.g. for traffic replay)
> - rx: apply VCID receive filter (value/mask) to be passed to the user space
> 
> With the 'tx pass through' option CAN_RAW_XL_VCID_TX_PASS all valid VCID
> values can be send, e.g. to replay full qualified CAN XL traffic.
> 
> The VCID value provided for the CAN_RAW_XL_VCID_TX_SET option will
> override the VCID value in the struct canxl_frame.prio defined for
> CAN_RAW_XL_VCID_TX_PASS when both flags are set.
> 
> With a rx_vcid_mask of zero all possible VCID values (0x00 - 0xFF) are passed
> to the user space when the CAN_RAW_XL_VCID_RX_FILTER flag is set. Without this
> flag only untagged CAN XL frames (VCID = 0x00, CANXL_VCID flag unset) are
> delivered to the user space.
> 
> The 8-bit VCID is stored inside the CAN XL prio element (only in CAN XL
> frames!) to not interfere with other CAN content or the CAN filters
> provided by the CAN_RAW sockets and kernel infrastruture.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>   include/uapi/linux/can.h     | 14 +++++--
>   include/uapi/linux/can/raw.h | 14 +++++++
>   net/can/raw.c                | 79 ++++++++++++++++++++++++++++++++++--
>   3 files changed, 100 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 939db2388208..30453905fbd2 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -180,34 +180,40 @@ struct canfd_frame {
>   };
>   
>   /*
>    * 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_frame.flags element contains CAN XL frame specific bits 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) */
> +#define CANXL_VCID 0x02 /* prio contains the virtual CAN network identifier */
> +
> +/* the 8-bit VCID is optionally placed in the canxl_frame.prio element */
> +#define CANXL_VCID_OFFSET 16 /* bit offset of VCID in prio element */
> +#define CANXL_VCID_VAL_MASK 0xFFUL /* VCID is an 8-bit value */
> +#define CANXL_VCID_MASK (CANXL_VCID_VAL_MASK << CANXL_VCID_OFFSET)
>   
>   /**
>    * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> - * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags / VCID
>    * @flags: additional flags for CAN XL
>    * @sdt:   SDU (service data unit) type
>    * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
>    * @af:    acceptance field
>    * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
>    *
>    * @prio shares the same position as @can_id from struct can[fd]_frame.
>    */
>   struct canxl_frame {
> -	canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> +	canid_t prio;  /* 11 bit priority for arbitration / 8 bit VCID */
>   	__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];
> diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
> index 31622c9b7988..8890b0d2fd48 100644
> --- a/include/uapi/linux/can/raw.h
> +++ b/include/uapi/linux/can/raw.h
> @@ -63,8 +63,22 @@ enum {
>   	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) */
> +	CAN_RAW_XL_VCID_OPTS,	/* CAN XL VCID configuration options */
>   };
>   
> +struct can_raw_vcid_options {
> +
> +	__u8 flags;		/* flags for vcid (filter) behaviour */
> +	__u8 tx_vcid;		/* VCID value set into canxl_frame.prio */
> +	__u8 rx_vcid;		/* VCID value for VCID filter */
> +	__u8 rx_vcid_mask;	/* VCID mask for VCID filter */
> +
> +};
> +
> +#define CAN_RAW_XL_VCID_TX_SET		0x01
> +#define CAN_RAW_XL_VCID_TX_PASS		0x02
> +#define CAN_RAW_XL_VCID_RX_FILTER	0x04
> +
>   #endif /* !_UAPI_CAN_RAW_H */
> diff --git a/net/can/raw.c b/net/can/raw.c
> index e6b822624ba2..a6f0b6a0a934 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -89,10 +89,14 @@ struct raw_sock {
>   	struct list_head notifier;
>   	int loopback;
>   	int recv_own_msgs;
>   	int fd_frames;
>   	int xl_frames;
> +	struct can_raw_vcid_options raw_vcid_opts;
> +	canid_t tx_vcid_shifted;
> +	canid_t rx_vcid_shifted;
> +	canid_t rx_vcid_mask_shifted;
>   	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;
> @@ -132,14 +136,33 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>   	/* check the received tx sock reference */
>   	if (!ro->recv_own_msgs && oskb->sk == sk)
>   		return;
>   
>   	/* make sure to not pass oversized frames to the socket */
> -	if ((!ro->fd_frames && can_is_canfd_skb(oskb)) ||
> -	    (!ro->xl_frames && can_is_canxl_skb(oskb)))
> +	if (!ro->fd_frames && can_is_canfd_skb(oskb))
>   		return;
>   
> +	if (can_is_canxl_skb(oskb)) {
> +		struct canxl_frame *cxl = (struct canxl_frame *)oskb->data;
> +
> +		/* make sure to not pass oversized frames to the socket */
> +		if (!ro->xl_frames)
> +			return;
> +
> +		/* filter CAN XL VCID content */
> +		if (ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_RX_FILTER) {
> +			/* apply VCID filter if user enabled the filter */
> +			if ((cxl->prio & ro->rx_vcid_mask_shifted) !=
> +			    (ro->rx_vcid_shifted & ro->rx_vcid_mask_shifted))
> +				return;
> +		} else {
> +			/* no filter => do not forward VCID tagged frames */
> +			if (cxl->prio & CANXL_VCID_MASK)
> +				return;
> +		}
> +	}
> +
>   	/* eliminate multiple filter matches for the same skb */
>   	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
>   	    this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
>   		if (!ro->join_filters)
>   			return;
> @@ -696,10 +719,23 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>   		/* Enabling CAN XL includes CAN FD */
>   		if (ro->xl_frames)
>   			ro->fd_frames = ro->xl_frames;
>   		break;
>   
> +	case CAN_RAW_XL_VCID_OPTS:
> +		if (optlen != sizeof(ro->raw_vcid_opts))
> +			return -EINVAL;
> +
> +		if (copy_from_sockptr(&ro->raw_vcid_opts, optval, optlen))
> +			return -EFAULT;
> +
> +		/* prepare 32 bit values for handling in hot path */
> +		ro->tx_vcid_shifted = ro->raw_vcid_opts.tx_vcid << CANXL_VCID_OFFSET;
> +		ro->rx_vcid_shifted = ro->raw_vcid_opts.rx_vcid << CANXL_VCID_OFFSET;
> +		ro->rx_vcid_mask_shifted = ro->raw_vcid_opts.rx_vcid_mask << CANXL_VCID_OFFSET;
> +		break;
> +
>   	case CAN_RAW_JOIN_FILTERS:
>   		if (optlen != sizeof(ro->join_filters))
>   			return -EINVAL;
>   
>   		if (copy_from_sockptr(&ro->join_filters, optval, optlen))
> @@ -784,10 +820,25 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
>   		if (len > sizeof(int))
>   			len = sizeof(int);
>   		val = &ro->xl_frames;
>   		break;
>   
> +	case CAN_RAW_XL_VCID_OPTS:
> +		/* user space buffer to small for VCID opts? */
> +		if (len < sizeof(ro->raw_vcid_opts)) {
> +			/* return -ERANGE and needed space in optlen */
> +			err = -ERANGE;
> +			if (put_user(sizeof(ro->raw_vcid_opts), optlen))
> +				err = -EFAULT;
> +		} else {
> +			if (len > sizeof(ro->raw_vcid_opts))
> +				len = sizeof(ro->raw_vcid_opts);
> +			if (copy_to_user(optval, &ro->raw_vcid_opts, len))
> +				err = -EFAULT;
> +		}
> +		break;
> +
>   	case CAN_RAW_JOIN_FILTERS:
>   		if (len > sizeof(int))
>   			len = sizeof(int);
>   		val = &ro->join_filters;
>   		break;
> @@ -814,12 +865,34 @@ static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
>   	    (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
>   		return false;
>   
>   	/* CAN XL -> needs to be enabled and a CAN XL device */
>   	if (ro->xl_frames && can_is_canxl_skb(skb) &&
> -	    can_is_canxl_dev_mtu(mtu))
> +	    can_is_canxl_dev_mtu(mtu)) {
> +		struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
> +
> +		/* sanitize non CAN XL bits */
> +		cxl->prio &= (CANXL_PRIO_MASK | CANXL_VCID_MASK);
> +
> +		/* clear VCID in CAN XL frame if pass through is disabled */
> +		if (!(ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_TX_PASS))
> +			cxl->prio &= CANXL_PRIO_MASK;
> +
> +		/* set VCID in CAN XL frame if enabled */
> +		if (ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_TX_SET) {
> +			cxl->prio &= CANXL_PRIO_MASK;
> +			cxl->prio |= ro->tx_vcid_shifted;
> +		}
> +
> +		/* set flag only for 'tagged' VCID values */
> +		if (cxl->prio & CANXL_VCID_MASK)
> +			cxl->flags |= CANXL_VCID;
> +		else
> +			cxl->flags &= ~CANXL_VCID;
> +
>   		return false;
> +	}
>   
>   	return true;
>   }
>   
>   static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)

Re: [PATCH] canxl: add virtual CAN network identifier support

[Vincent MAILHOL]

On Sun. 7 Jan. 2024 at 04:47, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> CAN XL data frames contain an 8-bit virtual CAN network identifier (VCID).
> A VCID value of zero represents an 'untagged' CAN XL frame.
>
> To receive and send these optional VCIDs via CAN_RAW sockets a new socket
> option CAN_RAW_XL_VCID_OPTS is introduced to define/access VCID content:
>
> - tx: set the outgoing VCID value by the kernel (one fixed 8-bit value)
> - tx: pass through VCID values from the user space (e.g. for traffic replay)
> - rx: apply VCID receive filter (value/mask) to be passed to the user space
>
> With the 'tx pass through' option CAN_RAW_XL_VCID_TX_PASS all valid VCID
> values can be send, e.g. to replay full qualified CAN XL traffic.
                ^^^^

sent

>
> The VCID value provided for the CAN_RAW_XL_VCID_TX_SET option will
> override the VCID value in the struct canxl_frame.prio defined for
> CAN_RAW_XL_VCID_TX_PASS when both flags are set.
>
> With a rx_vcid_mask of zero all possible VCID values (0x00 - 0xFF) are passed
> to the user space when the CAN_RAW_XL_VCID_RX_FILTER flag is set. Without this
> flag only untagged CAN XL frames (VCID = 0x00, CANXL_VCID flag unset) are
> delivered to the user space.
>
> The 8-bit VCID is stored inside the CAN XL prio element (only in CAN XL
> frames!) to not interfere with other CAN content or the CAN filters
> provided by the CAN_RAW sockets and kernel infrastruture.
                                             ^^^^^^^^^^^^^

infrastructure

>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/uapi/linux/can.h     | 14 +++++--
>  include/uapi/linux/can/raw.h | 14 +++++++
>  net/can/raw.c                | 79 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 100 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 939db2388208..30453905fbd2 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -180,34 +180,40 @@ struct canfd_frame {
>  };
>
>  /*
>   * 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_frame.flags element contains CAN XL frame specific bits 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) */
> +#define CANXL_VCID 0x02 /* prio contains the virtual CAN network identifier */
> +
> +/* the 8-bit VCID is optionally placed in the canxl_frame.prio element */
> +#define CANXL_VCID_OFFSET 16 /* bit offset of VCID in prio element */
> +#define CANXL_VCID_VAL_MASK 0xFFUL /* VCID is an 8-bit value */
> +#define CANXL_VCID_MASK (CANXL_VCID_VAL_MASK << CANXL_VCID_OFFSET)
>
>  /**
>   * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> - * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags / VCID
>   * @flags: additional flags for CAN XL
>   * @sdt:   SDU (service data unit) type
>   * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
>   * @af:    acceptance field
>   * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
>   *
>   * @prio shares the same position as @can_id from struct can[fd]_frame.
>   */
>  struct canxl_frame {
> -       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> +       canid_t prio;  /* 11 bit priority for arbitration / 8 bit VCID */

Isn't this a UAPI breaking change? Prior to this patch, the applications may do:

  canxl_frame.prio

to get the prio, but after this patch, applications are required to do:

  canxl_frame.prio & CANXL_PRIO_MASK

in order to mask out the VCID (currently, there are no requirements
that canxl_frame.prio must be masked before use).
In the past, I was reluctant to acknowledge the introduction of CANXL
in the kernel prior to reading the ISO standard because I was afraid
of such UAPI stability issues. Now we have to deal with it.

What do you think of:

  struct canxl_frame {
  #if defined(__LITTLE_ENDIAN)
          __u16 prio;  /* 11 bit priority for arbitration */
          __u8 vcid; /* 8 bit VCID */
          __u8 __reserved; /* must be 0 */
         /* ... */
  #elif defined(__BIG_ENDIAN)
          __u8 __reserved; /* must be 0 */
          __u8 vcid; /* 8 bit VCID */
          __u16 prio;  /* 11 bit priority for arbitration */
  #else
  #error "Unknown endianness"
  #endif
  }

Here, canxl_frame.prio always gives a correct value without need for
CANXL_PRIO_MASK masking. The big/little endianness checks are needed
to maintain the ABI compatibility. Not yet tested, so forgive if there
is a mistake. Getting the endianness logic correct on a first try is
not easy.

Also, the VCID can now be accessed through canxl_frame.vcid instead of
relying on some mask and shift logic.

The drawback is that you lose the can_id type. For what I understand,
this is only used for filtering. If we absolutely need to maintain the
canid_t, then maybe:

  struct canxl_frame {
          union {
                  canid_t filter;
                  struct {
  #if defined(__LITTLE_ENDIAN)
                          __u16 prio;  /* 11 bit priority for arbitration */
                          __u8 vcid; /* 8 bit VCID */
                          __u8 __reserved; /* must be 0 */
  #elif defined(__BIG_ENDIAN)
                          __u8 __reserved; /* must be 0 */
                          __u8 vcid; /* 8 bit VCID */
                          __u16 prio;  /* 11 bit priority for arbitration */
  #else
  #error "Unknown endianness"
  #endif
                  };
          };
         /* ... */
  }

But I think it is better to drop it. If someone wants a canid_t, then
he or she can just cast the XL frame to either struct can_frame or
struct canfd_frame.

Though?

>         __u8    flags; /* additional flags for CAN XL */

If CANXL_VCID is set, can vcid be zero? If not, no need for a flag.
Just need to check if canxl_frame.vcid is not zero.

>         __u8    sdt;   /* SDU (service data unit) type */
>         __u16   len;   /* frame payload length in byte */
>         __u32   af;    /* acceptance field */
>         __u8    data[CANXL_MAX_DLEN];
> diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
> index 31622c9b7988..8890b0d2fd48 100644
> --- a/include/uapi/linux/can/raw.h
> +++ b/include/uapi/linux/can/raw.h
> @@ -63,8 +63,22 @@ enum {
>         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) */
> +       CAN_RAW_XL_VCID_OPTS,   /* CAN XL VCID configuration options */
>  };
>
> +struct can_raw_vcid_options {
> +
> +       __u8 flags;             /* flags for vcid (filter) behaviour */
> +       __u8 tx_vcid;           /* VCID value set into canxl_frame.prio */
> +       __u8 rx_vcid;           /* VCID value for VCID filter */
> +       __u8 rx_vcid_mask;      /* VCID mask for VCID filter */
> +
> +};
> +
> +#define CAN_RAW_XL_VCID_TX_SET         0x01
> +#define CAN_RAW_XL_VCID_TX_PASS                0x02
> +#define CAN_RAW_XL_VCID_RX_FILTER      0x04
> +
>  #endif /* !_UAPI_CAN_RAW_H */
> diff --git a/net/can/raw.c b/net/can/raw.c
> index e6b822624ba2..a6f0b6a0a934 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -89,10 +89,14 @@ struct raw_sock {
>         struct list_head notifier;
>         int loopback;
>         int recv_own_msgs;
>         int fd_frames;
>         int xl_frames;
> +       struct can_raw_vcid_options raw_vcid_opts;
> +       canid_t tx_vcid_shifted;
> +       canid_t rx_vcid_shifted;
> +       canid_t rx_vcid_mask_shifted;
>         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;
> @@ -132,14 +136,33 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>         /* check the received tx sock reference */
>         if (!ro->recv_own_msgs && oskb->sk == sk)
>                 return;
>
>         /* make sure to not pass oversized frames to the socket */
> -       if ((!ro->fd_frames && can_is_canfd_skb(oskb)) ||
> -           (!ro->xl_frames && can_is_canxl_skb(oskb)))
> +       if (!ro->fd_frames && can_is_canfd_skb(oskb))
>                 return;
>
> +       if (can_is_canxl_skb(oskb)) {
> +               struct canxl_frame *cxl = (struct canxl_frame *)oskb->data;
> +
> +               /* make sure to not pass oversized frames to the socket */
> +               if (!ro->xl_frames)
> +                       return;
> +
> +               /* filter CAN XL VCID content */
> +               if (ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_RX_FILTER) {
> +                       /* apply VCID filter if user enabled the filter */
> +                       if ((cxl->prio & ro->rx_vcid_mask_shifted) !=
> +                           (ro->rx_vcid_shifted & ro->rx_vcid_mask_shifted))
> +                               return;
> +               } else {
> +                       /* no filter => do not forward VCID tagged frames */
> +                       if (cxl->prio & CANXL_VCID_MASK)
> +                               return;
> +               }
> +       }
> +
>         /* eliminate multiple filter matches for the same skb */
>         if (this_cpu_ptr(ro->uniq)->skb == oskb &&
>             this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
>                 if (!ro->join_filters)
>                         return;
> @@ -696,10 +719,23 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>                 /* Enabling CAN XL includes CAN FD */
>                 if (ro->xl_frames)
>                         ro->fd_frames = ro->xl_frames;
>                 break;
>
> +       case CAN_RAW_XL_VCID_OPTS:
> +               if (optlen != sizeof(ro->raw_vcid_opts))
> +                       return -EINVAL;
> +
> +               if (copy_from_sockptr(&ro->raw_vcid_opts, optval, optlen))
> +                       return -EFAULT;
> +
> +               /* prepare 32 bit values for handling in hot path */
> +               ro->tx_vcid_shifted = ro->raw_vcid_opts.tx_vcid << CANXL_VCID_OFFSET;
> +               ro->rx_vcid_shifted = ro->raw_vcid_opts.rx_vcid << CANXL_VCID_OFFSET;
> +               ro->rx_vcid_mask_shifted = ro->raw_vcid_opts.rx_vcid_mask << CANXL_VCID_OFFSET;
> +               break;
> +
>         case CAN_RAW_JOIN_FILTERS:
>                 if (optlen != sizeof(ro->join_filters))
>                         return -EINVAL;
>
>                 if (copy_from_sockptr(&ro->join_filters, optval, optlen))
> @@ -784,10 +820,25 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
>                 if (len > sizeof(int))
>                         len = sizeof(int);
>                 val = &ro->xl_frames;
>                 break;
>
> +       case CAN_RAW_XL_VCID_OPTS:
> +               /* user space buffer to small for VCID opts? */
> +               if (len < sizeof(ro->raw_vcid_opts)) {
> +                       /* return -ERANGE and needed space in optlen */
> +                       err = -ERANGE;
> +                       if (put_user(sizeof(ro->raw_vcid_opts), optlen))
> +                               err = -EFAULT;
> +               } else {
> +                       if (len > sizeof(ro->raw_vcid_opts))
> +                               len = sizeof(ro->raw_vcid_opts);
> +                       if (copy_to_user(optval, &ro->raw_vcid_opts, len))
> +                               err = -EFAULT;
> +               }
> +               break;
> +
>         case CAN_RAW_JOIN_FILTERS:
>                 if (len > sizeof(int))
>                         len = sizeof(int);
>                 val = &ro->join_filters;
>                 break;
> @@ -814,12 +865,34 @@ static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
>             (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
>                 return false;
>
>         /* CAN XL -> needs to be enabled and a CAN XL device */
>         if (ro->xl_frames && can_is_canxl_skb(skb) &&
> -           can_is_canxl_dev_mtu(mtu))
> +           can_is_canxl_dev_mtu(mtu)) {
> +               struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
> +
> +               /* sanitize non CAN XL bits */
> +               cxl->prio &= (CANXL_PRIO_MASK | CANXL_VCID_MASK);
> +
> +               /* clear VCID in CAN XL frame if pass through is disabled */
> +               if (!(ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_TX_PASS))
> +                       cxl->prio &= CANXL_PRIO_MASK;
> +
> +               /* set VCID in CAN XL frame if enabled */
> +               if (ro->raw_vcid_opts.flags & CAN_RAW_XL_VCID_TX_SET) {
> +                       cxl->prio &= CANXL_PRIO_MASK;
> +                       cxl->prio |= ro->tx_vcid_shifted;
> +               }
> +
> +               /* set flag only for 'tagged' VCID values */
> +               if (cxl->prio & CANXL_VCID_MASK)
> +                       cxl->flags |= CANXL_VCID;
> +               else
> +                       cxl->flags &= ~CANXL_VCID;
> +
>                 return false;
> +       }
>
>         return true;
>  }
>
>  static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> --
> 2.34.1
>
>

Re: [PATCH] canxl: add virtual CAN network identifier support

[Oliver Hartkopp]

Hi Vincent,

thanks for your review!

On 07.01.24 07:28, Vincent MAILHOL wrote:
> On Sun. 7 Jan. 2024 at 04:47, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>> values can be send, e.g. to replay full qualified CAN XL traffic.
>                  ^^^^
> sent
ACK

>> provided by the CAN_RAW sockets and kernel infrastruture.
>                                               ^^^^^^^^^^^^^
> infrastructure
ACK

>>   struct canxl_frame {
>> -       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>> +       canid_t prio;  /* 11 bit priority for arbitration / 8 bit VCID */
> 
> Isn't this a UAPI breaking change? Prior to this patch, the applications may do:
> 
>    canxl_frame.prio
> 
> to get the prio, but after this patch, applications are required to do:
> 
>    canxl_frame.prio & CANXL_PRIO_MASK
> 

Not really. I also thought about it but you *only* need to take care 
about the VCID content when you have enabled it explicitly with the new 
sockopt. Otherwise you will never see anything beyond the 32 bit prio.

> in order to mask out the VCID (currently, there are no requirements
> that canxl_frame.prio must be masked before use).
> In the past, I was reluctant to acknowledge the introduction of CANXL
> in the kernel prior to reading the ISO standard because I was afraid
> of such UAPI stability issues. Now we have to deal with it.

Yes, but that kind of extension would be backwards compatible.

> What do you think of:
> 
>    struct canxl_frame {
>    #if defined(__LITTLE_ENDIAN)
>            __u16 prio;  /* 11 bit priority for arbitration */
>            __u8 vcid; /* 8 bit VCID */
>            __u8 __reserved; /* must be 0 */
>           /* ... */
>    #elif defined(__BIG_ENDIAN)
>            __u8 __reserved; /* must be 0 */
>            __u8 vcid; /* 8 bit VCID */
>            __u16 prio;  /* 11 bit priority for arbitration */
>    #else
>    #error "Unknown endianness"
>    #endif
>    }
> 
> Here, canxl_frame.prio always gives a correct value without need for
> CANXL_PRIO_MASK masking. The big/little endianness checks are needed
> to maintain the ABI compatibility. Not yet tested, so forgive if there
> is a mistake. Getting the endianness logic correct on a first try is
> not easy.

Yes, I tested such approach too (with little endian only) and it worked 
great - and of course looked better in the code.

> Also, the VCID can now be accessed through canxl_frame.vcid instead of
> relying on some mask and shift logic.

Right. That looked nice.

> The drawback is that you lose the can_id type. For what I understand,
> this is only used for filtering. If we absolutely need to maintain the
> canid_t, then maybe:
> 
>    struct canxl_frame {
>            union {
>                    canid_t filter;
>                    struct {
>    #if defined(__LITTLE_ENDIAN)
>                            __u16 prio;  /* 11 bit priority for arbitration */
>                            __u8 vcid; /* 8 bit VCID */
>                            __u8 __reserved; /* must be 0 */
>    #elif defined(__BIG_ENDIAN)
>                            __u8 __reserved; /* must be 0 */
>                            __u8 vcid; /* 8 bit VCID */
>                            __u16 prio;  /* 11 bit priority for arbitration */
>    #else
>    #error "Unknown endianness"
>    #endif
>                    };
>            };
>           /* ... */
>    }
> 
> But I think it is better to drop it. If someone wants a canid_t, then
> he or she can just cast the XL frame to either struct can_frame or
> struct canfd_frame.
> 
> Though?

My only concern is that it looks really ugly :-/

The change of the prio element from u32 to u16 will also not harm anyone 
as I assume to be the only person who's currently working with CAN XL 
frames on virtual CAN interfaces:

https://github.com/hartkopp?tab=repositories&q=can-cia

I'll prepare a patch that picks up this suggestion of an __u16 prio etc.

Maybe we can add some compile time checks to ensure the correct struct 
layout for this case.

> 
>>          __u8    flags; /* additional flags for CAN XL */
> 
> If CANXL_VCID is set, can vcid be zero? If not, no need for a flag.
> Just need to check if canxl_frame.vcid is not zero.
> 

This is probably indeed a leftover which can be removed with my latest 
implementation. Will recheck.

Many thanks,
Oliver