Date:   Wed, 9 Mar 2022 07:30:34 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Jason Wang <jasowang@...hat.com>, Will Deacon <will@...nel.org>,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, f.hetzelt@...berlin.de,
        david.kaplan@....com, konrad.wilk@...cle.com,
        Boqun Feng <boqun.feng@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E . McKenney" <paulmck@...nel.org>, keirf@...gle.com
Subject: Re: [PATCH V3 05/10] virtio-pci: harden INTX interrupts

On Wed, Mar 09, 2022 at 12:14:14PM +0000, Marc Zyngier wrote:
> On Wed, 09 Mar 2022 11:27:42 +0000,
> "Michael S. Tsirkin" <mst@...hat.com> 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@...hat.com> 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@...il.com>
> > > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > > Cc: Peter Zijlstra <peterz@...radead.org>
> > > > Cc: Paul E. McKenney <paulmck@...nel.org>
> > > > Signed-off-by: Jason Wang <jasowang@...hat.com>
> > > > ---
> > > >  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).


Oh, I'm answering on the INTX patch. I was referring to the MSIX
path actually. Yes INTX can not currently start in disabled state
because it's shared.


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

It's not that we care. What we need is to make sure that interrupts sent
before driver is ready to accept them do not cause a callback.
If there was a way to tell kernel "I'm ready to accept callbacks now"
problem would be solved.



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

It's based on Peter's suggestion:
https://lore.kernel.org/r/YUCBZjjk77q8JS4f%40hirez.programming.kicks-ass.net


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

Powered by blists - more mailing lists