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

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

[Oliver Hartkopp]

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)
+
+	/* 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 (priv)
+		return !((priv->ctrlmode & MIXED_MODE) == CAN_CTRLMODE_XL);
+
+	/* 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;
+	}
+
 	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)
-- 
2.47.3

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

[Vincent Mailhol]

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

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

[Oliver Hartkopp]

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.

> 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 ...

> 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.

>> +	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.

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?

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

> 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.

Best regards,
Oliver

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

[Vincent Mailhol]

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

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

[Oliver Hartkopp]

Hi Vincent,

On 10.11.25 22:53, Vincent Mailhol wrote:

> 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.

Agreed. I sent a patch for raw.c only:

https://lore.kernel.org/linux-can/20251111185152.23193-1-socketcan@hartkopp.net/T/#u

But I wonder how many checks are really needed in can_dev_dropped_skb() 
as it later calls can_dropped_invalid_skb().

We should try not to check the same conditions multiple times.

Best regards,
Oliver

ps. can_dev_err_signal_is_disabled() is already using the FD/XL 
mixed-mode detection as I hope we will drop CAN_CTRLMODE_XL_ERR_SIGNAL ;-)