A recent bug-fix shaded light on possible incorrect use of
usb_control_msg() without proper error check. This resulted in
introducing new usb core api wrapper functions usb_control_msg_send()
and usb_control_msg_recv() by Greg KH. This patch series continue the
clean-up using the new functions.
Himadri Pandya (4):
net: usbnet: use usb_control_msg_recv() and usb_control_msg_send()
net: sierra_net: use usb_control_msg_recv()
net: usb: rtl8150: use usb_control_msg_recv() and
usb_control_msg_send()
net: rndis_host: use usb_control_msg_recv() and usb_control_msg_send()
drivers/net/usb/rndis_host.c | 44 +++++++++++++---------------------
drivers/net/usb/rtl8150.c | 32 +++++--------------------
drivers/net/usb/sierra_net.c | 17 ++++++-------
drivers/net/usb/usbnet.c | 46 ++++--------------------------------
4 files changed, 34 insertions(+), 105 deletions(-)
--
2.17.1
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 <[email protected]>
---
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);
- 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)
--
2.17.1
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 <[email protected]>
Nacked-by: Oliver Neukum <[email protected]>
> ---
> 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 02:35:15PM +0530, Himadri Pandya wrote:
> A recent bug-fix shaded light on possible incorrect use of
> usb_control_msg() without proper error check. This resulted in
> introducing new usb core api wrapper functions usb_control_msg_send()
> and usb_control_msg_recv() by Greg KH. This patch series continue the
> clean-up using the new functions.
>
> Himadri Pandya (4):
> net: usbnet: use usb_control_msg_recv() and usb_control_msg_send()
> net: sierra_net: use usb_control_msg_recv()
> net: usb: rtl8150: use usb_control_msg_recv() and
> usb_control_msg_send()
> net: rndis_host: use usb_control_msg_recv() and usb_control_msg_send()
>
> drivers/net/usb/rndis_host.c | 44 +++++++++++++---------------------
> drivers/net/usb/rtl8150.c | 32 +++++--------------------
> drivers/net/usb/sierra_net.c | 17 ++++++-------
> drivers/net/usb/usbnet.c | 46 ++++--------------------------------
> 4 files changed, 34 insertions(+), 105 deletions(-)
>
Note, all of these depend on a set of patches in my usb tree, so they
should probably wait to be sent to the networking developers until after
5.10-rc1 is out.
thanks,
greg k-h
On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <[email protected]> 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 <[email protected]>
> Nacked-by: Oliver Neukum <[email protected]>
>
> > ---
> > 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 <[email protected]> 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 <[email protected]> wrote:
>
> Am Mittwoch, den 23.09.2020, 19:36 +0530 schrieb Himadri Pandya:
> > On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <[email protected]> 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 <[email protected]>
> Nacked-by: Oliver Neukum <[email protected]>
>
> > ---
> > 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/[email protected]
On Thu, Sep 24, 2020 at 5:06 PM Oliver Neukum <[email protected]> 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
>
>