* [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack
@ 2017-04-22 18:37 Florian Fainelli
2017-04-22 21:31 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-04-22 18:37 UTC (permalink / raw)
To: linux-kernel
Cc: arnd, maksim.salau, Florian Fainelli, Greg Kroah-Hartman,
David Mosberger, Roger Quadros, Wolfram Sang, Oliver Neukum,
Jaejoong Kim, open list:USB SUBSYSTEM
We see a large number of fixes to several drivers to remove the usage of
on-stack buffers feeding into USB transfer functions. Make it easier to spot
the offenders by adding a warning in usb_start_wait_urb() for
urb->transfer_buffer to be located on the stack.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/usb/core/message.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 2184ef40a82a..abefddbe9243 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -8,6 +8,7 @@
#include <linux/pci.h> /* for scatterlist macros */
#include <linux/usb.h>
#include <linux/module.h>
+#include <linux/sched/task_stack.h> /* for object_is_on_stack */
#include <linux/slab.h>
#include <linux/mm.h>
#include <linux/timer.h>
@@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
unsigned long expire;
int retval;
+ WARN_ON(object_is_on_stack(urb->transfer_buffer));
+
init_completion(&ctx.done);
urb->context = &ctx;
urb->actual_length = 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack
2017-04-22 18:37 [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack Florian Fainelli
@ 2017-04-22 21:31 ` Alan Stern
2017-04-23 6:35 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2017-04-22 21:31 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-kernel, arnd, maksim.salau, Greg Kroah-Hartman,
David Mosberger, Roger Quadros, Wolfram Sang, Oliver Neukum,
Jaejoong Kim, open list:USB SUBSYSTEM
On Sat, 22 Apr 2017, Florian Fainelli wrote:
> We see a large number of fixes to several drivers to remove the usage of
> on-stack buffers feeding into USB transfer functions. Make it easier to spot
> the offenders by adding a warning in usb_start_wait_urb() for
> urb->transfer_buffer to be located on the stack.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/usb/core/message.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 2184ef40a82a..abefddbe9243 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -8,6 +8,7 @@
> #include <linux/pci.h> /* for scatterlist macros */
> #include <linux/usb.h>
> #include <linux/module.h>
> +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
> #include <linux/slab.h>
> #include <linux/mm.h>
> #include <linux/timer.h>
> @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
> unsigned long expire;
> int retval;
>
> + WARN_ON(object_is_on_stack(urb->transfer_buffer));
> +
> init_completion(&ctx.done);
> urb->context = &ctx;
> urb->actual_length = 0;
Does this really help? I mean, don't we already get warnings when
the host controller drivers try to map on-stack buffers for DMA?
The only difference is that one wouldn't have to read as far back into
the stack trace. But that hardly seems like an important
consideration.
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack
2017-04-22 21:31 ` Alan Stern
@ 2017-04-23 6:35 ` Greg Kroah-Hartman
2017-04-23 16:01 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2017-04-23 6:35 UTC (permalink / raw)
To: Alan Stern
Cc: Florian Fainelli, linux-kernel, arnd, maksim.salau,
David Mosberger, Roger Quadros, Wolfram Sang, Oliver Neukum,
Jaejoong Kim, open list:USB SUBSYSTEM
On Sat, Apr 22, 2017 at 05:31:27PM -0400, Alan Stern wrote:
> On Sat, 22 Apr 2017, Florian Fainelli wrote:
>
> > We see a large number of fixes to several drivers to remove the usage of
> > on-stack buffers feeding into USB transfer functions. Make it easier to spot
> > the offenders by adding a warning in usb_start_wait_urb() for
> > urb->transfer_buffer to be located on the stack.
> >
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > drivers/usb/core/message.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > index 2184ef40a82a..abefddbe9243 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -8,6 +8,7 @@
> > #include <linux/pci.h> /* for scatterlist macros */
> > #include <linux/usb.h>
> > #include <linux/module.h>
> > +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
> > #include <linux/slab.h>
> > #include <linux/mm.h>
> > #include <linux/timer.h>
> > @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
> > unsigned long expire;
> > int retval;
> >
> > + WARN_ON(object_is_on_stack(urb->transfer_buffer));
> > +
> > init_completion(&ctx.done);
> > urb->context = &ctx;
> > urb->actual_length = 0;
>
> Does this really help? I mean, don't we already get warnings when
> the host controller drivers try to map on-stack buffers for DMA?
>
> The only difference is that one wouldn't have to read as far back into
> the stack trace. But that hardly seems like an important
> consideration.
I don't think this will show up if you don't have the VMALLOC_STACKS
option enabled (or whatever it is), so this warning is good to have. I
didn't know we had that macro, as the USB stack has always required this
due to some platforms needing it, just not the "mainstream" ones...
I'll take this for the next kernel (after 4.12-rc1) and see what shakes
out in linux-next...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack
2017-04-23 6:35 ` Greg Kroah-Hartman
@ 2017-04-23 16:01 ` Alan Stern
2017-04-23 16:54 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2017-04-23 16:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Florian Fainelli, linux-kernel, arnd, maksim.salau,
David Mosberger, Roger Quadros, Wolfram Sang, Oliver Neukum,
Jaejoong Kim, open list:USB SUBSYSTEM
On Sun, 23 Apr 2017, Greg Kroah-Hartman wrote:
> On Sat, Apr 22, 2017 at 05:31:27PM -0400, Alan Stern wrote:
> > On Sat, 22 Apr 2017, Florian Fainelli wrote:
> >
> > > We see a large number of fixes to several drivers to remove the usage of
> > > on-stack buffers feeding into USB transfer functions. Make it easier to spot
> > > the offenders by adding a warning in usb_start_wait_urb() for
> > > urb->transfer_buffer to be located on the stack.
> > >
> > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > ---
> > > drivers/usb/core/message.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > > index 2184ef40a82a..abefddbe9243 100644
> > > --- a/drivers/usb/core/message.c
> > > +++ b/drivers/usb/core/message.c
> > > @@ -8,6 +8,7 @@
> > > #include <linux/pci.h> /* for scatterlist macros */
> > > #include <linux/usb.h>
> > > #include <linux/module.h>
> > > +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
> > > #include <linux/slab.h>
> > > #include <linux/mm.h>
> > > #include <linux/timer.h>
> > > @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
> > > unsigned long expire;
> > > int retval;
> > >
> > > + WARN_ON(object_is_on_stack(urb->transfer_buffer));
> > > +
> > > init_completion(&ctx.done);
> > > urb->context = &ctx;
> > > urb->actual_length = 0;
> >
> > Does this really help? I mean, don't we already get warnings when
> > the host controller drivers try to map on-stack buffers for DMA?
> >
> > The only difference is that one wouldn't have to read as far back into
> > the stack trace. But that hardly seems like an important
> > consideration.
>
> I don't think this will show up if you don't have the VMALLOC_STACKS
> option enabled (or whatever it is), so this warning is good to have. I
> didn't know we had that macro, as the USB stack has always required this
> due to some platforms needing it, just not the "mainstream" ones...
In that case, it would be better to move the warning to a central place
where it will always get triggered, such as map_urb_for_dma(). As it
is, the patch will only issue a warning for callers of usb_bulk_msg(),
usb_interrupt_msg(), or usb_control_msg().
Alan Stern
> I'll take this for the next kernel (after 4.12-rc1) and see what shakes
> out in linux-next...
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack
2017-04-23 16:01 ` Alan Stern
@ 2017-04-23 16:54 ` Florian Fainelli
2017-04-24 0:03 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-04-23 16:54 UTC (permalink / raw)
To: Alan Stern, Greg Kroah-Hartman
Cc: linux-kernel, arnd, maksim.salau, David Mosberger, Roger Quadros,
Wolfram Sang, Oliver Neukum, Jaejoong Kim,
open list:USB SUBSYSTEM
On 04/23/2017 09:01 AM, Alan Stern wrote:
> On Sun, 23 Apr 2017, Greg Kroah-Hartman wrote:
>
>> On Sat, Apr 22, 2017 at 05:31:27PM -0400, Alan Stern wrote:
>>> On Sat, 22 Apr 2017, Florian Fainelli wrote:
>>>
>>>> We see a large number of fixes to several drivers to remove the usage of
>>>> on-stack buffers feeding into USB transfer functions. Make it easier to spot
>>>> the offenders by adding a warning in usb_start_wait_urb() for
>>>> urb->transfer_buffer to be located on the stack.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>> drivers/usb/core/message.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>>>> index 2184ef40a82a..abefddbe9243 100644
>>>> --- a/drivers/usb/core/message.c
>>>> +++ b/drivers/usb/core/message.c
>>>> @@ -8,6 +8,7 @@
>>>> #include <linux/pci.h> /* for scatterlist macros */
>>>> #include <linux/usb.h>
>>>> #include <linux/module.h>
>>>> +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
>>>> #include <linux/slab.h>
>>>> #include <linux/mm.h>
>>>> #include <linux/timer.h>
>>>> @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
>>>> unsigned long expire;
>>>> int retval;
>>>>
>>>> + WARN_ON(object_is_on_stack(urb->transfer_buffer));
>>>> +
>>>> init_completion(&ctx.done);
>>>> urb->context = &ctx;
>>>> urb->actual_length = 0;
>>>
>>> Does this really help? I mean, don't we already get warnings when
>>> the host controller drivers try to map on-stack buffers for DMA?
>>>
>>> The only difference is that one wouldn't have to read as far back into
>>> the stack trace. But that hardly seems like an important
>>> consideration.
>>
>> I don't think this will show up if you don't have the VMALLOC_STACKS
>> option enabled (or whatever it is), so this warning is good to have. I
>> didn't know we had that macro, as the USB stack has always required this
>> due to some platforms needing it, just not the "mainstream" ones...
>
> In that case, it would be better to move the warning to a central place
> where it will always get triggered, such as map_urb_for_dma(). As it
> is, the patch will only issue a warning for callers of usb_bulk_msg(),
> usb_interrupt_msg(), or usb_control_msg().
map_urb_for_dma() seems like a better location indeed, let me submit a
v2 with that then.
Most HCDs implementation that define a map_urb_for_dma function pointer
also call usb_hcd_map_urb_for_dma() (except for max3421-hcd.c) so would
it be better to move the is_object_on_stack() check in
usb_hcd_map_urb_for_dma() for consistency with the existing
is_vmalloc_addr() check?
Thanks!
NB: initially, I was looking for a compile-time warning, and first there
does not seem to be a warning available in GCC that could tell you if a
variable is on the stack (why would there be?) and second, we'd have to
annotate functions that require some of their arguments to come from the
heap anyway. The amount of work needed to annotate and fix these
build-time warnings versus looking at reports and fixing warnings
end-ups being pretty much the same anyway.
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack
2017-04-23 16:54 ` Florian Fainelli
@ 2017-04-24 0:03 ` Alan Stern
0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2017-04-24 0:03 UTC (permalink / raw)
To: Florian Fainelli
Cc: Greg Kroah-Hartman, linux-kernel, arnd, maksim.salau,
David Mosberger, Roger Quadros, Wolfram Sang, Oliver Neukum,
Jaejoong Kim, open list:USB SUBSYSTEM
On Sun, 23 Apr 2017, Florian Fainelli wrote:
> > In that case, it would be better to move the warning to a central place
> > where it will always get triggered, such as map_urb_for_dma(). As it
> > is, the patch will only issue a warning for callers of usb_bulk_msg(),
> > usb_interrupt_msg(), or usb_control_msg().
>
> map_urb_for_dma() seems like a better location indeed, let me submit a
> v2 with that then.
>
> Most HCDs implementation that define a map_urb_for_dma function pointer
> also call usb_hcd_map_urb_for_dma() (except for max3421-hcd.c) so would
> it be better to move the is_object_on_stack() check in
> usb_hcd_map_urb_for_dma() for consistency with the existing
> is_vmalloc_addr() check?
Or maybe move the is_vmalloc_addr() check to map_urb_for_dma().
The only issue is that some weird HCD might sometimes not need to do
the DMA mapping (it might use PIO for some types of transfer). In
those cases we don't want to issue the warning. So perhaps
usb_hcd_map_urb_for_dma() would be better for both checks.
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-24 0:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-22 18:37 [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack Florian Fainelli
2017-04-22 21:31 ` Alan Stern
2017-04-23 6:35 ` Greg Kroah-Hartman
2017-04-23 16:01 ` Alan Stern
2017-04-23 16:54 ` Florian Fainelli
2017-04-24 0:03 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox