Re: [PATCH V3 05/10] virtio-pci: harden INTX interrupts

[Michael S. Tsirkin]

On Wed, Mar 09, 2022 at 10:45:49AM +0000, Marc Zyngier wrote:
> [Adding Will to check on my understanding of the interactions between
>  spinlocks and WRITE_ONCE()]
> 
> On Tue, 19 Oct 2021 08:01:47 +0100,
> Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > 
> > This patch tries to make sure the virtio interrupt handler for INTX
> > won't be called after a reset and before virtio_device_ready(). We
> > can't use IRQF_NO_AUTOEN since we're using shared interrupt
> > (IRQF_SHARED). So this patch tracks the INTX enabling status in a new
> > intx_soft_enabled variable and toggle it during in
> > vp_disable/enable_vectors(). The INTX interrupt handler will check
> > intx_soft_enabled before processing the actual interrupt.
> > 
> > Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > ---
> >  drivers/virtio/virtio_pci_common.c | 23 +++++++++++++++++++++--
> >  drivers/virtio/virtio_pci_common.h |  1 +
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 8d8f83aca721..1bce254a462a 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -30,8 +30,16 @@ void vp_disable_cbs(struct virtio_device *vdev)
> >  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >  	int i;
> >  
> > -	if (vp_dev->intx_enabled)
> > +	if (vp_dev->intx_enabled) {
> > +		/*
> > +		 * The below synchronize() guarantees that any
> > +		 * interrupt for this line arriving after
> > +		 * synchronize_irq() has completed is guaranteed to see
> > +		 * intx_soft_enabled == false.
> > +		 */
> > +		WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> >  		synchronize_irq(vp_dev->pci_dev->irq);
> > +	}
> >  
> >  	for (i = 0; i < vp_dev->msix_vectors; ++i)
> >  		disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > @@ -43,8 +51,16 @@ void vp_enable_cbs(struct virtio_device *vdev)
> >  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >  	int i;
> >  
> > -	if (vp_dev->intx_enabled)
> > +	if (vp_dev->intx_enabled) {
> > +		disable_irq(vp_dev->pci_dev->irq);
> > +		/*
> > +		 * The above disable_irq() provides TSO ordering and
> > +		 * as such promotes the below store to store-release.
> > +		 */
> > +		WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> 
> What do you mean by TSO here? AFAICT, the CPU is allowed hoist this
> write up into the lock used by disable_irq(), as the unlock only has
> release semantics. Is that what you are relying on? I don't see how
> this upgrades WRITE_ONCE() to have release semantics.
> 
> > +		enable_irq(vp_dev->pci_dev->irq);
> 
> Same thing does here: my understanding is that the write can be pushed
> down into the lock, which has acquire semantics only.
> 
> Thanks,
> 
> 	M.

Overall I feel what we are doing here is very standard and should be
pretty common for a driver that wants to be protected against a
malicious device:


1- get IRQ
2- initialize device with IRQ
3- enable IRQ

Doing it in the core kernel helps make sure interrupts are
not lost if they trigger during 2.

Without core kernel support one has to refactor the driver along the lines of:

a- initialize driver
b- get IRQ
c- initialize device with IRQ

and this is often tricky especially if one wants to do things like
discover device configuration and reconfigure the driver accordingly.


> -- 
> Without deviation from the norm, progress is not possible.