Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1045877AbdDWQB0 (ORCPT ); Sun, 23 Apr 2017 12:01:26 -0400 Received: from netrider.rowland.org ([192.131.102.5]:40857 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1045857AbdDWQBQ (ORCPT ); Sun, 23 Apr 2017 12:01:16 -0400 Date: Sun, 23 Apr 2017 12:01:14 -0400 (EDT) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: Greg Kroah-Hartman cc: Florian Fainelli , , , , David Mosberger , Roger Quadros , Wolfram Sang , Oliver Neukum , Jaejoong Kim , "open list:USB SUBSYSTEM" Subject: Re: [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack In-Reply-To: <20170423063518.GA9827@kroah.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2442 Lines: 62 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 > > > --- > > > 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 /* for scatterlist macros */ > > > #include > > > #include > > > +#include /* for object_is_on_stack */ > > > #include > > > #include > > > #include > > > @@ -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