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

[Alex Williamson <alex.williamson@redhat.com>]

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