Received-SPF: pass (google.com: domain of linux-kernel+bounces-98590-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <2e2ec36a-3d48-48f0-8019-f1c1b320a477@redhat.com>
Date: Mon, 11 Mar 2024 10:14:33 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Reply-To: eric.auger@redhat.com
Subject: Re: [PATCH v2 2/7] vfio/pci: Lock external INTx masking ops
Content-Language: en-US
To: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org, clg@redhat.com, reinette.chatre@intel.com,
 linux-kernel@vger.kernel.org, kevin.tian@intel.com, stable@vger.kernel.org
References: <20240308230557.805580-1-alex.williamson@redhat.com>
 <20240308230557.805580-3-alex.williamson@redhat.com>
From: Eric Auger <eric.auger@redhat.com>
In-Reply-To: <20240308230557.805580-3-alex.williamson@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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@vger.kernel.org
> Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver")
> Reported-by: Reinette Chatre <reinette.chatre@intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

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 */
>  	}