| Message ID | 20200923090519.361-4-himadrispandya@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: usb: avoid using usb_control_msg() directly | expand |
Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya: Hi, > Many usage of usb_control_msg() do not have proper error check on return > value leaving scope for bugs on short reads. New usb_control_msg_recv() > and usb_control_msg_send() nicely wraps usb_control_msg() with proper > error check. Hence use the wrappers instead of calling usb_control_msg() > directly. > > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com> Nacked-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/rtl8150.c | 32 ++++++-------------------------- > 1 file changed, 6 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c > index 733f120c852b..e3002b675921 100644 > --- a/drivers/net/usb/rtl8150.c > +++ b/drivers/net/usb/rtl8150.c > @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150"; > */ > static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) > { > - void *buf; > - int ret; > - > - buf = kmalloc(size, GFP_NOIO); GFP_NOIO is used here for a reason. You need to use this helper while in contexts of error recovery and runtime PM. > - if (!buf) > - return -ENOMEM; > - > - ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > - RTL8150_REQ_GET_REGS, RTL8150_REQT_READ, > - indx, 0, buf, size, 500); > - if (ret > 0 && ret <= size) > - memcpy(data, buf, ret); > - kfree(buf); > - return ret; > + return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS, > + RTL8150_REQT_READ, indx, 0, data, > + size, 500); This internally uses kmemdup() with GFP_KERNEL. You cannot make this change. The API does not support it. I am afraid we will have to change the API first, before more such changes are done. I would suggest dropping the whole series for now. Regards Oliver
On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote: > > Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya: > > Hi, > > > Many usage of usb_control_msg() do not have proper error check on return > > value leaving scope for bugs on short reads. New usb_control_msg_recv() > > and usb_control_msg_send() nicely wraps usb_control_msg() with proper > > error check. Hence use the wrappers instead of calling usb_control_msg() > > directly. > > > > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com> > Nacked-by: Oliver Neukum <oneukum@suse.com> > > > --- > > drivers/net/usb/rtl8150.c | 32 ++++++-------------------------- > > 1 file changed, 6 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c > > index 733f120c852b..e3002b675921 100644 > > --- a/drivers/net/usb/rtl8150.c > > +++ b/drivers/net/usb/rtl8150.c > > @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150"; > > */ > > static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) > > { > > - void *buf; > > - int ret; > > - > > - buf = kmalloc(size, GFP_NOIO); > > GFP_NOIO is used here for a reason. You need to use this helper > while in contexts of error recovery and runtime PM. > Understood. Apologies for proposing such a stupid change. > > - if (!buf) > > - return -ENOMEM; > > - > > - ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > > - RTL8150_REQ_GET_REGS, RTL8150_REQT_READ, > > - indx, 0, buf, size, 500); > > - if (ret > 0 && ret <= size) > > - memcpy(data, buf, ret); > > - kfree(buf); > > - return ret; > > + return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS, > > + RTL8150_REQT_READ, indx, 0, data, > > + size, 500); > > This internally uses kmemdup() with GFP_KERNEL. > You cannot make this change. The API does not support it. > I am afraid we will have to change the API first, before more > such changes are done. > > I would suggest dropping the whole series for now. Okay. Thanks for the review. Regards, Himadri > > Regards > Oliver >
Am Mittwoch, den 23.09.2020, 19:36 +0530 schrieb Himadri Pandya: > On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote: > > > > Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya: > > GFP_NOIO is used here for a reason. You need to use this helper > > while in contexts of error recovery and runtime PM. > > > > Understood. Apologies for proposing such a stupid change. Hi, sorry if you concluded that the patch was stupid. That was not my intent. It was the best the API allowed for. If an API makes it easy to make a mistake, the problem is with the API, not the developer. Regards Oliver
On Wed, Sep 23, 2020 at 7:51 PM Oliver Neukum <oneukum@suse.com> wrote: > > Am Mittwoch, den 23.09.2020, 19:36 +0530 schrieb Himadri Pandya: > > On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote: > > > > > > Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya: > > > > GFP_NOIO is used here for a reason. You need to use this helper > > > while in contexts of error recovery and runtime PM. > > > > > > > Understood. Apologies for proposing such a stupid change. > > Hi, > > sorry if you concluded that the patch was stupid. That was not my > intent. It was the best the API allowed for. If an API makes it > easy to make a mistake, the problem is with the API, not the developer. > > Regards > Oliver > I meant that it was stupid to change it without properly understanding the significance of GFP_NOIO in this context. So now, do we re-write the wrapper functions with flag passed as a parameter? Regards, Himadri
On 20-09-23 12:22:37, Oliver Neukum wrote: > Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya: > > Hi, > > > Many usage of usb_control_msg() do not have proper error check on return > > value leaving scope for bugs on short reads. New usb_control_msg_recv() > > and usb_control_msg_send() nicely wraps usb_control_msg() with proper > > error check. Hence use the wrappers instead of calling usb_control_msg() > > directly. > > > > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com> > Nacked-by: Oliver Neukum <oneukum@suse.com> > > > --- > > drivers/net/usb/rtl8150.c | 32 ++++++-------------------------- > > 1 file changed, 6 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c > > index 733f120c852b..e3002b675921 100644 > > --- a/drivers/net/usb/rtl8150.c > > +++ b/drivers/net/usb/rtl8150.c > > @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150"; > > */ > > static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) > > { > > - void *buf; > > - int ret; > > - > > - buf = kmalloc(size, GFP_NOIO); > > GFP_NOIO is used here for a reason. You need to use this helper > while in contexts of error recovery and runtime PM. > > > - if (!buf) > > - return -ENOMEM; > > - > > - ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > > - RTL8150_REQ_GET_REGS, RTL8150_REQT_READ, > > - indx, 0, buf, size, 500); > > - if (ret > 0 && ret <= size) > > - memcpy(data, buf, ret); > > - kfree(buf); > > - return ret; > > + return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS, > > + RTL8150_REQT_READ, indx, 0, data, > > + size, 500); > > This internally uses kmemdup() with GFP_KERNEL. > You cannot make this change. The API does not support it. > I am afraid we will have to change the API first, before more > such changes are done. One possible fix is to add yet another argument to usb_control_msg_recv(), which would be the GFP_XYZ flag to pass on to kmemdup(). Up to Greg, of course. cheers, Petko
Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov: > > This internally uses kmemdup() with GFP_KERNEL. > > You cannot make this change. The API does not support it. > > I am afraid we will have to change the API first, before more > > such changes are done. > > One possible fix is to add yet another argument to usb_control_msg_recv(), which > would be the GFP_XYZ flag to pass on to kmemdup(). Up to Greg, of course. Hi, submitted. The problem is those usages that are very hard to trace. I'd dislike to just slab GFP_NOIO on them for no obvious reason. Regards Oliver
Am Mittwoch, den 23.09.2020, 20:02 +0530 schrieb Himadri Pandya: > I meant that it was stupid to change it without properly understanding > the significance of GFP_NOIO in this context. > > So now, do we re-write the wrapper functions with flag passed as a parameter? Hi, I hope I set you in CC for a patch set doing exactly that. Do not let me or other maintainers discourage you from writing patches. Look at it this way. Had you not written this patch, I would not have looked into the matter. Patches are supposed to be reviewed. If you want additional information, just ask. We do not want people discouraged from writing substantial patches. Regards Oliver
On 20-09-24 13:09:05, Oliver Neukum wrote: > Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov: > > > One possible fix is to add yet another argument to usb_control_msg_recv(), > > which would be the GFP_XYZ flag to pass on to kmemdup(). Up to Greg, of > > course. > > submitted. The problem is those usages that are very hard to trace. I'd > dislike to just slab GFP_NOIO on them for no obvious reason. Do you mean you submitted a patch for usb_control_msg_recv() (because i don't see it on linux-netdev) or i'm reading this all wrong? Petko
On Thu, Sep 24, 2020 at 06:40:26PM +0300, Petko Manolov wrote: > On 20-09-24 13:09:05, Oliver Neukum wrote: > > Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov: > > > > > One possible fix is to add yet another argument to usb_control_msg_recv(), > > > which would be the GFP_XYZ flag to pass on to kmemdup(). Up to Greg, of > > > course. > > > > submitted. The problem is those usages that are very hard to trace. I'd > > dislike to just slab GFP_NOIO on them for no obvious reason. > > Do you mean you submitted a patch for usb_control_msg_recv() (because i don't > see it on linux-netdev) or i'm reading this all wrong? It's on the linux-usb list: https://lore.kernel.org/r/20200923134348.23862-1-oneukum@suse.com
On Thu, Sep 24, 2020 at 5:06 PM Oliver Neukum <oneukum@suse.com> wrote: > > Am Mittwoch, den 23.09.2020, 20:02 +0530 schrieb Himadri Pandya: > > > I meant that it was stupid to change it without properly understanding > > the significance of GFP_NOIO in this context. > > > > So now, do we re-write the wrapper functions with flag passed as a parameter? > > Hi, > > I hope I set you in CC for a patch set doing exactly that. > Yes. > Do not let me or other maintainers discourage you from writing patches. > Look at it this way. Had you not written this patch, I would not have > looked into the matter. Patches are supposed to be reviewed. > If you want additional information, just ask. We do not want > people discouraged from writing substantial patches. > Understood :). I'll send v2 after the update in API is merged. Thanks, Himadri > Regards > Oliver > >
diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c index 733f120c852b..e3002b675921 100644 --- a/drivers/net/usb/rtl8150.c +++ b/drivers/net/usb/rtl8150.c @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150"; */ static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) { - void *buf; - int ret; - - buf = kmalloc(size, GFP_NOIO); - if (!buf) - return -ENOMEM; - - ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), - RTL8150_REQ_GET_REGS, RTL8150_REQT_READ, - indx, 0, buf, size, 500); - if (ret > 0 && ret <= size) - memcpy(data, buf, ret); - kfree(buf); - return ret; + return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS, + RTL8150_REQT_READ, indx, 0, data, + size, 500); } static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data) { - void *buf; - int ret; - - buf = kmemdup(data, size, GFP_NOIO); - if (!buf) - return -ENOMEM; - - ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), - RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE, - indx, 0, buf, size, 500); - kfree(buf); - return ret; + return usb_control_msg_send(dev->udev, 0, RTL8150_REQ_SET_REGS, + RTL8150_REQT_WRITE, indx, 0, data, + size, 500); } static void async_set_reg_cb(struct urb *urb)
Many usage of usb_control_msg() do not have proper error check on return value leaving scope for bugs on short reads. New usb_control_msg_recv() and usb_control_msg_send() nicely wraps usb_control_msg() with proper error check. Hence use the wrappers instead of calling usb_control_msg() directly. Signed-off-by: Himadri Pandya <himadrispandya@gmail.com> --- drivers/net/usb/rtl8150.c | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-)