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

[Vincent Mailhol <mailhol@kernel.org>]

Hi Oliver,

On 09/11/2025 at 20:21, Oliver Hartkopp wrote:
> Hi Vincent,
> 
> On 09.11.25 14:42, Vincent Mailhol wrote:
> 
>>> +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;
> 
> Not sure if a
> 
> #define CAN_XL_MIXED_MODE (CAN_CTRLMODE_FD | CAN_CTRLMODE_XL)
> 
> generally would make sense?!?
> 
> I will therefore go with your local const u32 solution.

Ack. I also prefer it that way. My comment was more toward the fact that I am
not a huge fan of hanging #define in the middle of the code.

>> 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.
> 
> Btw. with the new defaults
> 
> ip link set can0 type can bitrate 1000000 xl on xbitrate 10000000
> 
> should create the same configuration ...

Yes, I just wanted to be explicit in my example.

>> 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).
> 
> But does it make sense when mixed-mode means XL & FD mixing?
> 
> Of course I don't want to argue for "FD on" just because is helps my v2 patch :-D
> 
> But I tend to follow the Bosch spec that mixed-mode is XL/FD/CC until someone
> really shows up with a request to omit FD in the XL/FD mixed mode.

I am not strongly against forbidding the above use case. As I stated in another
patch, everything that we arbitrarily ban can easily be allowed again without
breaking the uapi (the opposite is not true, once something is allowed at uapi
level, it is set in stone).

I agree that I do not see the use case for production at the moment, but I can
foresee that I would be annoyed to have to provide an FD bitrate even if I am
testing on an environment in which I do not (yet) need CAN FD.

It goes to what I stated in my other message: even if the production use case
does not exists, I want to give freedom to the user to do whatever is allowed
within the boundaries of the standard.

>>> +    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);
>>
>> ?
> 
> Huh!
> 
> We need
> 
> CAN_CTRLMODE_XL off (no CAN XL)
> 
> OR
> 
> CAN_CTRLMODE_XL on && CAN_CTRLMODE_XL_ERR_SIGNAL on (mixed mode)
> 
> I needed some minutes to find these conditions in your code as I've seen more
> parenthesis' than you have written ;-)
> 
> But it looks good! Will add this condition with some comment and remove the
> former MIXED_MODE code.

OK. I admit this one was tricky. It also took me a couple minutes to find the
correct formula.

It would probably be better to put this in its own helper function:

  /* Error signaling is only configurable when XL is selected. Otherwise,
   * it is always on. */
  static inline bool can_err_signal_is_enabled(const struct can_priv *priv)
  {
  	return !(priv->ctrlmode & CAN_CTRLMODE_XL) ||
  		(priv->ctrlmode & CAN_CTRLMODE_XL_ERR_SIGNAL);
  }

> This would also be a point for mixed-mode with FD off.
> But I'm still not sure if the mixed-mode without FD makes sense.
> 
> E.g. I'm using a bitrate calculation tool from PEAK and one from Bosch which
> require all bitrates (CC/FD/XL) when error-signalling is on (and TMS off).
> 
> And as I have to enable FD when enabling XL in the Bosch X_CAN IP cores I also
> have to provide a valid FD bitrate setting for the mixed mode.
> 
> This could lead to a new ctrlmode option that FD bitrates are required when XL
> is on.
> 
> ¯\_(ツ)_/¯
>>> +    /* 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
> Ok, what about
> 
> can_dev_*_enabled_safe() calling can_dev_*_enabled() then?

But then, the can_dev_*_enabled() would look something like:

	static inline bool can_dev_cc_enabled(struct can_priv *priv)
	{
		return can_err_signal_is_enabled(priv);
	}

	static inline bool can_dev_fd_enabled(struct can_priv *priv)
	{
		return priv->ctrlmode & CAN_CTRLMODE_FD;
	}

	static inline bool can_dev_fd_enabled(struct can_priv *priv)
	{
		return priv->ctrlmode & CAN_CTRLMODE_XL;
	}

Do we really need a function for that? This is how my draft
can_dev_dropped_skb() looks like at the moment:

  /* 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 |
  					    CAN_CTRLMODE_RESTRICTED);

  	if (silent_mode) {
  		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)) {
  		netdev_info_once(dev, "CAN FD is disabled, dropping skb\n");
  		goto invalid_skb;
  	}

  	if (!can_err_signal_is_enabled(priv) && !can_is_canxl_skb(skb)) {
  		netdev_info_once(dev,
  				 "Error signaling is disabled, dropping skb\n");
  		goto invalid_skb;
  	}

  	return can_dropped_invalid_skb(dev, skb);

  invalid_skb:
  	kfree_skb(skb);
  	dev->stats.tx_dropped++;
  	return true;
  }

(refactored to use the new can_err_signal_is_enabled() as proposed above).

Also, if the above looks good to you, you can leave the can_dev_dropped_skb() to
me and just focus on the can_dropped_invalid_skb() and the raw.c.

> can_dev_*_enabled_safe() is only used from raw.c which must handle virtual CAN
> interfaces too.

Ack. And the can_dev_*enabled_safe() would also go to raw.c. As we so here,
using those functions in a context where of a real device could give strange
results. So I would rather have this hidden in raw.c to reduce the risk of misuse.

>> and the FD
>> and XL MTU checks are dead code.
> Right. For the code sections that have definitely a priv pointer.
> 
>> 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()
> Will re-check and send a v3 patch.


Yours sincerely,
Vincent Mailhol