Re: [PATCH 1/9] can: dev: can_dev_dropped_skb: drop CAN FD skbs if FD is off

[Vincent Mailhol <mailhol@kernel.org>]

On 17/10/2025 at 22:27, Marc Kleine-Budde wrote:
> On 17.10.2025 10:28:38, Marc Kleine-Budde wrote:
>> On 13.10.2025 20:01:23, 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 limit with the introduction of CAN XL. For
>>> example, consider the two scenarios below:
>>>
>>>   1. An interface configured with CAN FD on and CAN XL on
>>>
>>>   2. An interface configured with CAN FD off and CAN XL on
>>>
>>> In those two scenarios, the interfaces would have the same MTU:
>>>
>>>   CANXL_MTU
>>>
>>> making it impossible to differentiate which one has CAN FD turned on
>>> and which one has it off.
>>>
>>> Because of the limitation, the only non-UAPI-breaking workaround is to
>>> do the check at the device level using the can_priv->ctrlmode flags.
>>> Unfortunately, the virtual interfaces (vcan, vxcan), which do not have
>>> a can_priv, are left behind.
>>>
>>> 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>
>>
>> What about merging both can_dev_dropped_skb() an
>> can_dropped_invalid_skb() in the skb.c, so that there is no stub in the
>> header file anymore.
> 
> Ouch! Don't do this. We still need can_dropped_invalid_skb() for virtual
> interfaces. See commit ae64438be192 ("can: dev: fix skb drop check").

Exactly!

> But then I'm asking: Why is there the difference between virtual and
> non-virtual interface in the first place? Can we get rid of it?

The fact is that:

  - We need a function for the physical interfaces to check the
    CAN_CTRLMODE_LISTENONLY, i.e. with an access to struct can_priv.

  - We need a similar function but which work for the virtual
    interfaces which do not have a can_priv member.

So unless we do a major code refactor so that the virtual interfaces,
I do not see how we could get rid of it.

>> Someone (i.e. me) used can_dropped_invalid_skb() in a driver, that means
>> the check for CAN_CTRLMODE_LISTENONLY is missing :/ (I'll send a fix).

At least, this does not seem like a security breach like it was the
case for the missing net_device_ops->ndo_change_mtu().

> These drivers need this fix:
> 
> | drivers/net/can/bxcan.c:845:     if (can_dropped_invalid_skb(ndev, skb))
> | drivers/net/can/esd/esdacc.c:257:        if (can_dropped_invalid_skb(netdev, skb))
> | drivers/net/can/rockchip/rockchip_canfd-tx.c:75: if (can_dropped_invalid_skb(ndev, skb))

Yeah, I think that this is a pitfall, but at the same time, I do not
see how to get rid of this can_dev_dropped_skb() and
can_dropped_invalid_skb() pair. The best I can think of it to be
careful on this problem doing the code review now that we are aware
about it.


Yours sincerely,
Vincent Mailhol