Re: [PATCH b4/canxl-netlink v2] can: drop unsupported CAN frames on socket and netdev level

[Vincent Mailhol <mailhol@kernel.org>]

Hi Oliver,

On 03/11/2025 at 19:53, Oliver Hartkopp wrote:
> Rework the checks for skbs containing CAN CC/FD/XL frames.
> 
> For real CAN interfaces the CAN_CTRLMODE_FD and CAN_CTRLMODE_XL control
> modes indicate whether an interface can handle those CAN FD/XL frames.
> 
> In the case a CAN XL interface is used with "TMS on" or "ERR_SIGNAL off"
> neither CAN CC nor CAN FD frames are supported to be sent. Add a check
> for the so-called 'mixed mode' (CC/FD/XL) in can_dev_cc_enabled().
> 
> CAN_CTRLMODE_FD is ensured to be disabled when TMS is enabled or
> ERR_SIGNAL is off.
> 
> The checks are performed on CAN_RAW sockets to give an instant feedback
> to the user when writing unsupported CAN frames to the interface.
> 
> Additionally we check for correct skbs on CAN netdev level in the case
> the CAN frames are provided via PF_PACKET sockets.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/linux/can/dev.h | 57 ++++++++++++++++++++++++++++++++++++++++-
>  net/can/raw.c           | 19 +++++++-------
>  2 files changed, 65 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 945c16743702..23743c44d300 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -129,10 +129,52 @@ int can_restart_now(struct net_device *dev);
>  void can_bus_off(struct net_device *dev);
>  
>  const char *can_get_state_str(const enum can_state state);
>  const char *can_get_ctrlmode_str(u32 ctrlmode);
>  
> +static inline bool can_dev_cc_enabled(struct net_device *dev)
> +{
> +	struct can_priv *priv = safe_candev_priv(dev);
> +
> +#define MIXED_MODE (CAN_CTRLMODE_FD | CAN_CTRLMODE_XL)
           ^^^^^^^^^^
If this is just used locally in one function, declare it as a u32:

	const u32 mixed_mode = CAN_CTRLMODE_FD | CAN_CTRLMODE_XL;

If you want to keep the #define, add a CAN_ prefix to avoid namespace pollution
and put it at the top of the file.

> +	/* When CAN XL is enabled but FD is disabled we are not running in the
> +	 * so-called 'mixed mode' (CC/FD/XL with TMS OFF and ERR_SIGNAL ON).
> +	 * Then either TMS is ON or ERR_SIGNAL is OFF in which cases the
> +	 * resulting XL-only mode does not allow the sending of CC/FD frames.
> +	 */

If we do this, then the user doing:

	ip link set can0 type can bitrate 1000000 \
		fd off \
		xl on xbitrate 10000000 tms off err-signal on

will get the Classical CAN disabled for no apparent reasons.

Even if the mixed mode is meant for CC + FD + XL, I think it is fair to allow
the end user to request mixed mode with FD disabled (i.e. just keep CC and XL).

> +	if (priv)
> +		return !((priv->ctrlmode & MIXED_MODE) == CAN_CTRLMODE_XL);

What about:

	if (priv)
		return !(priv->ctrlmode & CAN_CTRLMODE_XL) ||
			(priv->ctrlmode & CAN_CTRLMODE_XL_ERR_SIGNAL);

?

> +	/* virtual CAN interfaces always support CAN CC */
> +	return true;
> +}
> +
> +static inline bool can_dev_fd_enabled(struct net_device *dev)
> +{
> +	struct can_priv *priv = safe_candev_priv(dev);
> +
> +	/* check FD ctrlmode on real CAN interfaces */
> +	if (priv)
> +		return (priv->ctrlmode & CAN_CTRLMODE_FD);
> +
> +	/* check MTU for virtual CAN FD interfaces */
> +	return (READ_ONCE(dev->mtu) >= CANFD_MTU);
> +}
> +
> +static inline bool can_dev_xl_enabled(struct net_device *dev)
> +{
> +	struct can_priv *priv = safe_candev_priv(dev);
> +
> +	/* check XL ctrlmode on real CAN interfaces */
> +	if (priv)
> +		return (priv->ctrlmode & CAN_CTRLMODE_XL);
> +
> +	/* check MTU for virtual CAN XL interfaces */
> +	return (READ_ONCE(dev->mtu) >= CANXL_MIN_MTU);
> +}
> +
>  /* drop skb if it does not contain a valid CAN frame for sending */
>  static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *skb)
>  {
>  	struct can_priv *priv = netdev_priv(dev);
>  	u32 silent_mode = priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY |
> @@ -142,15 +184,28 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
>  		netdev_info_once(dev, "interface in %s mode, dropping skb\n",
>  				 can_get_ctrlmode_str(silent_mode));
>  		goto invalid_skb;
>  	}
>  
> -	if (!(priv->ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
> +	/* Classical CAN */
> +	if (can_is_can_skb(skb) && !can_dev_cc_enabled(dev)) {
> +		netdev_info_once(dev, "CAN CC with TMS on, dropping skb\n");
> +		goto invalid_skb;
> +	}
> +
> +	/* CAN FD */
> +	if (can_is_canfd_skb(skb) && !can_dev_fd_enabled(dev)) {
>  		netdev_info_once(dev, "CAN FD is disabled, dropping skb\n");
>  		goto invalid_skb;
>  	}
>  
> +	/* CAN XL */
> +	if (can_is_canxl_skb(skb) && !can_dev_xl_enabled(dev)) {
> +		netdev_info_once(dev, "CAN XL is disabled, dropping skb\n");
> +		goto invalid_skb;
> +	}
> +

The can_dev_*_enabled() functions use safe_candev_priv(), but
can_dev_dropped_skb() is only called by the devices which have a valid priv
member. So, in this context, the safe_candev_priv() becomes useless and the FD
and XL MTU checks are dead code.

The can_dev_*_enabled() must be split in two:

  - the checks on the priv flags go into can_dev_dropped_skb().

  - the checks on the MTU go into can_dropped_invalid_skb()

>  	return can_dropped_invalid_skb(dev, skb);
>  
>  invalid_skb:
>  	kfree_skb(skb);
>  	dev->stats.tx_dropped++;
> diff --git a/net/can/raw.c b/net/can/raw.c
> index a53853f5e9af..645f1e0b2555 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -890,24 +890,23 @@ static void raw_put_canxl_vcid(struct raw_sock *ro, struct sk_buff *skb)
>  		cxl->prio &= CANXL_PRIO_MASK;
>  		cxl->prio |= ro->tx_vcid_shifted;
>  	}
>  }
>  
> -static unsigned int raw_check_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
> +static unsigned int raw_check_txframe(struct raw_sock *ro, struct sk_buff *skb,
> +				      struct net_device *dev)
>  {
> -	/* Classical CAN -> no checks for flags and device capabilities */
> -	if (can_is_can_skb(skb))
> +	/* Classical CAN */
> +	if (can_is_can_skb(skb) && can_dev_cc_enabled(dev))
>  		return CAN_MTU;
>  
> -	/* CAN FD -> needs to be enabled and a CAN FD or CAN XL device */
> -	if (ro->fd_frames && can_is_canfd_skb(skb) &&
> -	    (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
> +	/* CAN FD */
> +	if (ro->fd_frames && can_is_canfd_skb(skb) && can_dev_fd_enabled(dev))
>  		return CANFD_MTU;
>  
> -	/* 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 XL */
> +	if (ro->xl_frames && can_is_canxl_skb(skb) && can_dev_xl_enabled(dev))
>  		return CANXL_MTU;
>  
>  	return 0;
>  }
>  
> @@ -959,11 +958,11 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		goto free_skb;
>  
>  	err = -EINVAL;
>  
>  	/* check for valid CAN (CC/FD/XL) frame content */
> -	txmtu = raw_check_txframe(ro, skb, READ_ONCE(dev->mtu));
> +	txmtu = raw_check_txframe(ro, skb, dev);
>  	if (!txmtu)
>  		goto free_skb;
>  
>  	/* only CANXL: clear/forward/set VCID value */
>  	if (txmtu == CANXL_MTU)


Yours sincerely,
Vincent Mailhol