Re: [RFC PATCH v3 14/20] vfio/pci: Add INTx recovery operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] vfio/pci: permanent INTx masking in __vfio_pci_intx_mask() on
  runtime suspend
- [High] vfio/pci: unmasked shared INTx in
  vfio_pci_intx_mask_for_recovery()
- [Medium] vfio/pci: premature INTx unmask after blocked irq in
  vfio_intx_handler()
--

commit 832141c440dca237115d9dc7d2fca6d1c8bdf80a
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Add INTx recovery operations

This commit adds blocked INTx operations and recovery helpers to mask INTx,
record pending unmask requests while access is blocked, and restore INTx
state when recovery finishes.

> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 64f80f64ff57a..f78c7217017d2 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -29,6 +29,8 @@ struct vfio_pci_irq_ctx {
>  	struct virqfd			*mask;
>  	char				*name;
>  	bool				masked;
> +	bool				recovery_masked;
> +	bool				unmask_pending;
>  	struct irq_bypass_producer	producer;
>  };
>  
> @@ -126,6 +128,10 @@ static bool __vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
>  	if (WARN_ON_ONCE(!ctx))
>  		goto out_unlock;
>  
> +	/* An explicit mask cancels any unmask request saved during recovery. */
> +	ctx->recovery_masked = false;
> +	ctx->unmask_pending = false;
> +
>  	if (!ctx->masked) {
>  		/*
>  		 * Can't use check_and_mask here because we always want to

[Severity: High]
Can __vfio_pci_intx_mask() leave INTx permanently masked if
vfio_pci_core_runtime_suspend() runs during recovery?

Unlike userspace mask paths, vfio_pci_core_runtime_suspend() is not gated
by vfio_pci_core_access() and calls:

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_core_runtime_suspend() {
    ...
	vdev->pm_intx_masked = ((vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX) &&
				vfio_pci_intx_mask(vdev));
    ...
}

When INTx is masked for recovery (ctx->masked is true and
ctx->recovery_masked or ctx->unmask_pending is true), a runtime suspend
(for example when pm_runtime_put() runs after an ioctl on a device in
VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY) clears both ctx->recovery_masked and
ctx->unmask_pending here, skips the !ctx->masked block, and returns false.
vfio_pci_core_runtime_suspend() then records vdev->pm_intx_masked = false,
so neither vfio_pci_core_runtime_resume() nor
vfio_pci_intx_recovery_finish() will unmask INTx again.

> @@ -220,33 +245,151 @@ void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev)
>  	mutex_unlock(&vdev->igate);
>  }
>  
> -static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
> +/*
> + * Recovery always masks INTx and remembers whether to unmask it afterward.
> + * The interrupt handler masks INTx only if this device has a pending
> + * interrupt, since another device may share the line.
> + */
> +static bool vfio_pci_intx_mask_for_recovery(struct vfio_pci_core_device *vdev,
> +					    struct vfio_pci_irq_ctx *ctx,
> +					    bool quiesce)
>  {
> -	struct vfio_pci_irq_ctx *ctx = dev_id;
> -	struct vfio_pci_core_device *vdev = ctx->vdev;
> -	unsigned long flags;
> -	int ret = IRQ_NONE;
> +	lockdep_assert_held(&vdev->irqlock);
>  
> -	spin_lock_irqsave(&vdev->irqlock, flags);
> +	if (ctx->masked)
> +		return false;
>  
> +	if (!vdev->pci_2_3)
> +		disable_irq_nosync(vdev->pdev->irq);
> +	else if (quiesce)
> +		pci_intx(vdev->pdev, 0);
> +	else if (!pci_check_and_mask_intx(vdev->pdev))
> +		return false;
>  
> +	ctx->masked = true;
> +	if (quiesce)
> +		ctx->recovery_masked = true;
> +	return true;
> +}

[Severity: High]
Does the early return when ctx->masked is true prevent
vfio_pci_intx_handle_blocked() from masking shared PCI 2.3 interrupts
during recovery?

vfio_pci_intx_recovery_start() calls vfio_pci_intx_mask_for_recovery()
with quiesce = true, setting ctx->masked = true.  However, on a PCI 2.3
device, PCI_COMMAND_INTX_DISABLE can still be clear in hardware if config
space read 0xffff when error_detected ran, or after a host slot/bus reset
clears PCI_COMMAND before state restoration.  If the device then asserts a
shared INTx line while access is blocked, vfio_pci_intx_handle_blocked()
calls vfio_pci_intx_mask_for_recovery(vdev, ctx, false), which returns
false immediately because ctx->masked is true instead of calling
pci_check_and_mask_intx(), returning IRQ_NONE and leaving the shared level
interrupt asserted.

> +
> +irqreturn_t vfio_pci_intx_handle(struct vfio_pci_core_device *vdev,
> +				 struct vfio_pci_irq_ctx *ctx)
> +{
> +	lockdep_assert_held(&vdev->irqlock);
>  	if (!vdev->pci_2_3) {
>  		disable_irq_nosync(vdev->pdev->irq);
>  		ctx->masked = true;
> -		ret = IRQ_HANDLED;
> -	} else if (!ctx->masked &&  /* may be shared */
> -		   pci_check_and_mask_intx(vdev->pdev)) {
> +		return IRQ_HANDLED;
> +	}
> +	if (!ctx->masked && pci_check_and_mask_intx(vdev->pdev)) {
>  		ctx->masked = true;
> -		ret = IRQ_HANDLED;
> +		return IRQ_HANDLED;
>  	}
> +	return IRQ_NONE;
> +}
> +
> +irqreturn_t vfio_pci_intx_handle_blocked(struct vfio_pci_core_device *vdev,
> +					 struct vfio_pci_irq_ctx *ctx)
> +{
> +	lockdep_assert_held(&vdev->irqlock);
> +	/* A shared interrupt line still needs pending INTx to be masked. */
> +	if (vfio_pci_intx_mask_for_recovery(vdev, ctx, false) ||
> +	    (ctx->masked && !vdev->pci_2_3))
> +		return IRQ_HANDLED;
> +	return IRQ_NONE;
> +}
> +
> +static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
> +{
> +	struct vfio_pci_irq_ctx *ctx = dev_id;
> +	struct vfio_pci_core_device *vdev = ctx->vdev;
> +	unsigned long flags;
> +	irqreturn_t ret;
>  
> +	spin_lock_irqsave(&vdev->irqlock, flags);
> +	ret = vfio_pci_access_ops(vdev)->intx_handler(vdev, ctx);
>  	spin_unlock_irqrestore(&vdev->irqlock, flags);
>  
>  	if (ret == IRQ_HANDLED)
>  		vfio_send_intx_eventfd(vdev, ctx);
> -
>  	return ret;
>  }

[Severity: Medium]
When vfio_pci_intx_handle_blocked() returns IRQ_HANDLED,
vfio_intx_handler() delivers the automasked interrupt to userspace via
vfio_send_intx_eventfd(), but neither vfio_pci_intx_mask_for_recovery()
nor vfio_pci_intx_handle_blocked() clears ctx->recovery_masked or
ctx->unmask_pending.  Can this cause vfio_pci_intx_recovery_finish() to
unmask INTx prematurely before userspace has processed and unmasked the
interrupt?

CPU0                                      CPU1 (<hardirq>)
vfio_pci_intx_recovery_start()
  spin_lock_irqsave(&vdev->irqlock)
  vfio_pci_intx_mask_for_recovery(..., true)
    disable_irq_nosync(pdev->irq)
    ctx->masked = true
    ctx->recovery_masked = true
  spin_unlock_irqrestore(&vdev->irqlock)
                                          vfio_intx_handler()
                                            spin_lock_irqsave(&vdev->irqlock)
                                            vfio_pci_intx_handle_blocked()
                                              ctx->masked && !vdev->pci_2_3
                                              --> returns IRQ_HANDLED
                                            spin_unlock_irqrestore(...)
                                            vfio_send_intx_eventfd()
...
vfio_pci_intx_recovery_finish()
  if (ctx->recovery_masked || ...)
    vfio_pci_intx_unmask_live()           <-- unmasks before userspace

On !vdev->pci_2_3 devices, if ctx->masked was already true before recovery
started, the (ctx->masked && !vdev->pci_2_3) check also returns
IRQ_HANDLED and triggers a spurious eventfd signal while masked.  The same
premature unmask happens if vfio_pci_intx_unmask_blocked() sets
ctx->unmask_pending = true while ctx->masked is still false (before
vfio_pci_intx_recovery_start() runs) and an interrupt is then masked and
signaled by vfio_pci_intx_handle_blocked() without clearing
ctx->unmask_pending.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112141632.918954-1-skolothumtho@nvidia.com?part=14