On Thu, 20 Nov 2025 at 05:27, Yang, Liang1 <[email protected]> wrote:
>
> Dear QEMU maintainers,
>
>
>
> I would like to submit a patch for preventing the guest VM crash caused by
> the assertion failure in usb_host_ep_update() during USB hotplug/unplug on
> host passthrough.
>
>
>
> QEMU issue submitted:
>
> https://gitlab.com/qemu-project/qemu/-/issues/3189
>
>
>
> Please help to review below patch, thanks!
Hi; thanks for this patch. I don't really know our USB
subsystem, so this is more some surface-level comments
rather than an in-depth review.
(PS: for future patch submissions, it would be helpful
if you could send them as plain text, not HTML emails.)
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 691bc881fb..3a08caafa5 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
>
> @@ -885,6 +885,15 @@ static void usb_host_ep_update(USBHostDevice *s)
> trace_usb_host_parse_config(s->bus_num, s->addr,
> conf->bConfigurationValue, true);
>
> + /* Log and skip if configuration is NULL or has no interfaces */
This comment says what the code does, which is not very useful.
More helpful is to say *why* the code does something.
> + if (!conf || conf->bNumInterfaces == 0) {
Why are we testing conf for NULL here? We already dereference
it in the line above, so this is too late to check if we need to.
We get conf by calling libusb_get_active_config_descriptor(),
and I don't think that will set conf to NULL if it succeeds,
so it looks to me like we don't need to test for NULL.
> + warn_report("usb-host: ignoring invalid configuration "
> + "for device %s (bus=%03d, addr=%03d)",
> + udev->product_desc ? udev->product_desc : "unknown",
> + s->bus_num, s->addr);
> + return;
This is something we already get wrong in a lot of the
error-exit paths of this function, but we need to free
the conf descriptor with libusb_free_config_descriptor().
(If you wanted to include a patch to fix those existing
leaks that would be great.)
We already ignored a bNumInterfaces==0 config
(because the rest of the function would just do nothing), so
the only change here is to warn about it.
(1) Have you seen the bNumInterfaces==0 situation in real life?
(2) This is unrelated to the other half of the patch, so it
should be its own patch (i.e. one which just says "warn about
this kind of invalid device rather than silently ignoring it").
(3) All the other cases in this function of "the info about
the endpoint looked weird" we report via the tracepoint
trace_usb_host_parse_error(). I don't have a strong opinion
about warn_report vs a tracepoint here, but I do think we
should be consistent.
> + }
> +
> for (i = 0; i < conf->bNumInterfaces; i++) {
> /*
> * The udev->altsetting array indexes alternate settings
> @@ -896,7 +905,21 @@ static void usb_host_ep_update(USBHostDevice *s)
> alt = udev->altsetting[intf->bInterfaceNumber];
>
> if (alt != 0) {
> - assert(alt < conf->interface[i].num_altsetting);
> + if (alt >= conf->interface[i].num_altsetting) {
> + /*
> + * Recommend fix: sometimes libusb reports a temporary
"Recommend fix" doesn't make sense here -- you can delete that bit.
> + * invalid altsetting index during fast hotplug/unplug.
> + * Instead of aborting, log a warning and skip the interface.
> + */
> + warn_report("usb-host: ignoring invalid altsetting=%d
> (max=%d) "
> + "for interface=%d on %s (bus=%03d, addr=%03d)",
> + alt,
> + conf->interface[i].num_altsetting ?
> conf->interface[i].num_altsetting - 1 : -1,
> + i,
> + udev->product_desc ? udev->product_desc : "unknown",
> + s->bus_num, s->addr);
> + continue;
For other errors with the intf info we "return", i.e.
skip the whole endpoint, rather than just continuing.
Should we do the same thing here ?
(Conceptually the same thing -- just "return" leaks
the config descriptor, as noted above.)
> + }
> intf = &conf->interface[i].altsetting[alt];
> }
thanks
-- PMM
