Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
Hi, Greg Kroah-Hartmanwrites: > On Tue, Apr 25, 2017 at 01:35:33PM +0300, Maksim Salau wrote: >> > + } else if (object_is_on_stack(urb->transfer_buffer)) { >> > + WARN_ONCE(1, "transfer buffer is on stack\n"); >> > + ret = -EAGAIN; >> >} else { >> >> Hi, >> >> Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack, >> kmemdup it and continue with a warning. This will give us both: functional >> drivers (with possibly decreased efficiency in speed and memory footprint) >> and warnings for developers that a particular driver requires attention. > > No, I do not want that, let's fix the drivers. > >> This mode will not affect drivers which obey the rules, but will make >> offenders at least functional. My main concern is that not every user is able >> to detect and report a problem, which prevents drivers from functioning. >> Especially this is a problem for not wide spread devices. >> Due to this users a seeing unusable equipment, but developers are not >> aware of those, even if fixes are trivial. >> >> Such mode has a also a negative effect: if a developer has a device >> with an offending driver, he can miss the warning message, since the driver >> just works. > > Exactly, let's fix the bugs. These have been bugs for 10+ years now, > they should get fixed, it's not complex :) We should probably have a similar patch on drivers/usb/gadget/udc/core.c::usb_gadget_map_request_by_dev() -- balbi
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
Hi, Greg Kroah-Hartman writes: > On Tue, Apr 25, 2017 at 01:35:33PM +0300, Maksim Salau wrote: >> > + } else if (object_is_on_stack(urb->transfer_buffer)) { >> > + WARN_ONCE(1, "transfer buffer is on stack\n"); >> > + ret = -EAGAIN; >> >} else { >> >> Hi, >> >> Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack, >> kmemdup it and continue with a warning. This will give us both: functional >> drivers (with possibly decreased efficiency in speed and memory footprint) >> and warnings for developers that a particular driver requires attention. > > No, I do not want that, let's fix the drivers. > >> This mode will not affect drivers which obey the rules, but will make >> offenders at least functional. My main concern is that not every user is able >> to detect and report a problem, which prevents drivers from functioning. >> Especially this is a problem for not wide spread devices. >> Due to this users a seeing unusable equipment, but developers are not >> aware of those, even if fixes are trivial. >> >> Such mode has a also a negative effect: if a developer has a device >> with an offending driver, he can miss the warning message, since the driver >> just works. > > Exactly, let's fix the bugs. These have been bugs for 10+ years now, > they should get fixed, it's not complex :) We should probably have a similar patch on drivers/usb/gadget/udc/core.c::usb_gadget_map_request_by_dev() -- balbi
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
On Tue, Apr 25, 2017 at 01:35:33PM +0300, Maksim Salau wrote: > > + } else if (object_is_on_stack(urb->transfer_buffer)) { > > + WARN_ONCE(1, "transfer buffer is on stack\n"); > > + ret = -EAGAIN; > > } else { > > Hi, > > Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack, > kmemdup it and continue with a warning. This will give us both: functional > drivers (with possibly decreased efficiency in speed and memory footprint) > and warnings for developers that a particular driver requires attention. No, I do not want that, let's fix the drivers. > This mode will not affect drivers which obey the rules, but will make > offenders at least functional. My main concern is that not every user is able > to detect and report a problem, which prevents drivers from functioning. > Especially this is a problem for not wide spread devices. > Due to this users a seeing unusable equipment, but developers are not > aware of those, even if fixes are trivial. > > Such mode has a also a negative effect: if a developer has a device > with an offending driver, he can miss the warning message, since the driver > just works. Exactly, let's fix the bugs. These have been bugs for 10+ years now, they should get fixed, it's not complex :) thanks, greg k-h
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
On Tue, Apr 25, 2017 at 01:35:33PM +0300, Maksim Salau wrote: > > + } else if (object_is_on_stack(urb->transfer_buffer)) { > > + WARN_ONCE(1, "transfer buffer is on stack\n"); > > + ret = -EAGAIN; > > } else { > > Hi, > > Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack, > kmemdup it and continue with a warning. This will give us both: functional > drivers (with possibly decreased efficiency in speed and memory footprint) > and warnings for developers that a particular driver requires attention. No, I do not want that, let's fix the drivers. > This mode will not affect drivers which obey the rules, but will make > offenders at least functional. My main concern is that not every user is able > to detect and report a problem, which prevents drivers from functioning. > Especially this is a problem for not wide spread devices. > Due to this users a seeing unusable equipment, but developers are not > aware of those, even if fixes are trivial. > > Such mode has a also a negative effect: if a developer has a device > with an offending driver, he can miss the warning message, since the driver > just works. Exactly, let's fix the bugs. These have been bugs for 10+ years now, they should get fixed, it's not complex :) thanks, greg k-h
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
> + } else if (object_is_on_stack(urb->transfer_buffer)) { > + WARN_ONCE(1, "transfer buffer is on stack\n"); > + ret = -EAGAIN; > } else { Hi, Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack, kmemdup it and continue with a warning. This will give us both: functional drivers (with possibly decreased efficiency in speed and memory footprint) and warnings for developers that a particular driver requires attention. This mode will not affect drivers which obey the rules, but will make offenders at least functional. My main concern is that not every user is able to detect and report a problem, which prevents drivers from functioning. Especially this is a problem for not wide spread devices. Due to this users a seeing unusable equipment, but developers are not aware of those, even if fixes are trivial. Such mode has a also a negative effect: if a developer has a device with an offending driver, he can miss the warning message, since the driver just works. Regards, Maksim.
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
> + } else if (object_is_on_stack(urb->transfer_buffer)) { > + WARN_ONCE(1, "transfer buffer is on stack\n"); > + ret = -EAGAIN; > } else { Hi, Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack, kmemdup it and continue with a warning. This will give us both: functional drivers (with possibly decreased efficiency in speed and memory footprint) and warnings for developers that a particular driver requires attention. This mode will not affect drivers which obey the rules, but will make offenders at least functional. My main concern is that not every user is able to detect and report a problem, which prevents drivers from functioning. Especially this is a problem for not wide spread devices. Due to this users a seeing unusable equipment, but developers are not aware of those, even if fixes are trivial. Such mode has a also a negative effect: if a developer has a device with an offending driver, he can miss the warning message, since the driver just works. Regards, Maksim.
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
On Sun, 23 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_hcd_map_urb_for_dma() checking that > urb->transfer_buffer is not a stack object. > > Signed-off-by: Florian Fainelli> --- > Changes in v2: > > - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma() You probably should add a similar check to the pathway that maps urb->setup_packet, for the sake of consistency. Alan Stern > drivers/usb/core/hcd.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 49550790a3cb..ce9063ce906a 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1587,6 +1588,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct > urb *urb, > } else if (is_vmalloc_addr(urb->transfer_buffer)) { > WARN_ONCE(1, "transfer buffer not dma > capable\n"); > ret = -EAGAIN; > + } else if (object_is_on_stack(urb->transfer_buffer)) { > + WARN_ONCE(1, "transfer buffer is on stack\n"); > + ret = -EAGAIN; > } else { > urb->transfer_dma = dma_map_single( > hcd->self.sysdev, >
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
On Sun, 23 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_hcd_map_urb_for_dma() checking that > urb->transfer_buffer is not a stack object. > > Signed-off-by: Florian Fainelli > --- > Changes in v2: > > - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma() You probably should add a similar check to the pathway that maps urb->setup_packet, for the sake of consistency. Alan Stern > drivers/usb/core/hcd.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 49550790a3cb..ce9063ce906a 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1587,6 +1588,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct > urb *urb, > } else if (is_vmalloc_addr(urb->transfer_buffer)) { > WARN_ONCE(1, "transfer buffer not dma > capable\n"); > ret = -EAGAIN; > + } else if (object_is_on_stack(urb->transfer_buffer)) { > + WARN_ONCE(1, "transfer buffer is on stack\n"); > + ret = -EAGAIN; > } else { > urb->transfer_dma = dma_map_single( > hcd->self.sysdev, >
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
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_hcd_map_urb_for_dma() checking that > urb->transfer_buffer is not a stack object. This description is incomplete. > + } else if (object_is_on_stack(urb->transfer_buffer)) { > + WARN_ONCE(1, "transfer buffer is on stack\n"); > + ret = -EAGAIN; > } else { > urb->transfer_dma = dma_map_single( Not only is there a warning, but the check also forces all those URBs to abort with an error. Well, that makes it even easier to spot the offenders ... Regards, Clemens
Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
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_hcd_map_urb_for_dma() checking that > urb->transfer_buffer is not a stack object. This description is incomplete. > + } else if (object_is_on_stack(urb->transfer_buffer)) { > + WARN_ONCE(1, "transfer buffer is on stack\n"); > + ret = -EAGAIN; > } else { > urb->transfer_dma = dma_map_single( Not only is there a warning, but the check also forces all those URBs to abort with an error. Well, that makes it even easier to spot the offenders ... Regards, Clemens
[PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
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_hcd_map_urb_for_dma() checking that urb->transfer_buffer is not a stack object. Signed-off-by: Florian Fainelli--- Changes in v2: - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma() drivers/usb/core/hcd.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 49550790a3cb..ce9063ce906a 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -1587,6 +1588,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, } else if (is_vmalloc_addr(urb->transfer_buffer)) { WARN_ONCE(1, "transfer buffer not dma capable\n"); ret = -EAGAIN; + } else if (object_is_on_stack(urb->transfer_buffer)) { + WARN_ONCE(1, "transfer buffer is on stack\n"); + ret = -EAGAIN; } else { urb->transfer_dma = dma_map_single( hcd->self.sysdev, -- 2.11.0
[PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack
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_hcd_map_urb_for_dma() checking that urb->transfer_buffer is not a stack object. Signed-off-by: Florian Fainelli --- Changes in v2: - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma() drivers/usb/core/hcd.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 49550790a3cb..ce9063ce906a 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -1587,6 +1588,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, } else if (is_vmalloc_addr(urb->transfer_buffer)) { WARN_ONCE(1, "transfer buffer not dma capable\n"); ret = -EAGAIN; + } else if (object_is_on_stack(urb->transfer_buffer)) { + WARN_ONCE(1, "transfer buffer is on stack\n"); + ret = -EAGAIN; } else { urb->transfer_dma = dma_map_single( hcd->self.sysdev, -- 2.11.0