Extending USB_CONNECTINFO ioctl

Extending USB_CONNECTINFO ioctl

[Dmitry Torokhov]

Hi Alan, Greg,

When running software in a jailed environment where sysfs or udev is not
readily available and one can only have an FD to usbdevfs device passed
into the jail, there is a desire to allow libusb working. Alan recently
added USBDEVFS_GET_SPEED, but we are still missing bus number and list
of port numbers on the way to the root to be able to better identify the
device in question.

What do you think about adding a new ioctl:

struct usbdevfs_connectinfo_ex {
	__u32 size;		/* size of the structure from the kernel POV */
	__u32 busnum;
	__u32 devnum;
	__u32 speed;		/* USB_SPEED_* form ch9.h */
	u8 num_ports;		/* Number of entries in port_numbers array */
	u8 port_numbers[31];	/* Current limit in USB3.0 spec is 7 */
};

/*
 * Returns struct usbdevfs_connectinfo_ex; length is variable to allow
 * extending size of the data returned.
 */
#define USBDEVFS_CONNINFO_EX(len)  _IOC(_IOC_READ, 'U', 32, len)

Thanks.

-- 
Dmitry

Re: Extending USB_CONNECTINFO ioctl

[Greg Kroah-Hartman]

On Mon, Jun 03, 2019 at 05:24:10PM -0700, Dmitry Torokhov wrote:
> Hi Alan, Greg,
> 
> When running software in a jailed environment where sysfs or udev is not
> readily available and one can only have an FD to usbdevfs device passed
> into the jail, there is a desire to allow libusb working. Alan recently
> added USBDEVFS_GET_SPEED, but we are still missing bus number and list
> of port numbers on the way to the root to be able to better identify the
> device in question.
> 
> What do you think about adding a new ioctl:
> 
> struct usbdevfs_connectinfo_ex {
> 	__u32 size;		/* size of the structure from the kernel POV */
> 	__u32 busnum;
> 	__u32 devnum;
> 	__u32 speed;		/* USB_SPEED_* form ch9.h */
> 	u8 num_ports;		/* Number of entries in port_numbers array */
> 	u8 port_numbers[31];	/* Current limit in USB3.0 spec is 7 */
> };
> 
> /*
>  * Returns struct usbdevfs_connectinfo_ex; length is variable to allow
>  * extending size of the data returned.
>  */
> #define USBDEVFS_CONNINFO_EX(len)  _IOC(_IOC_READ, 'U', 32, len)

Sounds reasonable, as long as you get the 'variable data' portion
correct :)

thanks,

greg k-h

Re: Extending USB_CONNECTINFO ioctl

[Alan Stern]

On Mon, 3 Jun 2019, Dmitry Torokhov wrote:

> Hi Alan, Greg,
> 
> When running software in a jailed environment where sysfs or udev is not
> readily available and one can only have an FD to usbdevfs device passed
> into the jail, there is a desire to allow libusb working. Alan recently
> added USBDEVFS_GET_SPEED, but we are still missing bus number and list
> of port numbers on the way to the root to be able to better identify the
> device in question.
> 
> What do you think about adding a new ioctl:
> 
> struct usbdevfs_connectinfo_ex {
> 	__u32 size;		/* size of the structure from the kernel POV */

This should be an input/output parameter.  That is, the original value
says how big the userspace structure is, and the value stored by the
kernel says how much space was actually filled in.  Or alternatively, 
have two size fields (one for input and one for output).

> 	__u32 busnum;
> 	__u32 devnum;
> 	__u32 speed;		/* USB_SPEED_* form ch9.h */
> 	u8 num_ports;		/* Number of entries in port_numbers array */
> 	u8 port_numbers[31];	/* Current limit in USB3.0 spec is 7 */
> };

Yeah, 31 is overkill.  Even the USB-2.0 spec limits the topology to 7
tiers (including the root hub), meaning that only 6 port numbers are
needed.

> /*
>  * Returns struct usbdevfs_connectinfo_ex; length is variable to allow
>  * extending size of the data returned.
>  */
> #define USBDEVFS_CONNINFO_EX(len)  _IOC(_IOC_READ, 'U', 32, len)

Sounds okay to me.  Have you asked the libusb Linux-port maintainers
what they think and if they have any suggestions for additional fields?

Also, if you implement this, remember to add a USBDEVFS_CAP_CONNINFO_EX
capability flag for the new ioctl (see the USBDEVFS_GET_CAPABILITIES 
handler).

Alan Stern

Re: Extending USB_CONNECTINFO ioctl

[Dmitry Torokhov]

Hi Alan,

On Tue, Jun 04, 2019 at 10:17:25AM -0400, Alan Stern wrote:
> On Mon, 3 Jun 2019, Dmitry Torokhov wrote:
> 
> > Hi Alan, Greg,
> > 
> > When running software in a jailed environment where sysfs or udev is not
> > readily available and one can only have an FD to usbdevfs device passed
> > into the jail, there is a desire to allow libusb working. Alan recently
> > added USBDEVFS_GET_SPEED, but we are still missing bus number and list
> > of port numbers on the way to the root to be able to better identify the
> > device in question.
> > 
> > What do you think about adding a new ioctl:
> > 
> > struct usbdevfs_connectinfo_ex {
> > 	__u32 size;		/* size of the structure from the kernel POV */
> 
> This should be an input/output parameter.  That is, the original value
> says how big the userspace structure is, and the value stored by the
> kernel says how much space was actually filled in.  Or alternatively, 
> have two size fields (one for input and one for output).

The "incoming" size is encoded in the ioctl number (the 'len' argument
in USBDEVFS_CONNINFO_EX()), there is no reason to add it separately (or
make read/write).

> 
> > 	__u32 busnum;
> > 	__u32 devnum;
> > 	__u32 speed;		/* USB_SPEED_* form ch9.h */
> > 	u8 num_ports;		/* Number of entries in port_numbers array */
> > 	u8 port_numbers[31];	/* Current limit in USB3.0 spec is 7 */
> > };
> 
> Yeah, 31 is overkill.  Even the USB-2.0 spec limits the topology to 7
> tiers (including the root hub), meaning that only 6 port numbers are
> needed.

If you insist I can bring it down to 7, but this structure is short
lived, on stack, and there should not be many of them in the kernel, so
if we can avoid extending a particular field in the future I think it is
worth it.
.

> 
> > /*
> >  * Returns struct usbdevfs_connectinfo_ex; length is variable to allow
> >  * extending size of the data returned.
> >  */
> > #define USBDEVFS_CONNINFO_EX(len)  _IOC(_IOC_READ, 'U', 32, len)
> 
> Sounds okay to me.  Have you asked the libusb Linux-port maintainers
> what they think and if they have any suggestions for additional fields?

Why don't we just get them into this conversation? Hopefully
libusb-devel that I just CCed here is not subscription-only.

> 
> Also, if you implement this, remember to add a USBDEVFS_CAP_CONNINFO_EX
> capability flag for the new ioctl (see the USBDEVFS_GET_CAPABILITIES 
> handler).

OK, will do.

Thanks.

-- 
Dmitry

Re: Extending USB_CONNECTINFO ioctl

[Greg Kroah-Hartman]

On Tue, Jun 04, 2019 at 09:14:51AM -0700, Dmitry Torokhov wrote:
> Hi Alan,
> 
> On Tue, Jun 04, 2019 at 10:17:25AM -0400, Alan Stern wrote:
> > On Mon, 3 Jun 2019, Dmitry Torokhov wrote:
> > 
> > > Hi Alan, Greg,
> > > 
> > > When running software in a jailed environment where sysfs or udev is not
> > > readily available and one can only have an FD to usbdevfs device passed
> > > into the jail, there is a desire to allow libusb working. Alan recently
> > > added USBDEVFS_GET_SPEED, but we are still missing bus number and list
> > > of port numbers on the way to the root to be able to better identify the
> > > device in question.
> > > 
> > > What do you think about adding a new ioctl:
> > > 
> > > struct usbdevfs_connectinfo_ex {
> > > 	__u32 size;		/* size of the structure from the kernel POV */
> > 
> > This should be an input/output parameter.  That is, the original value
> > says how big the userspace structure is, and the value stored by the
> > kernel says how much space was actually filled in.  Or alternatively, 
> > have two size fields (one for input and one for output).
> 
> The "incoming" size is encoded in the ioctl number (the 'len' argument
> in USBDEVFS_CONNINFO_EX()), there is no reason to add it separately (or
> make read/write).
> 
> > 
> > > 	__u32 busnum;
> > > 	__u32 devnum;
> > > 	__u32 speed;		/* USB_SPEED_* form ch9.h */
> > > 	u8 num_ports;		/* Number of entries in port_numbers array */
> > > 	u8 port_numbers[31];	/* Current limit in USB3.0 spec is 7 */
> > > };
> > 
> > Yeah, 31 is overkill.  Even the USB-2.0 spec limits the topology to 7
> > tiers (including the root hub), meaning that only 6 port numbers are
> > needed.
> 
> If you insist I can bring it down to 7, but this structure is short
> lived, on stack, and there should not be many of them in the kernel, so
> if we can avoid extending a particular field in the future I think it is
> worth it.

I don't know what USB 4 has in it (and if I did, I couldn't mention it),
but someone who can see those specs might want to take a look at them to
just say "yeah, 31 looks like a good number" or "nope, make it a bit
bigger please!"

thanks,

greg k-h

Re: Extending USB_CONNECTINFO ioctl

[Dmitry Torokhov]

On Tue, Jun 04, 2019 at 06:48:53PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2019 at 09:14:51AM -0700, Dmitry Torokhov wrote:
> > Hi Alan,
> > 
> > On Tue, Jun 04, 2019 at 10:17:25AM -0400, Alan Stern wrote:
> > > On Mon, 3 Jun 2019, Dmitry Torokhov wrote:
> > > 
> > > > Hi Alan, Greg,
> > > > 
> > > > When running software in a jailed environment where sysfs or udev is not
> > > > readily available and one can only have an FD to usbdevfs device passed
> > > > into the jail, there is a desire to allow libusb working. Alan recently
> > > > added USBDEVFS_GET_SPEED, but we are still missing bus number and list
> > > > of port numbers on the way to the root to be able to better identify the
> > > > device in question.
> > > > 
> > > > What do you think about adding a new ioctl:
> > > > 
> > > > struct usbdevfs_connectinfo_ex {
> > > > 	__u32 size;		/* size of the structure from the kernel POV */
> > > 
> > > This should be an input/output parameter.  That is, the original value
> > > says how big the userspace structure is, and the value stored by the
> > > kernel says how much space was actually filled in.  Or alternatively, 
> > > have two size fields (one for input and one for output).
> > 
> > The "incoming" size is encoded in the ioctl number (the 'len' argument
> > in USBDEVFS_CONNINFO_EX()), there is no reason to add it separately (or
> > make read/write).
> > 
> > > 
> > > > 	__u32 busnum;
> > > > 	__u32 devnum;
> > > > 	__u32 speed;		/* USB_SPEED_* form ch9.h */
> > > > 	u8 num_ports;		/* Number of entries in port_numbers array */
> > > > 	u8 port_numbers[31];	/* Current limit in USB3.0 spec is 7 */
> > > > };
> > > 
> > > Yeah, 31 is overkill.  Even the USB-2.0 spec limits the topology to 7
> > > tiers (including the root hub), meaning that only 6 port numbers are
> > > needed.
> > 
> > If you insist I can bring it down to 7, but this structure is short
> > lived, on stack, and there should not be many of them in the kernel, so
> > if we can avoid extending a particular field in the future I think it is
> > worth it.
> 
> I don't know what USB 4 has in it (and if I did, I couldn't mention it),
> but someone who can see those specs might want to take a look at them to
> just say "yeah, 31 looks like a good number" or "nope, make it a bit
> bigger please!"

OK, I believe 7 tiers should be enough, so I'll cut it down to 7 from
31.

Thanks.

-- 
Dmitry