On 15.07.22 05:53, Vincent Mailhol wrote:
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).
But then you would have to name it struct canxlhdr or canxl_hdr to
follow this pattern, right?
And this is my other problem. The struct canxl_frame should be able to
contain a CAN XL frame (as can[fd]_frame do).
I'm fine with introducing e.g. a
struct canxl_hdr {
canid_t prio; /* 11 bit priority for arbitration (canid_t) */
__u8 flags; /* additional flags for CAN XL */
__u8 sdt; /* SDU (service data unit) type */
__u16 len; /* frame payload length in byte */
__u32 af; /* acceptance field */
__u8 data[];
};
and propose the suggested use-patterns.
But I just don't feel good to give up the struct canxl_frame analogue to
can[fd]_frame.
Best regards,
Oliver
ps. can we perhaps put canxl_frame and canxl_hdr in some union structure
that does not look ugly?
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
