On Mon, 11 Mar 2024 08:36:07 +0100 Eric Auger eric.auger@redhat.com wrote:

Hi Alex,

On 3/9/24 00:05, Alex Williamson wrote:

...

Currently for devices requiring masking at the irqchip for INTx, ie. devices without DisINTx support, the IRQ is enabled in request_irq() and subsequently disabled as necessary to align with the masked status flag. This presents a window where the interrupt could fire between these events, resulting in the IRQ incrementing the disable depth twice. This would be unrecoverable for a user since the masked flag prevents nested enables through vfio.

Instead, invert the logic using IRQF_NO_AUTOEN such that exclusive INTx is never auto-enabled, then unmask as required. Cc: stable@vger.kernel.org Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver") Reviewed-by: Kevin Tian kevin.tian@intel.com Signed-off-by: Alex Williamson alex.williamson@redhat.com

---

drivers/vfio/pci/vfio_pci_intrs.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 237beac83809..136101179fcb 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -296,8 +296,15 @@ static int vfio_intx_set_signal(struct vfio_pci_core_device *vdev, int fd) ctx->trigger = trigger;

• /*

• * Devices without DisINTx support require an exclusive interrupt,
  

• * IRQ masking is performed at the IRQ chip.  The masked status is
  

• * protected by vdev->irqlock. Setup the IRQ without auto-enable and
  

• * unmask as necessary below under lock.  DisINTx is unmodified by
  

• * the IRQ configuration and may therefore use auto-enable.  
  

If I remember correctly the main reason why the

vdev->pci_2_3 path is left unchanged is due to the fact the irq may not be exclusive and setting IRQF_NO_AUTOEN could be wrong in that case. May be worth to precise in the commit msg or here? Besides Reviewed-by: Eric Auger eric.auger@redhat.com Eric

IRQF_SHARED and IRQF_NO_AUTOEN are in fact mutually exclusive. Even if we could disable auto-enable, the driver sharing the interrupt could independently enable it. But really the basis for using IRQF_SHARED is that we have device level INTx detection and masking. The comment here is only to note that request_irq() doesn't gratuitously clear DisINTx, so the mask state previously applied through config space of the device is persistent. Thanks,

Alex

...

• */
  

  if (!vdev->pci_2_3)

• irqflags = 0;
  

• irqflags = IRQF_NO_AUTOEN;
  

ret = request_irq(pdev->irq, vfio_intx_handler, irqflags, ctx->name, vdev); @@ -308,13 +315,9 @@ static int vfio_intx_set_signal(struct vfio_pci_core_device *vdev, int fd) return ret; }

• /*

• * INTx disable will stick across the new irq setup,
  

• * disable_irq won't.
  

• */
  

  spin_lock_irqsave(&vdev->irqlock, flags);
• if (!vdev->pci_2_3 && ctx->masked)

• disable_irq_nosync(pdev->irq);
  

• if (!vdev->pci_2_3 && !ctx->masked)

• enable_irq(pdev->irq);
  

  spin_unlock_irqrestore(&vdev->irqlock, flags);

return 0;