Re: [RFC PATCH v3 1/2] can: skb: enforce CANFD_FDF check in can_is_canfd_skb()

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 09.09.25 08:25, Vincent Mailhol wrote:
> On Tue. 9 Sep. 2025 at 03:45, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> The check in can_is_canfd_skb() is about a length check of skb->len and
>> the CAN FD data length. As a skb length of CANFD_MTU can potentially be
>> created with a CAN XL frame with a data length of 60, the length check of
>> the CAN FD data length is used to detect CAN XL frames via its CANXL_XLF
>> flag which exceeds valid CAN FD data length values.
>>
>> To make sure the CANFD_FDF flag can be safely used as a marker for CAN FD
>> frame skbs the bit was set in can_send() and is now also set in
>> raw_check_txframe() to re-use the indroduced can_is_canfd_skb_set_fdf()
>> function. In the RX path alloc_canfd_skb() sets the CANFD_FDF flag.
>>
>> The enforced CANFD_FDF check in can_is_canfd_skb() clears up the potential
>> uncertainty when using the skb->len check with the CANFD_MTU.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   include/linux/can/skb.h | 25 +++++++++++++++++++++++--
>>   net/can/af_can.c        |  7 +------
>>   net/can/raw.c           |  2 +-
>>   3 files changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
>> index 1abc25a8d144..38d036b43280 100644
>> --- a/include/linux/can/skb.h
>> +++ b/include/linux/can/skb.h
>> @@ -111,12 +111,33 @@ static inline bool can_is_can_skb(const struct sk_buff *skb)
>>
>>   static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>>   {
>>          struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>>
>> -       /* the CAN specific type of skb is identified by its data length */
>> -       return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
>> +       if (skb->len != CANFD_MTU || cfd->len > CANFD_MAX_DLEN)
>> +               return false;
>> +
>> +       return cfd->flags & CANFD_FDF;
>> +}
>> +
>> +static inline bool can_is_canfd_skb_set_fdf(const struct sk_buff *skb)
>> +{
>> +       struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> +
>> +       /* The CAN specific type of skb is identified by its data length.
>> +        * A CAN XL frame skb might have a skb->len of CANFD_MTU but the
>> +        * skb would have the CANXL_XLF bit set (0x80 = 128) in the
>> +        * cfd->len field position which would intentionally break the
>> +        * CAN FD length check here. So we can surely tag it as CAN FD.
>> +        */
>> +       if (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN) {
>> +               /* set CAN FD flag for CAN FD frames by default */
>> +               cfd->flags |= CANFD_FDF;
>> +               return true;
>> +       }
>> +
>> +       return false;
>>   }
>>
>>   static inline bool can_is_canxl_skb(const struct sk_buff *skb)
>>   {
>>          const struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index b2387a46794a..0caf75a9e27f 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -207,17 +207,12 @@ int can_send(struct sk_buff *skb, int loop)
>>
>>          if (can_is_canxl_skb(skb)) {
>>                  skb->protocol = htons(ETH_P_CANXL);
>>          } else if (can_is_can_skb(skb)) {
>>                  skb->protocol = htons(ETH_P_CAN);
>> -       } else if (can_is_canfd_skb(skb)) {
>> -               struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> -
>> +       } else if (can_is_canfd_skb_set_fdf(skb)) {
>>                  skb->protocol = htons(ETH_P_CANFD);
>> -
>> -               /* set CAN FD flag for CAN FD frames by default */
>> -               cfd->flags |= CANFD_FDF;
>>          } else {
>>                  goto inval_skb;
>>          }
>>
>>          /* Make sure the CAN frame can pass the selected CAN netdevice. */
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index 76b867d21def..f48b1f3fd6e8 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
>> @@ -886,11 +886,11 @@ static unsigned int raw_check_txframe(struct raw_sock *ro, struct sk_buff *skb,
> 
> With this change, raw_check_txframe() now has a side effect. It will
> set the CANFD_FDF under some conditions. This is weird for a function
> named "check". When I read the code, I expected a check function to
> not have such side effects.
> 
> I would suggest to set the flag in raw_sendmsg(), something like that:
> 
>           txmtu = raw_check_txframe(ro, skb, dev->mtu);
>           if (!txmtu)
>                   goto free_skb;
> 
> +        /* set CAN FD flag for CAN FD frames by default */
> +        if (txmtu == CANFD_MTU) {
> +                struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +
> +                cfd->flags |= CANFD_FDF;
> +        }
> +

Right. I've seen this remark to late for my v4.
Will propose a better solution.

Best regards,
Oliver

>           /* only CANXL: clear/forward/set VCID value */
>           if (txmtu == CANXL_MTU)
>                   raw_put_canxl_vcid(ro, skb);
> 
>>          /* Classical CAN -> no checks for flags and device capabilities */
>>          if (can_is_can_skb(skb))
>>                  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) &&
>> +       if (ro->fd_frames && can_is_canfd_skb_set_fdf(skb) &&
>>              (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
>>                  return CANFD_MTU;
>>
>>          /* CAN XL -> needs to be enabled and a CAN XL device */
>>          if (ro->xl_frames && can_is_canxl_skb(skb) &&