Re: [PATCH v4] usb: usbfs: Require claiming a hub interface for hub class requests

[Alan Stern]

On Fri, Nov 06, 2026 at 01:23:36AM +0800, YuanZhe Liu wrote:
> Hi Alan,
> 
> v4 drops the port owner exception.  You are right that
> USBDEVFS_CLAIM_PORT hands over the device behind the port, not the port
> itself: the device is driven and suspended through its own fd, so
> letting a request poke port state on behalf of a claimed port does not
> buy anything.  With the exception gone the helper in hub.c and the
> declaration in usb.h go away too, and this is a single file patch again.
> 
> I kept the interface class test.  I agree it should not matter for a
> device that follows the spec, but it costs nothing and it keeps the
> check in line with the two entries in hub_id_table.
> 
> Then the -EBUSY for tools that switch port power without unbinding the
> hub driver is intended, as you said.
> 
> No Fixes: tag and no Cc: stable, following your recommendation.
> 
> You mentioned that usb_hub_release_port() and usb_hub_release_all_ports()
> should synchronize with outstanding URBs sent to the device on the port:
> kill the current ones and keep new ones from being submitted.  I would
> like to write that as a separate patch.  Should I send it now, on top of
> this one, or wait until this one is settled?

I'd wait.  Also, don't be so quick to send new versions of patches.  One 
or two per day is plenty; you want to leave time for non-AI people to go 
over the current version and comment on it before sending a new one.

> YuanZhe
> 
>  drivers/usb/core/devio.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 486ced74405e..d39ef9a80ec2 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -878,6 +878,23 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
>  	return -ENOENT;
>  }
>  
> +/*
> + * Return true if @dev is a hub.  The hub driver matches devices by device
> + * class or by interface class, and a hub has a single interface either way.
> + */
> +static bool is_hub(struct usb_device *dev)
> +{
> +	struct usb_host_config *config = dev->actconfig;
> +
> +	if (dev->descriptor.bDeviceClass == USB_CLASS_HUB)
> +		return true;
> +	if (!config || config->desc.bNumInterfaces < 1)
> +		return false;
> +	return config->interface[0] && config->interface[0]->cur_altsetting &&
> +	       config->interface[0]->cur_altsetting->desc.bInterfaceClass ==
> +			USB_CLASS_HUB;
> +}

More than is really needed, but okay.

> +
>  static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
>  			   unsigned int request, unsigned int index)
>  {
> @@ -891,6 +908,18 @@ static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
>  	if (USB_TYPE_VENDOR == (USB_TYPE_MASK & requesttype))
>  		return 0;
>  
> +	/*
> +	 * A hub driver owns the ports and the devices behind them.  Letting
> +	 * userspace change hub or port state behind its back, for example
> +	 * with a SET_FEATURE(PORT_SUSPEND) request while the hub interface is
> +	 * being released, is not safe: the request is passed down to the HCD,
> +	 * which may walk per-device state that is being torn down at the same
> +	 * time.  Port requests use USB_RECIP_OTHER and hub-wide ones use
> +	 * USB_RECIP_DEVICE; the hub driver owns both.

That last sentence is odd, given that the code you're adding doesn't 
check the recipient.  It should instead say that IN requests are safe 
since they don't affect the hub, but OUT requests aren't allowed unless 
the caller has claimed the hub interface.

> +	 */
> +	if ((requesttype & USB_DIR_IN) == 0 && is_hub(ps->dev))
> +		return checkintf(ps, 0);

I'd do the is_hub() test first, since that's what this special case is 
all about.

Aside from those minor points, this looks good.

Alan Stern