On 14.07.22 22:06, Marc Kleine-Budde wrote:
On 14.07.2022 18:05:39, Oliver Hartkopp wrote:
(..)
+struct sk_buff *alloc_canxl_skb(struct net_device *dev, + struct canxl_frame **cfx, + unsigned int datalen) +{ + struct sk_buff *skb; + + if (datalen < CANXL_MIN_DLEN || datalen > CANXL_MAX_DLEN) + goto out_error; + + skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) + + CANXL_HEAD_SZ + datalen); + if (unlikely(!skb)) + goto out_error; + + skb->protocol = htons(ETH_P_CANXL); + skb->pkt_type = PACKET_BROADCAST; + skb->ip_summed = CHECKSUM_UNNECESSARY; + + skb_reset_mac_header(skb); + skb_reset_network_header(skb); + skb_reset_transport_header(skb); + + can_skb_reserve(skb); + can_skb_prv(skb)->ifindex = dev->ifindex; + can_skb_prv(skb)->skbcnt = 0; + + *cfx = skb_put_zero(skb, CANXL_HEAD_SZ + datalen);Should the CANXL_XLF be set here?
Yes, we can set that bit here directly - for convenience reasons ;-)
I have a bad feeling if we have a struct canxl_frame with a fixed size, but it might not completely be backed by data..... For example, I've updated the gs_usb driver to work with flexible arrays to accommodate the different USB frame length: https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L216 Maybe we should talk to Kees Cook what's best to use here.
I see this struct canxl_frame with 2048 byte of data more as a user space thing.
You can simply read() from the CAN_RAW socket into this struct (as you know it from CAN/CANFD) and it works safely.
That we optimize the length to the really needed length inside the skb and for CAN XL socket read/write operations is on another page for me.
If we *only* had the canxl data structure inside the kernel I would be definitely ok with flexible arrays. The current implementation indeed never allocates space with the sizeof(struct canxl_frame) ...
But I tend to maintain the pattern we introduced for CAN and CAN FD for the user space visible data structures. That is clearer and safe to use by default instead of reading documentation about flexible arrays and how to build some data structure on your own.
Regards, Oliver
