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

[Marc Zyngier]

On Wed, 09 Mar 2022 11:27:42 +0000,
"Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> 
> 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.

But this isn't the core kernel. You're doing that in some random
driver (and even more, only for the PCI version of that driver).

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

But this isn't what this patch is about, is it? You are just tracking
whether interrupts are enabled or not. To which my reply to you on the
previous patch still applies (this is the wrong place to track such
state).

You also haven't answered my question: what are your ordering
expectations wrt the WRITE_ONCE() above? The comment says 'TSO', and I
don't really understand how this is enforced.

Thanks,

	M.

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