On Mon, 24 Feb 2014, Dan Williams wrote:
> > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > > index 2518c3250750..ff7cd489f55d 100644
> > > --- a/drivers/usb/core/hcd.c
> > > +++ b/drivers/usb/core/hcd.c
> > > @@ -1502,6 +1502,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd,
> > > struct urb *urb,
> > > ret = -EAGAIN;
> > > else
> > > urb->transfer_flags |= URB_DMA_MAP_PAGE;
> > > + } else if (is_vmalloc_addr(urb->transfer_buffer)) {
> > > + WARN_ONCE(1, "transfer buffer not dma
> > > capable\n");
> > > + return -EAGAIN;
> > > } else {
> > > urb->transfer_dma = dma_map_single(
> > > hcd->self.controller,
> > >
> >
> > You mustn't just return -EAGAIN. Set ret = -EAGAIN and fall through to
> > the error-handling pathway.
>
> Alan, I'm simply trying to trade one undefined behavior for another ;-).
> Good catch.
>
> Although the result would be the same in this case, it's something
> subtle for someone to trip over later.
I don't know what you mean. Think about what would happen to the DMA
mapping for the SETUP buffer in a control transfer if the data buffer
was a vmalloc'ed region.
> Hmm, this check above also short-circuits the error fall-through:
>
> /* We don't support sg for isoc transfers ! */
> if (usb_endpoint_xfer_isoc(&urb->ep->desc)) {
> WARN_ON(1);
> return -EINVAL;
> }
True, but in this case it doesn't matter because an URB for an
isochronous endpoint will never get a DMA mapping for a SETUP buffer.
> 8<------------
> Subject: usb: catch attempts to submit urbs with a vmalloc'd transfer buffer
>
> From: Dan Williams <[email protected]>
>
> Save someone else the debug cycles of figuring out why a driver's
> transfer request is failing or causing undefined system behavior.
> Buffers submitted for dma must come from GFP allocated / DMA-able
> memory.
>
> Return -EAGAIN matching the return value for dma_mapping_error() cases.
>
> Cc: Alan Stern <[email protected]>
> Cc: Sarah Sharp <[email protected]>
> Cc: Mathias Nyman <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> drivers/usb/core/hcd.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 2518c3250750..a8a8cc94b6e3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1502,6 +1502,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct
> urb *urb,
> ret = -EAGAIN;
> else
> urb->transfer_flags |= URB_DMA_MAP_PAGE;
> + } else if (is_vmalloc_addr(urb->transfer_buffer)) {
> + WARN_ONCE(1, "transfer buffer not dma
> capable\n");
> + ret = -EAGAIN;
> } else {
> urb->transfer_dma = dma_map_single(
> hcd->self.controller,
Acked-by: Alan Stern <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
