| Message ID | 20240308230557.805580-2-alex.williamson@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Interrupt eventfd hardening | expand |
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 > + */ > 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;
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; >
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 (!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;
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(-)