Re: [PATCH] USB: core: WARN if pipe direction != setup packet direction

[Johan Hovold <johan@kernel.org>]

On Thu, May 20, 2021 at 04:20:56PM -0400, Alan Stern wrote:
> When a control URB is submitted, the direction indicated by URB's pipe
> member is supposed to match the direction indicated by the setup
> packet's bRequestType member.  A mismatch could lead to trouble,
> depending on which field the host controller drivers use for
> determining the actual direction.
> 
> This shouldn't ever happen; it would represent a careless bug in a
> kernel driver somewhere.  This patch adds a dev_WARN to let people
> know about the potential problem.
> 
> Suggested-by: "Geoffrey D. Bennett" <g@b4.vu>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> 
> [as1960]
> 
> 
>  drivers/usb/core/urb.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: usb-devel/drivers/usb/core/urb.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/urb.c
> +++ usb-devel/drivers/usb/core/urb.c
> @@ -407,6 +407,9 @@ int usb_submit_urb(struct urb *urb, gfp_
>  			return -ENOEXEC;
>  		is_out = !(setup->bRequestType & USB_DIR_IN) ||
>  				!setup->wLength;
> +		if (usb_pipeout(urb->pipe) != is_out)
> +			dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n",
> +					urb->pipe, setup->bRequestType);
>  	} else {
>  		is_out = usb_endpoint_dir_out(&ep->desc);
>  	}

While I agree with intention here, I'm worried that this will start
flooding the logs of users.

So first, this should probably be rate limited.

Second, did you try to estimate how many call sites that get this wrong?
I always felt a bit pedantic when pointing out that the pipe direction
should match the request type to driver author's during review when (in
almost all cases?) this hasn't really mattered. I fear we may have
accumulated a fairly large number of these mismatches over the years but
I haven't verified that.

Johan