On Fri. 15 Jul. 2022 at 06:14, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> 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 usings the flexible array member, this would become:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
sizeof(struct canxl_frame) + datalen);
or even:
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
struct_size(*cxl, data, datalen));
This is an illustration of my point that flexible data arrays are more
idiomatic. I find it weird to have to mix sizeof(struct can_skb_priv)
and CANXL_HEAD_SZ in the same expression...
> >> + 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.....
I tried to think hard of what could go wrong with the
data[CANXL_MAX_DLEN] declaration.
The worst I could think of would be some:
int datalen = 64; /* or anything less than CANXL_MAX_DLEN */
struct canxl_frame *cxl1 = malloc(CANXL_HEAD_SZ + datalen);
struct canxl_frame *cxl2 = malloc(CANXL_HEAD_SZ + datalen);
memcpy(cxl1, cxl2, sizeof(*cxl1));
But that example is a bit convoluted. That's why I wrote in my
previous message that I saw no killer arguments against it.
> > 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.
Here, you are making the assumption that the end user will only be
familiar with the CAN(-FD) and not with other concepts.
Building data structures on your own is fairly common, the best
example being the struct iphdr or the struct tcphdr for TCP/IP:
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L86
* https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/tcp.h#L25
(in those examples, it is not a flexible array member but the concept
is basically the same).
I think it is fair to expect from a developer using Berkeley sockets
(what SocketCAN is) to be familiar with this.
In the worst case, the developper who still completely ignore the
documentation and just do sed "s/canfd/canxl/g" on their existing code
base will eventually do this:
write(sock, &cxl, sizeof(canxl));
And that would fail immediately (because sizeof(canxl) <
CANXL_MIN_TU). So I think it is still safe. The real foot gun is when
you can write incorrect code that still works (e.g. buffer overflow).
If it directly fails, people will copy/paste the accepted answer on
stackoverflow and will eventually do the correct:
write(sock, &cxl, sizeof(cxl) + cxl.len);
Finally, for both solutions, user can not do this anymore:
assert(read(sock, &cxl, sizeof(cxl)) == sizeof(cxl));
But instead should do:
assert(read(sock, &cxl, sizeof(cxl)) >= CANXL_MINTU);
So regardless of the solution we use, the developer needs to be aware
to some extent of the variable size (and ignoring the return value of
read() is a bad practice so I won't accept this as a counterargument).
The debate is really on "reusing CAN(-FD) patterns" vs. "doing
idiomatic C". I will not veto the data[CANXL_MAX_DLEN], but I vote for
the flexible array member for the reasons listed here.
Yours sincerely,
Vincent Mailhol
