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

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

[Jiang Yi]

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().

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

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

[Marc Zyngier]

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.

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

[Auger Eric]

Hi,
On 11/27/19 5:49 PM, Jiang Yi wrote:
> 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().
> 
> 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);
Looks the right way too

Reviewed-by: Eric Auger <eric.auger@redhat.com>

May be worth checking it does not alter the x86 posted interrupt setup
though. update_pi_irte() gets called. I was concerned about the fact the
interrupts may be enabled when doing the unregistration (TBC). The
irq_bypass framework offers producer start/stop callbacks that would
allow to handle this but nobody use them atm.

Thanks

Eric
>  		kfree(vdev->ctx[vector].name);
>  		eventfd_ctx_put(vdev->ctx[vector].trigger);
>  		vdev->ctx[vector].trigger = NULL;
>  	}
>  
>  	if (fd < 0)
>  		return 0;
>  
> 

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

[Alex Williamson]

On Wed, 27 Nov 2019 18:20:14 +0000
Marc Zyngier <maz@kernel.org> wrote:

> 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.

Yep, it seems a lot safer to reverse this but we need to incorporate
some of Marc's rationale above into the commit log to justify the
stable and fixes tags.  Here's an attempt:

--
free_irq() may release resources required by the irqbypass
del_producer() callback.  Notably on arm64 with 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]

This has the potential to wedge hardware.  Re-order to free the IRQ
after unregistering the irqbypass producer, which also provides the
proper mirror of setup ordering.
--

Cc'ing some usual suspects from AMD, Intel, and Power where the
kvm_arch_irq_bypass_del_producer() callback is also implemented.
Thanks,

Alex

> > 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.
>