On 3/9/24 00:05, Alex Williamson wrote:
> Mask operations through config space changes to DisINTx may race INTx
> configuration changes via ioctl. Create wrappers that add locking for
> paths outside of the core interrupt code.
>
> In particular, irq_type is updated holding igate, therefore testing
> is_intx() requires holding igate. For example clearing DisINTx from
> config space can otherwise race changes of the interrupt configuration.
>
> This aligns interfaces which may trigger the INTx eventfd into two
> camps, one side serialized by igate and the other only enabled while
> INTx is configured. A subsequent patch introduces synchronization for
> the latter flows.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver")
> Reported-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
Eric
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 34 +++++++++++++++++++++++++------
> 1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 136101179fcb..75c85eec21b3 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -99,13 +99,15 @@ static void vfio_send_intx_eventfd(void *opaque, void *unused)
> }
>
> /* Returns true if the INTx vfio_pci_irq_ctx.masked value is changed. */
> -bool vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
> +static bool __vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
> {
> struct pci_dev *pdev = vdev->pdev;
> struct vfio_pci_irq_ctx *ctx;
> unsigned long flags;
> bool masked_changed = false;
>
> + lockdep_assert_held(&vdev->igate);
> +
> spin_lock_irqsave(&vdev->irqlock, flags);
>
> /*
> @@ -143,6 +145,17 @@ bool vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
> return masked_changed;
> }
>
> +bool vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
> +{
> + bool mask_changed;
> +
> + mutex_lock(&vdev->igate);
> + mask_changed = __vfio_pci_intx_mask(vdev);
> + mutex_unlock(&vdev->igate);
> +
> + return mask_changed;
> +}
> +
> /*
> * If this is triggered by an eventfd, we can't call eventfd_signal
> * or else we'll deadlock on the eventfd wait queue. Return >0 when
> @@ -194,12 +207,21 @@ static int vfio_pci_intx_unmask_handler(void *opaque, void *unused)
> return ret;
> }
>
> -void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev)
> +static void __vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev)
> {
> + lockdep_assert_held(&vdev->igate);
> +
> if (vfio_pci_intx_unmask_handler(vdev, NULL) > 0)
> vfio_send_intx_eventfd(vdev, NULL);
> }
>
> +void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev)
> +{
> + mutex_lock(&vdev->igate);
> + __vfio_pci_intx_unmask(vdev);
> + mutex_unlock(&vdev->igate);
> +}
> +
> static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
> {
> struct vfio_pci_core_device *vdev = dev_id;
> @@ -563,11 +585,11 @@ static int vfio_pci_set_intx_unmask(struct vfio_pci_core_device *vdev,
> return -EINVAL;
>
> if (flags & VFIO_IRQ_SET_DATA_NONE) {
> - vfio_pci_intx_unmask(vdev);
> + __vfio_pci_intx_unmask(vdev);
> } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> uint8_t unmask = *(uint8_t *)data;
> if (unmask)
> - vfio_pci_intx_unmask(vdev);
> + __vfio_pci_intx_unmask(vdev);
> } else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> struct vfio_pci_irq_ctx *ctx = vfio_irq_ctx_get(vdev, 0);
> int32_t fd = *(int32_t *)data;
> @@ -594,11 +616,11 @@ static int vfio_pci_set_intx_mask(struct vfio_pci_core_device *vdev,
> return -EINVAL;
>
> if (flags & VFIO_IRQ_SET_DATA_NONE) {
> - vfio_pci_intx_mask(vdev);
> + __vfio_pci_intx_mask(vdev);
> } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> uint8_t mask = *(uint8_t *)data;
> if (mask)
> - vfio_pci_intx_mask(vdev);
> + __vfio_pci_intx_mask(vdev);
> } else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> return -ENOTTY; /* XXX implement me */
> }
