On 15.07.2022 12:53:14, 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));
ACK. I was thinking of the 2nd option with the struct_size().
> 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.
It just feels wrong to have a pointer to a struct canxl_frame that's
only backed half by memory. I haven't followed the flex array discussion
in great detail, but it opens a whole class of errors if arbitrary
kernel memory can be accessed with struct canxl_frame::data using
seemingly valid array indices.
> > > 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.
We don't have to use the same data structure in user space and in the
kernel.
> > 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).
struct_size() is not exported to user space, but I think this could be
changed.
> 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.
How are raw Ethernet frames handled in user space?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
