[PATCH -mm] drivers/usb/core/config.c: kzalloc(0,..)

[PATCH -mm] drivers/usb/core/config.c: kzalloc(0,..)

[Dan Kruchinin]

The following patch fixes such SLUB report(when someone tries to
allocate 0 bytes):
--
May  8 00:19:15 midgard kernel: [   21.933467] BUG: at
include/linux/slub_def.h:88 kmalloc_index()
May  8 00:19:15 midgard kernel: [   21.933470]
[show_registers+410/736] show_trace_log_lvl+0x1a/0x30
May  8 00:19:15 midgard kernel: [   21.933478]
[print_trace_warning_symbol+50/64] show_trace+0x12/0x20
May  8 00:19:15 midgard kernel: [   21.933482]  [fixup_irqs+38/192]
dump_stack+0x16/0x20
May  8 00:19:15 midgard kernel: [   21.933485]  [do_lookup+195/400]
get_slab+0x213/0x230
May  8 00:19:15 midgard kernel: [   21.933489]  [do_lookup+309/400]
__kmalloc_track_caller+0x15/0x40
May  8 00:19:15 midgard kernel: [   21.933493]  [__vunmap+25/240]
__kzalloc+0x19/0x50
May  8 00:19:15 midgard kernel: [   21.933498]  [<df05778e>]
usb_parse_configuration+0x85e/0xe70 [usbcore]
May  8 00:19:15 midgard kernel: [   21.933520]  [<df057fcb>]
usb_get_configuration+0x12b/0x450 [usbcore]
May  8 00:19:15 midgard kernel: [   21.933535]  [<df04ed87>]
usb_new_device+0x17/0x1c0 [usbcore]
May  8 00:19:15 midgard kernel: [   21.933550]  [<df05073a>]
hub_thread+0x79a/0xfd0 [usbcore]
May  8 00:19:15 midgard kernel: [   21.933564]
[run_posix_cpu_timers+1218/2064] kthread+0x42/0x70
May  8 00:19:15 midgard kernel: [   21.933569]  [math_error+87/240]
kernel_thread_helper+0x7/0x10
May  8 00:19:15 midgard kernel: [   21.933572]  =======================
--

The problem was in drivers/usb/core/config.c in function usb_parse_interface:
---
num_ep = num_ep_orig = alt->desc.bNumEndpoints;
...
len = sizeof(struct usb_host_endpoint) * num_ep;
alt->endpoint = kzalloc(len, GFP_KERNEL);
---

num_ep can be 0, as it was in my case, so following patch makes this
situation more obvious
and clear.

--------------
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index bfb3731..4db6b21 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -185,7 +185,10 @@ static int usb_parse_interface(struct device
*ddev, int cfgno,
 		num_ep = USB_MAXENDPOINTS;
 	}

-	len = sizeof(struct usb_host_endpoint) * num_ep;
+	len = sizeof(struct usb_host_endpoint);
+	if (num_ep > 0)
+	  len *= num_ep;
+	
 	alt->endpoint = kzalloc(len, GFP_KERNEL);
 	if (!alt->endpoint)
 		return -ENOMEM;
---------------

Dan Kruchinin.

Re: [PATCH -mm] drivers/usb/core/config.c: kzalloc(0,..)

[Jiri Slaby]

Dan Kruchinin napsal(a):
> The following patch fixes such SLUB report(when someone tries to
> allocate 0 bytes):
> -- 
> May  8 00:19:15 midgard kernel: [   21.933467] BUG: at
> include/linux/slub_def.h:88 kmalloc_index()
> May  8 00:19:15 midgard kernel: [   21.933470]
> [show_registers+410/736] show_trace_log_lvl+0x1a/0x30
> May  8 00:19:15 midgard kernel: [   21.933478]
> [print_trace_warning_symbol+50/64] show_trace+0x12/0x20
> May  8 00:19:15 midgard kernel: [   21.933482]  [fixup_irqs+38/192]
> dump_stack+0x16/0x20
> May  8 00:19:15 midgard kernel: [   21.933485]  [do_lookup+195/400]
> get_slab+0x213/0x230
> May  8 00:19:15 midgard kernel: [   21.933489]  [do_lookup+309/400]
> __kmalloc_track_caller+0x15/0x40
> May  8 00:19:15 midgard kernel: [   21.933493]  [__vunmap+25/240]
> __kzalloc+0x19/0x50
> May  8 00:19:15 midgard kernel: [   21.933498]  [<df05778e>]
> usb_parse_configuration+0x85e/0xe70 [usbcore]
> May  8 00:19:15 midgard kernel: [   21.933520]  [<df057fcb>]
> usb_get_configuration+0x12b/0x450 [usbcore]
> May  8 00:19:15 midgard kernel: [   21.933535]  [<df04ed87>]
> usb_new_device+0x17/0x1c0 [usbcore]
> May  8 00:19:15 midgard kernel: [   21.933550]  [<df05073a>]
> hub_thread+0x79a/0xfd0 [usbcore]
> May  8 00:19:15 midgard kernel: [   21.933564]
> [run_posix_cpu_timers+1218/2064] kthread+0x42/0x70
> May  8 00:19:15 midgard kernel: [   21.933569]  [math_error+87/240]
> kernel_thread_helper+0x7/0x10
> May  8 00:19:15 midgard kernel: [   21.933572]  =======================
> -- 
> 
> The problem was in drivers/usb/core/config.c in function
> usb_parse_interface:
> ---
> num_ep = num_ep_orig = alt->desc.bNumEndpoints;
> ...
> len = sizeof(struct usb_host_endpoint) * num_ep;
> alt->endpoint = kzalloc(len, GFP_KERNEL);
> ---
> 
> num_ep can be 0, as it was in my case, so following patch makes this
> situation more obvious
> and clear.
> 
> --------------
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index bfb3731..4db6b21 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -185,7 +185,10 @@ static int usb_parse_interface(struct device
> *ddev, int cfgno,
>         num_ep = USB_MAXENDPOINTS;
>     }
> 
> -    len = sizeof(struct usb_host_endpoint) * num_ep;
> +    len = sizeof(struct usb_host_endpoint);
> +    if (num_ep > 0)
> +      len *= num_ep;
> +   
>     alt->endpoint = kzalloc(len, GFP_KERNEL);
>     if (!alt->endpoint)
>         return -ENOMEM;
> ---------------

I don't think, this is correct, but let USB people make a decision (always CC
subsys maintainers plus people who might be involved).

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: [PATCH -mm] drivers/usb/core/config.c: kzalloc(0,..)

[Greg KH]

On Tue, May 08, 2007 at 07:03:08AM +0400, Dan Kruchinin wrote:
>  The following patch fixes such SLUB report(when someone tries to
>  allocate 0 bytes):
>  --
>  May  8 00:19:15 midgard kernel: [   21.933467] BUG: at
>  include/linux/slub_def.h:88 kmalloc_index()
>  May  8 00:19:15 midgard kernel: [   21.933470]
>  [show_registers+410/736] show_trace_log_lvl+0x1a/0x30
>  May  8 00:19:15 midgard kernel: [   21.933478]
>  [print_trace_warning_symbol+50/64] show_trace+0x12/0x20
>  May  8 00:19:15 midgard kernel: [   21.933482]  [fixup_irqs+38/192]
>  dump_stack+0x16/0x20
>  May  8 00:19:15 midgard kernel: [   21.933485]  [do_lookup+195/400]
>  get_slab+0x213/0x230
>  May  8 00:19:15 midgard kernel: [   21.933489]  [do_lookup+309/400]
>  __kmalloc_track_caller+0x15/0x40
>  May  8 00:19:15 midgard kernel: [   21.933493]  [__vunmap+25/240]
>  __kzalloc+0x19/0x50
>  May  8 00:19:15 midgard kernel: [   21.933498]  [<df05778e>]
>  usb_parse_configuration+0x85e/0xe70 [usbcore]
>  May  8 00:19:15 midgard kernel: [   21.933520]  [<df057fcb>]
>  usb_get_configuration+0x12b/0x450 [usbcore]
>  May  8 00:19:15 midgard kernel: [   21.933535]  [<df04ed87>]
>  usb_new_device+0x17/0x1c0 [usbcore]
>  May  8 00:19:15 midgard kernel: [   21.933550]  [<df05073a>]
>  hub_thread+0x79a/0xfd0 [usbcore]
>  May  8 00:19:15 midgard kernel: [   21.933564]
>  [run_posix_cpu_timers+1218/2064] kthread+0x42/0x70
>  May  8 00:19:15 midgard kernel: [   21.933569]  [math_error+87/240]
>  kernel_thread_helper+0x7/0x10
>  May  8 00:19:15 midgard kernel: [   21.933572]  =======================
>  --
> 
>  The problem was in drivers/usb/core/config.c in function 
>  usb_parse_interface:
>  ---
>  num_ep = num_ep_orig = alt->desc.bNumEndpoints;
>  ...
>  len = sizeof(struct usb_host_endpoint) * num_ep;
>  alt->endpoint = kzalloc(len, GFP_KERNEL);
>  ---
> 
>  num_ep can be 0, as it was in my case, so following patch makes this
>  situation more obvious
>  and clear.

Well, the whitespace was damaged, but the idea looks sane.  Care to
respin this?

However, what kind of device do you have that you have no endpoints for
an interface?  Why have the interface then?

thanks,

greg k-h

Re: [linux-usb-devel] [PATCH -mm] drivers/usb/core/config.c: kzalloc(0, ..)

[Alan Stern]

On Tue, 8 May 2007, Greg KH wrote:

> >  The problem was in drivers/usb/core/config.c in function 
> >  usb_parse_interface:
> >  ---
> >  num_ep = num_ep_orig = alt->desc.bNumEndpoints;
> >  ...
> >  len = sizeof(struct usb_host_endpoint) * num_ep;
> >  alt->endpoint = kzalloc(len, GFP_KERNEL);
> >  ---
> > 
> >  num_ep can be 0, as it was in my case, so following patch makes this
> >  situation more obvious
> >  and clear.

How about instead just doing:

+	num_ep = max(num_ep, 1);
 	len = sizeof(struct usb_host_endpoint) * num_ep;

Also, wasn't it true at one point that it was legal to call kmalloc() with 
a length of 0?  ISTR seeing somewhere that it's true for regular malloc().

Alan Stern

Re: [linux-usb-devel] [PATCH -mm] drivers/usb/core/config.c: kzalloc(0, ..)

[Randy Dunlap]

On Tue, 8 May 2007 11:57:07 -0400 (EDT) Alan Stern wrote:

> On Tue, 8 May 2007, Greg KH wrote:
> 
> > >  The problem was in drivers/usb/core/config.c in function 
> > >  usb_parse_interface:
> > >  ---
> > >  num_ep = num_ep_orig = alt->desc.bNumEndpoints;
> > >  ...
> > >  len = sizeof(struct usb_host_endpoint) * num_ep;
> > >  alt->endpoint = kzalloc(len, GFP_KERNEL);
> > >  ---
> > > 
> > >  num_ep can be 0, as it was in my case, so following patch makes this
> > >  situation more obvious
> > >  and clear.
> 
> How about instead just doing:
> 
> +	num_ep = max(num_ep, 1);
>  	len = sizeof(struct usb_host_endpoint) * num_ep;
> 
> Also, wasn't it true at one point that it was legal to call kmalloc() with 
> a length of 0?  ISTR seeing somewhere that it's true for regular malloc().

kmalloc(0) was legal with CONFIG_SLAB=y.  However, there is now
something called SLUB, which just returned an error when size == 0.
It has recently been modified to mirror the SLAB behavior but also
do a stack dump so that "bad" callers can be fixed.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [linux-usb-devel] [PATCH -mm] drivers/usb/core/config.c: kzalloc(0, ..)

[Dan Kruchinin]

On 5/8/07, Randy Dunlap <rdunlap@xenotime.net> wrote:
> On Tue, 8 May 2007 11:57:07 -0400 (EDT) Alan Stern wrote:
>
> > On Tue, 8 May 2007, Greg KH wrote:
> >
> > > >  The problem was in drivers/usb/core/config.c in function
> > > >  usb_parse_interface:
> > > >  ---
> > > >  num_ep = num_ep_orig = alt->desc.bNumEndpoints;
> > > >  ...
> > > >  len = sizeof(struct usb_host_endpoint) * num_ep;
> > > >  alt->endpoint = kzalloc(len, GFP_KERNEL);
> > > >  ---
> > > >
> > > >  num_ep can be 0, as it was in my case, so following patch makes this
> > > >  situation more obvious
> > > >  and clear.
> >
> > How about instead just doing:
> >
> > +     num_ep = max(num_ep, 1);
> >       len = sizeof(struct usb_host_endpoint) * num_ep;
> >
> > Also, wasn't it true at one point that it was legal to call kmalloc() with
> > a length of 0?  ISTR seeing somewhere that it's true for regular malloc().
>
> kmalloc(0) was legal with CONFIG_SLAB=y.  However, there is now
> something called SLUB, which just returned an error when size == 0.

SLUB works correctly with kmalloc(0) too, but it calls
WARN_ON_ONCE(size == 0);
in include/linux/slub_def.h: kmalloc_index.

btw: as I know when kmalloc(0) both slub and slab allocate the
smallest possible size. Can this size be smaller than sizeof(struct
usb_host_endpoint)? If it is, may it be a problem?

thanks.

Dan Kruchinin.

> It has recently been modified to mirror the SLAB behavior but also
> do a stack dump so that "bad" callers can be fixed.
>