On Fri, 20 Jun 2025 15:50:27 +0300 Pekka Riikonen <[email protected]> wrote:
> Check if kernel returns 0 interrupt vectors and try another interrupt > type in that case. Failing to check the vector count can select an > interrupt type that's unusable. > > Signed-off-by: Pekka Riikonen <[email protected]> Patch looks good, automated review had some feedback. 1. Should add a Fixes: and Cc: [email protected] 2. There is a pre-existing bugs in the file as well Error 1: Missing handling when interrupt type was explicitly requested and count is 0 Confidence: ~85% The existing VFIO_IRQ_INFO_EVENTFD check (lines 223–228) has two branches: if the user explicitly requested this interrupt type (intr_mode != RTE_INTR_MODE_NONE), it returns an error; otherwise it continues to the next type. The new irq.count == 0 check does not mirror this pattern — it unconditionally does continue. This means if a user explicitly requested e.g. MSI-X via command line (--vfio-intr=msix) but the device reports 0 vectors for MSI-X, the code will silently fall through and try MSI or INTx instead of reporting an error. This is inconsistent with the existing behavior for the eventfd flag check, and arguably a bug: the user asked for a specific interrupt type that doesn't work, and should be told. Suggested fix: c /* If no vectors, try another type (or fail if explicitly requested) */ if (irq.count == 0) { if (intr_mode != RTE_INTR_MODE_NONE) { PCI_LOG(ERR, "Interrupt vector has no entries!"); return -1; } continue; } Error 2: Resource leak — eventfd fd leaked on later error paths (pre-existing) Confidence: ~90% This is a pre-existing issue, not introduced by this patch, but worth noting since the patch touches this function. At line 240, eventfd() creates a file descriptor. If rte_intr_fd_set() at line 247 fails, the function returns -1 without closing fd. This is a file descriptor leak on that error path. Similarly, the rte_intr_event_list_update failure path at line 236–237 is fine (no fd allocated yet), but the rte_intr_fd_set failure is a real leak. This is pre-existing and not caused by this patch, so it's informational rather than something blocking this patch.
