Re: [RFC PATCH] can: dev: can_dev_dropped_skb: drop CAN FD skbs if FD is off

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Vincent,

can_dev_dropped_skb() is not what you are looking for.

Whether a CAN frame fits to the CAN device is checked in 
raw_check_txframe() here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/can/raw.c#n884

Or do I miss anything?

Best regards,
Oliver

On 07.09.25 10:05, Vincent Mailhol wrote:
> Currently, the CAN FD skb validation logic is based on the MTU: the
> interface is deemed FD capable if and only if its MTU is greater or
> equal to CANFD_MTU.
> 
> This logic is showing its limits since the introduction of CAN XL.
> Indeed, an interface which is configured to support CAN XL but not CAN
> FD would have its MTU set to CANXL_MIN_MTU which is greater than
> CANFD_MTU, thus fulfilling the CAN FD check condition. That is to say,
> any CAN XL interface currently appears as CAN FD capable with no way
> of configuring or probing this at the skb level.
> 
> Because of the limitation, the only non-UAPI-breaking workaround is to
> perform the check at the device level using the CAN_CTRLMODE_FD flag.
> Unfortunately, the virtual interfaces (vcan, vxcan) are left behind
> with this approach.
> 
> Add a check on the CAN_CTRLMODE_FD flag in can_dev_dropped_skb() and
> drop FD frames whenever the feature is turned off.
> 
> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> ---
> While testing, I encountered a potential issue when:
> 
>    - CAN FD is off
>    - CAN XL is on
> 
> My understanding is that this is allowed and, even more, it is
> sometimes mandatory when, for example, disabling error signalling or
> when using the transceiver mode switch (TMS) or both.
> 
> It seems to me that the current implementation is incapable of
> handling this case at the skb level for the reasons listed in the
> patch body.
> 
> I have two ideas to fix this issue:
> 
>    1. above patch which assumes that we are OK not being able to set
>       CAN XL on and FD off on virtual interfaces
> 
>    2. add some information to the skb itself. I have not looked at this
>       in detail. One potential idea would be to use:
> 
>         sk_buff->protocol
> 
>       as a bitfield. Whenever the MTU is greater than CANFD_MTU, below
>       logic would apply:
> 
>         - if sk_buff->protocol & CAN_SKB_CC is true, classical CAN is
>           supported
> 
>         - if sk_buff->protocol & CAN_SKB_FD is true, CAN FD is
>           supported
> 
>         - if sk_buff->protocol & CAN_SKB_XL is true, CAN XL is
>           supported
> 
>       with any of the combinations above accepted.
> 
>       Of course, because this comes as an afterthought, it is an UAPI
>       breaking change for any existing CAN XL code. Also,
>       sk_buff->protocol was not designed for this, making this solution
>       an ugly hack.
> 
> Thanks for your comments.
> ---
>   include/linux/can/dev.h | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 9a92cbe5b2cb7..5053d5bc20c99 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -164,12 +164,18 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
>   	if (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY) {
>   		netdev_info_once(dev,
>   				 "interface in listen only mode, dropping skb\n");
> -		kfree_skb(skb);
> -		dev->stats.tx_dropped++;
> -		return true;
> +		goto invalid_skb;
>   	}
>   
> +	if (!(priv->ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb))
> +		goto invalid_skb;
> +
>   	return can_dropped_invalid_skb(dev, skb);
> +
> +invalid_skb:
> +	kfree_skb(skb);
> +	dev->stats.tx_dropped++;
> +	return true;
>   }
>   
>   void can_setup(struct net_device *dev);