| Message ID | 20201029132256.11793-1-anant.thazhemadam@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API | expand |
On 29/10/20 6:52 pm, Anant Thazhemadam wrote: > Currently, __usbnet_{read|write}_cmd() use usb_control_msg(), > and thus consider potential partial reads/writes being done to > be perfectly valid. > Quite a few callers of usbnet_{read|write}_cmd() don't enforce > checking for partial reads/writes into account either, automatically > assuming that a complete read/write occurs. > > However, the new usb_control_msg_{send|recv}() APIs don't allow partial > reads and writes. > Using the new APIs also relaxes the return value checking that must > be done after usbnet_{read|write}_cmd() is called. > > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> <mailto:anant.thazhemadam@gmail.com> > --- > Changes in v2: > * Fix build error > > This patch has been compile and build tested with a .config file that > was generated using make allyesconfig, and the build error has been > fixed. > Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing, > and would appreciate it if someone could do that. > > drivers/net/usb/usbnet.c | 52 ++++++++-------------------------------- > 1 file changed, 10 insertions(+), 42 deletions(-) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index bf6c58240bd4..2f7c7b7f4047 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change); > static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, > u16 value, u16 index, void *data, u16 size) > { > - void *buf = NULL; > - int err = -ENOMEM; > > netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x" > " value=0x%04x index=0x%04x size=%d\n", > cmd, reqtype, value, index, size); > > - if (size) { > - buf = kmalloc(size, GFP_KERNEL); > - if (!buf) > - goto out; > - } > - > - err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > - cmd, reqtype, value, index, buf, size, > - USB_CTRL_GET_TIMEOUT); > - if (err > 0 && err <= size) { > - if (data) > - memcpy(data, buf, err); > - else > - netdev_dbg(dev->net, > - "Huh? Data requested but thrown away.\n"); > - } > - kfree(buf); > -out: > - return err; > + return usb_control_msg_recv(dev->udev, 0, > + cmd, reqtype, value, index, data, size, > + USB_CTRL_GET_TIMEOUT, GFP_KERNEL); > } > > static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, > u16 value, u16 index, const void *data, > u16 size) > { > - void *buf = NULL; > - int err = -ENOMEM; > - > netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x" > " value=0x%04x index=0x%04x size=%d\n", > cmd, reqtype, value, index, size); > > - if (data) { > - buf = kmemdup(data, size, GFP_KERNEL); > - if (!buf) > - goto out; > - } else { > - if (size) { > - WARN_ON_ONCE(1); > - err = -EINVAL; > - goto out; > - } > - } > - > - err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), > - cmd, reqtype, value, index, buf, size, > - USB_CTRL_SET_TIMEOUT); > - kfree(buf); > + if (size && !data) { > + WARN_ON_ONCE(1); > + return -EINVAL; > + } > > -out: > - return err; > + return usb_control_msg_send(dev->udev, 0, > + cmd, reqtype, value, index, data, size, > + USB_CTRL_SET_TIMEOUT, GFP_KERNEL); > } > > /* I had a v2 prepared and ready but was told to wait for a week before sending it in, since usb_control_msg_{send|recv}() that were being used were not present in the networking tree at the time, and all the trees would be converged by then. So, just to be on the safer side, I waited for two weeks. I checked the net tree, and found the APIs there too (defined in usb.h). However the build seems to fail here, https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/O2BERGN7SYYC6LNOOKNUGPS2IJLDWYT7/ I'm not entirely sure at this point why this is happening, and would appreciate it if someone could take the time to tell me if and how this might be an issue with my patch. Thanks, Anant
On Thu, 29 Oct 2020 18:52:56 +0530 Anant Thazhemadam wrote: > + return usb_control_msg_recv(dev->udev, 0, > + cmd, reqtype, value, index, data, size, > + USB_CTRL_GET_TIMEOUT, GFP_KERNEL); Please align continuation lines after the opening bracket.
On 01/11/20 2:41 am, Jakub Kicinski wrote: > On Thu, 29 Oct 2020 18:52:56 +0530 Anant Thazhemadam wrote: >> + return usb_control_msg_recv(dev->udev, 0, >> + cmd, reqtype, value, index, data, size, >> + USB_CTRL_GET_TIMEOUT, GFP_KERNEL); > Please align continuation lines after the opening bracket. I will do that, and send in a v3 right away. Thanks, Anant
Am Sonntag, den 01.11.2020, 03:05 +0530 schrieb Anant Thazhemadam: > Currently, __usbnet_{read|write}_cmd() use usb_control_msg(). > However, this could lead to potential partial reads/writes being > considered valid, and since most of the callers of > usbnet_{read|write}_cmd() don't take partial reads/writes into account > (only checking for negative error number is done), and this can lead to > issues. > Hi, plesae send this as a post of its own. We cannot take a new set as a reply to an older set. Regards Oliver
On 02/11/20 3:10 pm, Oliver Neukum wrote: > Am Sonntag, den 01.11.2020, 03:05 +0530 schrieb Anant Thazhemadam: >> Currently, __usbnet_{read|write}_cmd() use usb_control_msg(). >> However, this could lead to potential partial reads/writes being >> considered valid, and since most of the callers of >> usbnet_{read|write}_cmd() don't take partial reads/writes into account >> (only checking for negative error number is done), and this can lead to >> issues. >> > Hi, > > plesae send this as a post of its own. We cannot take a new set > as a reply to an older set. > > Regards > Oliver > Got it. I will do that. Thanks, Anant
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index bf6c58240bd4..2f7c7b7f4047 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change); static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value, u16 index, void *data, u16 size) { - void *buf = NULL; - int err = -ENOMEM; netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x" " value=0x%04x index=0x%04x size=%d\n", cmd, reqtype, value, index, size); - if (size) { - buf = kmalloc(size, GFP_KERNEL); - if (!buf) - goto out; - } - - err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), - cmd, reqtype, value, index, buf, size, - USB_CTRL_GET_TIMEOUT); - if (err > 0 && err <= size) { - if (data) - memcpy(data, buf, err); - else - netdev_dbg(dev->net, - "Huh? Data requested but thrown away.\n"); - } - kfree(buf); -out: - return err; + return usb_control_msg_recv(dev->udev, 0, + cmd, reqtype, value, index, data, size, + USB_CTRL_GET_TIMEOUT, GFP_KERNEL); } static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value, u16 index, const void *data, u16 size) { - void *buf = NULL; - int err = -ENOMEM; - netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x" " value=0x%04x index=0x%04x size=%d\n", cmd, reqtype, value, index, size); - if (data) { - buf = kmemdup(data, size, GFP_KERNEL); - if (!buf) - goto out; - } else { - if (size) { - WARN_ON_ONCE(1); - err = -EINVAL; - goto out; - } - } - - err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), - cmd, reqtype, value, index, buf, size, - USB_CTRL_SET_TIMEOUT); - kfree(buf); + if (size && !data) { + WARN_ON_ONCE(1); + return -EINVAL; + } -out: - return err; + return usb_control_msg_send(dev->udev, 0, + cmd, reqtype, value, index, data, size, + USB_CTRL_SET_TIMEOUT, GFP_KERNEL); } /*
Currently, __usbnet_{read|write}_cmd() use usb_control_msg(), and thus consider potential partial reads/writes being done to be perfectly valid. Quite a few callers of usbnet_{read|write}_cmd() don't enforce checking for partial reads/writes into account either, automatically assuming that a complete read/write occurs. However, the new usb_control_msg_{send|recv}() APIs don't allow partial reads and writes. Using the new APIs also relaxes the return value checking that must be done after usbnet_{read|write}_cmd() is called. Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> --- Changes in v2: * Fix build error This patch has been compile and build tested with a .config file that was generated using make allyesconfig, and the build error has been fixed. Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing, and would appreciate it if someone could do that. drivers/net/usb/usbnet.c | 52 ++++++++-------------------------------- 1 file changed, 10 insertions(+), 42 deletions(-)