netdev - Re: [PATCH net-next 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: <16ff513ac43a23fb0abfeaf9ff139115d87dbf3a.camel@codeconstruct.com.au>
Date: Fri, 07 Feb 2025 15:45:02 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: Oliver Neukum <oneukum@...e.com>, 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>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-usb@...r.kernel.org, Santosh Puranik
	 <spuranik@...dia.com>
Subject: Re: [PATCH net-next 2/2] net: mctp: Add MCTP USB transport driver

Hi Oliver,

Thanks for taking a look. Some responses too.

> > +               hdr = skb_pull_data(skb, sizeof(*hdr));
> > +               if (!hdr)
> > +                       break;
> > +
> > +               if (be16_to_cpu(hdr->id) != MCTP_USB_DMTF_ID) {
> 
> It would be more efficient to do the conversion on the constant

Compiler should be clever enough for that not to make a difference:

  $ diff -u \
    <(arm-linux-gnueabihf-objdump -d obj/drivers/net/mctp/mctp-usb.o.orig) \
    <(arm-linux-gnueabihf-objdump -d obj/drivers/net/mctp/mctp-usb.o)
  --- /dev/fd/63	2025-02-07 15:32:53.813084894 +0800
  +++ /dev/fd/62	2025-02-07 15:32:53.809084826 +0800
  @@ -1,5 +1,5 @@
   
  -obj/drivers/net/mctp/mctp-usb.o.orig:     file format elf32-littlearm
  +obj/drivers/net/mctp/mctp-usb.o:     file format elf32-littlearm

  Disassembly of section .text:
  $

And endian-converting the header field (rather than the const) seems
more readable to me.

> > +               if (pkt_len < skb->len) {
> > +                       /* more packets may follow - clone to a new
> > +                        * skb to use on the next iteration
> > +                        */
> > +                       skb2 = skb_clone(skb, GFP_ATOMIC);
> > +                       if (skb2) {
> > +                               if (!skb_pull(skb2, pkt_len)) {
> > +                                       kfree_skb(skb2);
> > +                                       skb2 = NULL;
> > +                               }
> > +                       }
> > +                       skb_trim(skb, pkt_len);
> 
> This is functional. Though in terms of algorithm you are copying
> the same data multiple times.

There should be no copy here; they're shared clones of the same buffer.
Or am I missing some situation where they would get unshared?

> > +static int mctp_usb_open(struct net_device *dev)
> > +{
> > +       struct mctp_usb *mctp_usb = netdev_priv(dev);
> > +
> > +       return mctp_usb_rx_queue(mctp_usb);
> 
> This will needlessly use GFP_ATOMIC

It's only the one (first) skb and urb submission, but fair enough. I'll
add a gfp_t argument in a v2.

> > +       SET_NETDEV_DEV(netdev, &intf->dev);
> > +       dev = netdev_priv(netdev);
> > +       dev->netdev = netdev;
> > +       dev->usbdev = usb_get_dev(interface_to_usbdev(intf));
> 
> Taking a reference.
> Where is the corresponding put?

Good catch - we should have one in disconnect(). Coming up in v2 too.

Cheers,


Jeremy