Re: [PATCH] vfio: call irq_bypass_unregister_producer() before freeing irq

[Marc Zyngier <maz@kernel.org>]

On Wed, 27 Nov 2019 16:49:10 +0000,
Jiang Yi <giangyi@amazon.com> wrote:

Hi Jiang,

Thanks for spotting this!

> Since irq_bypass_register_producer() is called after request_irq(), we
> should do tear-down in reverse order: irq_bypass_unregister_producer()
> then free_irq().

More importantly, free_irq() is going to releases resources that can
still be required by the del_producer callback. Notably, for arm64 and
GICv4:

free_irq(irq)
  __free_irq(irq)
    irq_domain_deactivate_irq(irq)
      its_irq_domain_deactivate()
        [unmap the VLPI from the ITS]

kvm_arch_irq_bypass_del_producer(cons, prod)
  kvm_vgic_v4_unset_forwarding(kvm, irq, ...)
    its_unmap_vlpi(irq)
      [Unmap the VLPI from the ITS (again), remap the original LPI]

which isn't great, and has the potential to wedge the HW. Reversing
the two makes more sense: Unmap the VLPI, remap the LPI, and finally
unmap the LPI. I haven't checked what it does with VT-D.

> Signed-off-by: Jiang Yi <giangyi@amazon.com>
> ---
>  drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 3fa3f728fb39..2056f3f85f59 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -289,18 +289,18 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
>  	int irq, ret;
>  
>  	if (vector < 0 || vector >= vdev->num_ctx)
>  		return -EINVAL;
>  
>  	irq = pci_irq_vector(pdev, vector);
>  
>  	if (vdev->ctx[vector].trigger) {
> -		free_irq(irq, vdev->ctx[vector].trigger);
>  		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
> +		free_irq(irq, vdev->ctx[vector].trigger);
>  		kfree(vdev->ctx[vector].name);
>  		eventfd_ctx_put(vdev->ctx[vector].trigger);
>  		vdev->ctx[vector].trigger = NULL;
>  	}
>  
>  	if (fd < 0)
>  		return 0;
>  
> -- 
> 2.17.1
> 
> 

FWIW:

Cc: stable@vger.kernel.org # v4.4+
Fixes: 6d7425f109d26 ("vfio: Register/unregister irq_bypass_producer")
Reviewed-by: Marc Zyngier <maz@kernel.org>

Thanks again,

	M.

-- 
Jazz is not dead, it just smells funny.