Received-SPF: pass (google.com: domain of linux-kernel+bounces-104631-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: <d141c24d-4d88-45ec-b8cf-5697c91cc6a5@redhat.com>
Date: Fri, 15 Mar 2024 17:36:44 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Reply-To: eric.auger@redhat.com
Subject: Re: [PATCH v2 6/7] vfio/platform: Create persistent IRQ handlers
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-7-alex.williamson@redhat.com>
From: Eric Auger <eric.auger@redhat.com>
In-Reply-To: <20240308230557.805580-7-alex.williamson@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi Alex,

On 3/9/24 00:05, Alex Williamson wrote:
> The vfio-platform SET_IRQS ioctl currently allows loopback triggering of
> an interrupt before a signaling eventfd has been configured by the user,
> which thereby allows a NULL pointer dereference.
>
> Rather than register the IRQ relative to a valid trigger, register all
> IRQs in a disabled state in the device open path.  This allows mask
> operations on the IRQ to nest within the overall enable state governed
> by a valid eventfd signal.  This decouples @masked, protected by the
> @locked spinlock from @trigger, protected via the @igate mutex.
>
> In doing so, it's guaranteed that changes to @trigger cannot race the
> IRQ handlers because the IRQ handler is synchronously disabled before
> modifying the trigger, and loopback triggering of the IRQ via ioctl is
> safe due to serialization with trigger changes via igate.
>
> For compatibility, request_irq() failures are maintained to be local to
> the SET_IRQS ioctl rather than a fatal error in the open device path.
> This allows, for example, a userspace driver with polling mode support
> to continue to work regardless of moving the request_irq() call site.
> This necessarily blocks all SET_IRQS access to the failed index.
>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: 57f972e2b341 ("vfio/platform: trigger an interrupt via eventfd")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  drivers/vfio/platform/vfio_platform_irq.c | 100 +++++++++++++++-------
>  1 file changed, 68 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index e5dcada9e86c..ef41ecef83af 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -136,6 +136,16 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
>  	return 0;
>  }
>  
> +/*
> + * The trigger eventfd is guaranteed valid in the interrupt path
> + * and protected by the igate mutex when triggered via ioctl.
> + */
> +static void vfio_send_eventfd(struct vfio_platform_irq *irq_ctx)
> +{
> +	if (likely(irq_ctx->trigger))
> +		eventfd_signal(irq_ctx->trigger);
> +}
> +
>  static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
>  {
>  	struct vfio_platform_irq *irq_ctx = dev_id;
> @@ -155,7 +165,7 @@ static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
>  	spin_unlock_irqrestore(&irq_ctx->lock, flags);
>  
>  	if (ret == IRQ_HANDLED)
> -		eventfd_signal(irq_ctx->trigger);
> +		vfio_send_eventfd(irq_ctx);
>  
>  	return ret;
>  }
> @@ -164,52 +174,40 @@ static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
>  {
>  	struct vfio_platform_irq *irq_ctx = dev_id;
>  
> -	eventfd_signal(irq_ctx->trigger);
> +	vfio_send_eventfd(irq_ctx);
>  
>  	return IRQ_HANDLED;
>  }
>  
>  static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
> -			    int fd, irq_handler_t handler)
> +			    int fd)
>  {
>  	struct vfio_platform_irq *irq = &vdev->irqs[index];
>  	struct eventfd_ctx *trigger;
> -	int ret;
>  
>  	if (irq->trigger) {
> -		irq_clear_status_flags(irq->hwirq, IRQ_NOAUTOEN);
> -		free_irq(irq->hwirq, irq);
> -		kfree(irq->name);
> +		disable_irq(irq->hwirq);
>  		eventfd_ctx_put(irq->trigger);
>  		irq->trigger = NULL;
>  	}
>  
>  	if (fd < 0) /* Disable only */
>  		return 0;
> -	irq->name = kasprintf(GFP_KERNEL_ACCOUNT, "vfio-irq[%d](%s)",
> -			      irq->hwirq, vdev->name);
> -	if (!irq->name)
> -		return -ENOMEM;
>  
>  	trigger = eventfd_ctx_fdget(fd);
> -	if (IS_ERR(trigger)) {
> -		kfree(irq->name);
> +	if (IS_ERR(trigger))
>  		return PTR_ERR(trigger);
> -	}
>  
>  	irq->trigger = trigger;
>  
> -	irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN);
> -	ret = request_irq(irq->hwirq, handler, 0, irq->name, irq);
> -	if (ret) {
> -		kfree(irq->name);
> -		eventfd_ctx_put(trigger);
> -		irq->trigger = NULL;
> -		return ret;
> -	}
> -
> -	if (!irq->masked)
> -		enable_irq(irq->hwirq);
> +	/*
> +	 * irq->masked effectively provides nested disables within the overall
> +	 * enable relative to trigger.  Specifically request_irq() is called
> +	 * with NO_AUTOEN, therefore the IRQ is initially disabled.  The user
> +	 * may only further disable the IRQ with a MASK operations because
> +	 * irq->masked is initially false.
> +	 */
> +	enable_irq(irq->hwirq);
>  
>  	return 0;
>  }
> @@ -228,7 +226,7 @@ static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
>  		handler = vfio_irq_handler;
>  
>  	if (!count && (flags & VFIO_IRQ_SET_DATA_NONE))
> -		return vfio_set_trigger(vdev, index, -1, handler);
> +		return vfio_set_trigger(vdev, index, -1);
>  
>  	if (start != 0 || count != 1)
>  		return -EINVAL;
> @@ -236,7 +234,7 @@ static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
>  	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
>  		int32_t fd = *(int32_t *)data;
>  
> -		return vfio_set_trigger(vdev, index, fd, handler);
> +		return vfio_set_trigger(vdev, index, fd);
>  	}
>  
>  	if (flags & VFIO_IRQ_SET_DATA_NONE) {
> @@ -260,6 +258,14 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>  		    unsigned start, unsigned count, uint32_t flags,
>  		    void *data) = NULL;
>  
> +	/*
> +	 * For compatibility, errors from request_irq() are local to the
> +	 * SET_IRQS path and reflected in the name pointer.  This allows,
> +	 * for example, polling mode fallback for an exclusive IRQ failure.
> +	 */
> +	if (IS_ERR(vdev->irqs[index].name))
> +		return PTR_ERR(vdev->irqs[index].name);
> +
>  	switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
>  	case VFIO_IRQ_SET_ACTION_MASK:
>  		func = vfio_platform_set_irq_mask;
> @@ -280,7 +286,7 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>  
>  int vfio_platform_irq_init(struct vfio_platform_device *vdev)
>  {
> -	int cnt = 0, i;
> +	int cnt = 0, i, ret = 0;
>  
>  	while (vdev->get_irq(vdev, cnt) >= 0)
>  		cnt++;
> @@ -292,29 +298,54 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
>  
>  	for (i = 0; i < cnt; i++) {
>  		int hwirq = vdev->get_irq(vdev, i);
> +		irq_handler_t handler = vfio_irq_handler;
>  
> -		if (hwirq < 0)
> +		if (hwirq < 0) {
> +			ret = -EINVAL;
>  			goto err;
> +		}
>  
>  		spin_lock_init(&vdev->irqs[i].lock);
>  
>  		vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD;
>  
> -		if (irq_get_trigger_type(hwirq) & IRQ_TYPE_LEVEL_MASK)
> +		if (irq_get_trigger_type(hwirq) & IRQ_TYPE_LEVEL_MASK) {
>  			vdev->irqs[i].flags |= VFIO_IRQ_INFO_MASKABLE
>  						| VFIO_IRQ_INFO_AUTOMASKED;
> +			handler = vfio_automasked_irq_handler;
> +		}
>  
>  		vdev->irqs[i].count = 1;
>  		vdev->irqs[i].hwirq = hwirq;
>  		vdev->irqs[i].masked = false;
> +		vdev->irqs[i].name = kasprintf(GFP_KERNEL_ACCOUNT,
> +					       "vfio-irq[%d](%s)", hwirq,
> +					       vdev->name);
> +		if (!vdev->irqs[i].name) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +
> +		ret = request_irq(hwirq, handler, IRQF_NO_AUTOEN,
> +				  vdev->irqs[i].name, &vdev->irqs[i]);
> +		if (ret) {
> +			kfree(vdev->irqs[i].name);
> +			vdev->irqs[i].name = ERR_PTR(ret);
> +		}
>  	}
>  
>  	vdev->num_irqs = cnt;
>  
>  	return 0;
>  err:
> +	for (--i; i >= 0; i--) {
> +		if (!IS_ERR(vdev->irqs[i].name)) {
> +			free_irq(vdev->irqs[i].hwirq, &vdev->irqs[i]);
> +			kfree(vdev->irqs[i].name);
> +		}
> +	}
>  	kfree(vdev->irqs);
> -	return -EINVAL;
> +	return ret;
>  }
>  
>  void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
> @@ -324,7 +355,12 @@ void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
>  	for (i = 0; i < vdev->num_irqs; i++) {
>  		vfio_virqfd_disable(&vdev->irqs[i].mask);
>  		vfio_virqfd_disable(&vdev->irqs[i].unmask);
> -		vfio_set_trigger(vdev, i, -1, NULL);
> +		if (!IS_ERR(vdev->irqs[i].name)) {
> +			free_irq(vdev->irqs[i].hwirq, &vdev->irqs[i]);
> +			if (vdev->irqs[i].trigger)
> +				eventfd_ctx_put(vdev->irqs[i].trigger);
> +			kfree(vdev->irqs[i].name);
> +		}
>  	}
>  
>  	vdev->num_irqs = 0;