Hi Peter, Thank you very much for the review and the detailed comments! Please see my comments embedded below and I will update the patch accordingly.
Best regards, Yang Liang -----Original Message----- From: Peter Maydell <[email protected]> Sent: Monday, November 24, 2025 8:50 PM To: Yang, Liang1 <[email protected]> Cc: [email protected]; [email protected]; [email protected]; Kim, Dongwon <[email protected]> Subject: Re: [PATCH] hw/usb/host-libusb: Do not assert when detects invalid altsetting 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.) YL: Thanks, well noted. > 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. YL: Agree, will update accordingly. > + 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. YL: Agree, will remove the check for conf==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.) YL: I see, let me check and see if can create a patch to fix those leaks. 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? YL: No, I have not seen (or noticed) this in real testing, but I think it's good to have especially during very fast USB hot-unplug/hot-replug. (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"). YL: This is for preventing the potential assert, but we can separate it into another patch or just remove it if it is really not required. (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. YL: Understand, I think it depends. For normal QEMU USB event tracing or debugging, it's better to use the tracepoint functions. But in this case, it is reporting an unexpected condition or behavior. > + } > + > 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. YL: Agree, will remove it. > + * 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.) YL: In this case, only one interface has an inconsistent altsetting index (caused by hot unplug/replug) while the others remain valid, so I use "continue" for skipping only the invalid interface avoids losing valid endpoint information. Returning early would drop all remaining interfaces, which seems undesirable. Thanks. > + } > intf = &conf->interface[i].altsetting[alt]; > } thanks -- PMM
