netdev - Re: [PATCH net-next v2 2/2] net: mctp: Add MCTP USB transport driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0fddf411bfb13c46703286381b81bd64fda4ac45.camel@codeconstruct.com.au>
Date: Mon, 17 Feb 2025 16:40:49 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Matt Johnston <matt@...econstruct.com.au>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, Andrew Lunn <andrew+netdev@...n.ch>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo
 Abeni <pabeni@...hat.com>,  netdev@...r.kernel.org,
 linux-usb@...r.kernel.org, Santosh Puranik <spuranik@...dia.com>
Subject: Re: [PATCH net-next v2 2/2] net: mctp: Add MCTP USB transport driver

Hi Jakub,

Thanks for the review. Comments inline.

> > +       __u8 ep_in;
> > +       __u8 ep_out;
> 
> same nit about u8 as on the header

Ack, have changed, as well as on the header.

> Letter for letter dev_dstats_tx_dropped() ?
[...]
> And this dev_dstats_tx_add() ?
[...]
> dev_dstats_rx_add()

Neat, thanks!

> > +static netdev_tx_t mctp_usb_start_xmit(struct sk_buff *skb,
> > +                                      struct net_device *dev)
> > +{
> > +       struct mctp_usb *mctp_usb = netdev_priv(dev);
> > +       struct mctp_usb_hdr *hdr;
> > +       unsigned int plen;
> > +       struct urb *urb;
> > +       int rc;
> > +
> > +       plen = skb->len;
> > +
> > +       if (plen + sizeof(*hdr) > MCTP_USB_XFER_SIZE)
> > +               goto err_drop;
> > +
> > +       hdr = skb_push(skb, sizeof(*hdr));
> 
> Hm, I guess MCTP may have its own rules but technically you should
> call skb_cow_head() before you start writing to the header buffer.

We currently have ensured that we have headroom when the skb had been
allocated. However, things will get a bit more complex when we introduce
proper forwarding, so I will add the skb_cow_head() for v3 (and
introduce it on the other drivers as we go...)

> > +       if (skb)
> > +               kfree_skb(skb);
> > +
> > +       mctp_usb_rx_queue(mctp_usb, GFP_ATOMIC);
> 
> What if we fail to allocate an skb ?
> Admittedly the buffers are relatively small but if the allocation
> fails we'd get stuck, no more packets will ever be received, right?
> May be safer to allocate the skb first, and if it fails reuse the
> skb that just completed (effectively discarding the incoming packets
> until a replacement buffer can be allocated).

I think we can do a little better, and defer retries to a non-atomic
context instead. This means we have a chance of flow control over the
IN transfers from the device too, rather than dropping everything.

I'll implement that for v3.

> > +       dev->hard_header_len = sizeof(struct mctp_usb_hdr);
> > +       dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> > +       dev->addr_len = 0;
> > +       dev->flags = IFF_NOARP;
> > +       dev->netdev_ops = &mctp_usb_netdev_ops;
> > +       dev->needs_free_netdev = false;
> 
> Is there a reason to set this to false?
> dev memory is guaranteed to be zero'ed out.

.. only because I had previously had it as true before the usb
disconnect was implemented. With that change, I had decided to not
remove it with the justification that it's a little more clear that we
need to do our own free after unregister.

Happy to make this more conventional though, so will remove (as well
as the addr_len assignment) in v3.

Cheers,

Jeremy