| Message ID | 20201102173946.13800-1-anant.thazhemadam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API | expand |
On Mon, 2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote: > 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. > > 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> So you're changing the semantics without updating the callers? I'm confused. Is this supposed to be applied to some tree which already has the callers fixed? At a quick scan at least drivers/net/usb/plusb.c* would get confused as it compares the return value to zero and 0 used to mean "nothing transferred", now it means "all good", no? * I haven't looked at all the other callers
On 05/11/20 5:54 am, Jakub Kicinski wrote: > On Mon, 2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote: >> 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. >> >> 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> > So you're changing the semantics without updating the callers? > > I'm confused. > > Is this supposed to be applied to some tree which already has the > callers fixed? > > At a quick scan at least drivers/net/usb/plusb.c* would get confused > as it compares the return value to zero and 0 used to mean "nothing > transferred", now it means "all good", no? > > * I haven't looked at all the other callers I see. I checked most of the callers that directly called the functions, but it seems to have slipped my mind that these callers were also wrappers, and to check the callers for these wrapper. I apologize for the oversight. I'll perform a more in-depth analysis of all the callers, fix this mistake, and send in a patch series instead, that update all the callers too. Would that be alright? Thank you for your time. Thanks, Anant
On Thu, 5 Nov 2020 07:56:08 +0530 Anant Thazhemadam wrote: > On 05/11/20 5:54 am, Jakub Kicinski wrote: > > On Mon, 2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote: > >> 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. > >> > >> 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> > > So you're changing the semantics without updating the callers? > > > > I'm confused. > > > > Is this supposed to be applied to some tree which already has the > > callers fixed? > > > > At a quick scan at least drivers/net/usb/plusb.c* would get confused > > as it compares the return value to zero and 0 used to mean "nothing > > transferred", now it means "all good", no? > > > > * I haven't looked at all the other callers > > I see. I checked most of the callers that directly called the functions, > but it seems to have slipped my mind that these callers were also > wrappers, and to check the callers for these wrapper. > I apologize for the oversight. > I'll perform a more in-depth analysis of all the callers, fix this mistake, > and send in a patch series instead, that update all the callers too. > Would that be alright? Yes. Probably best if you rename the existing function as first patch, then add a new one with the old name using usb_control_msg_{send|recv}() then switch the callers one by one, and finally remove the old renamed function.
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index bf6c58240bd4..b2df3417a41c 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(). 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. 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 v3: * Aligned continuation lines after the opening brackets Changes in v2: * Fix build error drivers/net/usb/usbnet.c | 52 ++++++++-------------------------------- 1 file changed, 10 insertions(+), 42 deletions(-)