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

[Alan Stern]

On Thu, Nov 05, 2026 at 02:56:27PM +0800, YuanZhe Liu wrote:
> Hi Alan,
> 
> v3.  v2 had two problems, both pointed out after it went out:
> 
>  - a hub in a composite device matches the hub driver by interface
>    class, not by device class, so testing bDeviceClass alone missed it.
>    The check now looks up the interface the hub driver uses, whichever
>    way the device matched.

The USB spec forbids hubs to be composite devices.  You can go ahead and 
match by interface class rather than device class if you want, but I 
don't think it will make any difference.

Similarly, the spec forbids hubs from having more than one configuration 
and one interface.  Sashiko warned about devices violating this 
restriction, but I don't think we need to worry about them.

>  - USBDEVFS_CLAIM_PORT hands userspace a single port while the hub
>    driver stays bound; that is what the ioctl is for, and v2 made it
>    unusable by requiring the whole interface.  Requests for a port owned
>    by the caller are allowed now.  That needs a small helper in hub.c to
>    test ownership, which is why this touches three files.

We don't need to allow requests for claimed ports, do we?  

USBDEVFS_CLAIM_PORT doesn't really give userspace ownership of the port 
itself; rather, it gives userspace ownership of any device connected to 
the port.  For example, userspace shouldn't be allowed to suspend the 
connected device by sending a request to the port.  It has to use the 
runtime PM mechanism to suspend the device in the usual way.

> On whether this closes the window completely: it does not serialize the
> transfer, and it is not meant to.  check_ctrlrecip() runs under the
> device lock and do_proc_control() drops that lock before
> usbfs_start_wait_urb(), so an in-flight request is not ordered against a
> claim being released.  What the check gives us is that whenever a
> request is allowed, the hub driver is bound, so the port has no device
> for xhci_free_dev() to free.  The memory safety itself comes from the
> xhci hunk in the other thread.

Right now usb_hub_release_port() and usb_hub_release_all_ports() do not 
synchronize with outstanding URBs sent to a child on the port.  They 
really should.  (This means killing current URBs and preventing new URBs 
from being submitted.)  Fixing that can be a separate patch.

> With the port owner exception above, tools like uhubctl that send
> SET_FEATURE(PORT_POWER) to a hub without claiming either the port or the
> interface will still get -EBUSY.  Do you want an exception while
> privileges have not been dropped, or is losing that use case the point?

Well, it _is_ kind of the point.

The only program I know about for manipulating port power settings is 
one I wrote many years ago; it is careful to unbind the hub driver 
before changing any settings.

If we want to allow this sort of thing (changing a port's power while it 
is under control of the hub driver), the right way to do it is by adding 
a new command to the hub_ioctl() routine.

If people complain about regressions from this decision, we may be 
forced to make it a special case, which would require it to be 
integrated with the hub driver somehow.

> Whether this should be marked for stable is your call.  The missing
> check goes back to the initial git import (1da177e4c3f4) and the ioctl
> that can race with it arrived with 0837e7e5270b, so I did not add a
> Fixes: tag; I also left out Cc: stable because this changes behaviour
> for userspace.  The stable candidate is the xhci_hcd fix in the other
> thread.  If you would rather see this backported, tell me which commit
> you want named and I will resend with the tags.

Given the potential for bad interaction with user programs, I recommend 
not marking this for -stable.

Alan Stern