Message-ID: <Yqi7UhasBDPKCpuV@e120937-lin>
Date:   Tue, 14 Jun 2022 17:46:10 +0100
From:   Cristian Marussi <cristian.marussi@....com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Marc Zyngier <maz@...nel.org>,
        Halil Pasic <pasic@...ux.ibm.com>,
        Cornelia Huck <cohuck@...hat.com>,
        eperezma <eperezma@...hat.com>, Cindy Lu <lulu@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        Vineeth Vijayan <vneethv@...ux.ibm.com>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        linux-s390@...r.kernel.org, conghui.chen@...el.com,
        Viresh Kumar <viresh.kumar@...aro.org>,
        netdev <netdev@...r.kernel.org>, pankaj.gupta.linux@...il.com,
        sudeep.holla@....com, Bjorn Andersson <bjorn.andersson@...aro.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
Subject: Re: [PATCH V6 8/9] virtio: harden vring IRQ

On Tue, Jun 14, 2022 at 03:40:21PM +0800, Jason Wang wrote:
> On Mon, Jun 13, 2022 at 5:28 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> >

Hi Jason,

> > On Mon, Jun 13, 2022 at 05:14:59PM +0800, Jason Wang wrote:
> > > On Mon, Jun 13, 2022 at 5:08 PM Jason Wang <jasowang@...hat.com> wrote:
> > > >
> > > > On Mon, Jun 13, 2022 at 4:59 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > >
> > > > > On Mon, Jun 13, 2022 at 04:51:08PM +0800, Jason Wang wrote:
> > > > > > On Mon, Jun 13, 2022 at 4:19 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 13, 2022 at 04:07:09PM +0800, Jason Wang wrote:
> > > > > > > > On Mon, Jun 13, 2022 at 3:23 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Jun 13, 2022 at 01:26:59PM +0800, Jason Wang wrote:
> > > > > > > > > > On Sat, Jun 11, 2022 at 1:12 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, May 27, 2022 at 02:01:19PM +0800, Jason Wang wrote:
> > > > > > > > > > > > This is a rework on the previous IRQ hardening that is done for
> > > > > > > > > > > > virtio-pci where several drawbacks were found and were reverted:
> > > > > > > > > > > >
> > > > > > > > > > > > 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ
> > > > > > > > > > > >    that is used by some device such as virtio-blk
> > > > > > > > > > > > 2) done only for PCI transport
> > > > > > > > > > > >
> > > > > > > > > > > > The vq->broken is re-used in this patch for implementing the IRQ
> > > > > > > > > > > > hardening. The vq->broken is set to true during both initialization
> > > > > > > > > > > > and reset. And the vq->broken is set to false in
> > > > > > > > > > > > virtio_device_ready(). Then vring_interrupt() can check and return
> > > > > > > > > > > > when vq->broken is true. And in this case, switch to return IRQ_NONE
> > > > > > > > > > > > to let the interrupt core aware of such invalid interrupt to prevent
> > > > > > > > > > > > IRQ storm.
> > > > > > > > > > > >
> > > > > > > > > > > > The reason of using a per queue variable instead of a per device one
> > > > > > > > > > > > is that we may need it for per queue reset hardening in the future.
> > > > > > > > > > > >
> > > > > > > > > > > > Note that the hardening is only done for vring interrupt since the
> > > > > > > > > > > > config interrupt hardening is already done in commit 22b7050a024d7
> > > > > > > > > > > > ("virtio: defer config changed notifications"). But the method that is
> > > > > > > > > > > > used by config interrupt can't be reused by the vring interrupt
> > > > > > > > > > > > handler because it uses spinlock to do the synchronization which is
> > > > > > > > > > > > expensive.
> > > > > > > > > > > >
> > > > > > > > > > > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > > > > > > > > > > Cc: Peter Zijlstra <peterz@...radead.org>
> > > > > > > > > > > > Cc: "Paul E. McKenney" <paulmck@...nel.org>
> > > > > > > > > > > > Cc: Marc Zyngier <maz@...nel.org>
> > > > > > > > > > > > Cc: Halil Pasic <pasic@...ux.ibm.com>
> > > > > > > > > > > > Cc: Cornelia Huck <cohuck@...hat.com>
> > > > > > > > > > > > Cc: Vineeth Vijayan <vneethv@...ux.ibm.com>
> > > > > > > > > > > > Cc: Peter Oberparleiter <oberpar@...ux.ibm.com>
> > > > > > > > > > > > Cc: linux-s390@...r.kernel.org
> > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@...hat.com>
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Jason, I am really concerned by all the fallout.
> > > > > > > > > > > I propose adding a flag to suppress the hardening -
> > > > > > > > > > > this will be a debugging aid and a work around for
> > > > > > > > > > > users if we find more buggy drivers.
> > > > > > > > > > >
> > > > > > > > > > > suppress_interrupt_hardening ?
> > > > > > > > > >
> > > > > > > > > > I can post a patch but I'm afraid if we disable it by default, it
> > > > > > > > > > won't be used by the users so there's no way for us to receive the bug
> > > > > > > > > > report. Or we need a plan to enable it by default.
> > > > > > > > > >
> > > > > > > > > > It's rc2, how about waiting for 1 and 2 rc? Or it looks better if we
> > > > > > > > > > simply warn instead of disable it by default.
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > I meant more like a flag in struct virtio_driver.
> > > > > > > > > For now, could you audit all drivers which don't call _ready?
> > > > > > > > > I found 5 of these:
> > > > > > > > >
> > > > > > > > > drivers/bluetooth/virtio_bt.c
> > > > > > > >
> > > > > > > > This driver seems to be fine, it doesn't use the device/vq in its probe().
> > > > > > >
> > > > > > >
> > > > > > > But it calls hci_register_dev and that in turn queues all kind of
> > > > > > > work. Also, can linux start using the device immediately after
> > > > > > > it's registered?
> > > > > >
> > > > > > So I think the driver is allowed to queue before DRIVER_OK.
> > > > >
> > > > > it's not allowed to kick
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > > If yes,
> > > > > > the only side effect is the delay of the tx interrupt after DRIVER_OK
> > > > > > for a well behaved device.
> > > > >
> > > > > your patches drop the interrupt though, it won't be just delayed.
> > > >
> > > > For a well behaved device, it can only trigger the interrupt after DRIVER_OK.
> > > >
> > > > So for virtio bt, it works like:
> > > >
> > > > 1) driver queue buffer and kick
> > > > 2) driver set DRIVER_OK
> > > > 3) device start to process the buffer
> > > > 4) device send an notification
> > > >
> > > > The only risk is that the virtqueue could be filled before DRIVER_OK,
> > > > or anything I missed?
> > >
> > > btw, hci has an open and close method and we do rx refill in
> > > hdev->open, so we're probably fine here.
> > >
> > > Thanks
> >
> >
> > Sounds good. Now to audit the rest of them from this POV ;)
> 
> Adding maintainers.
> 
> >
> >  drivers/i2c/busses/i2c-virtio.c
> 
> It looks to me the device could be used immediately after
> i2c_add_adapter() return. So we probably need to add
> virtio_device_ready() before that. Fortunately, there's no rx vq in
> i2c and the callback looks safe if the callback is called before the
> i2c registration and after virtio_device_ready().
> 
> >  drivers/net/caif/caif_virtio.c
> 
> A networking device, RX is backed by vringh so we don't need to
> refill. TX is backed by virtio and is available until ndo_open. So
> it's fine to let the core to set DRIVER_OK after probe().
> 
> >  drivers/nvdimm/virtio_pmem.c
> 
> It doesn't use interrupt so far, so it has nothing to do with the IRQ hardening.
> 
> But the device could be used by the subsystem immediately after
> nvdimm_pmem_region_create(), this means the flush could be issued
> before DRIVER_OK. We need virtio_device_ready() before. We don't have
> a RX virtqueue and the callback looks safe if the callback is called
> after virtio_device_ready() but before the nvdimm region creating.
> 
> And it looks to me there's a race between the assignment of
> provider_data and virtio_pmem_flush(). If the flush was issued before
> the assignment we will end up with a NULL pointer dereference. This is
> something we need to fix.
> 
> >  arm_scmi
> 
> It looks to me the singleton device could be used by SCMI immediately after
> 
>         /* Ensure initialized scmi_vdev is visible */
>         smp_store_mb(scmi_vdev, vdev);
> 
> So we probably need to do virtio_device_ready() before that. It has an
> optional rx queue but the filling is done after the above assignment,
> so it's safe. And the callback looks safe is a callback is triggered
> after virtio_device_ready() buy before the above assignment.
> 

I wanted to give it a go at this series testing it on the context of
SCMI but it does not apply

- not on a v5.18:

17:33 $ git rebase -i v5.18
17:33 $ git am ./v6_20220527_jasowang_rework_on_the_irq_hardening_of_virtio.mbx
Applying: virtio: use virtio_device_ready() in virtio_device_restore()
Applying: virtio: use virtio_reset_device() when possible
Applying: virtio: introduce config op to synchronize vring callbacks
Applying: virtio-pci: implement synchronize_cbs()
Applying: virtio-mmio: implement synchronize_cbs()
error: patch failed: drivers/virtio/virtio_mmio.c:345
error: drivers/virtio/virtio_mmio.c: patch does not apply
Patch failed at 0005 virtio-mmio: implement synchronize_cbs()

- neither on a v5.19-rc2:

17:33 $ git rebase -i v5.19-rc2 
17:35 $ git am ./v6_20220527_jasowang_rework_on_the_irq_hardening_of_virtio.mbx
Applying: virtio: use virtio_device_ready() in virtio_device_restore()
error: patch failed: drivers/virtio/virtio.c:526
error: drivers/virtio/virtio.c: patch does not apply
Patch failed at 0001 virtio: use virtio_device_ready() in
virtio_device_restore()
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".

... what I should take as base ?

Thanks,
Cristian

Powered by blists - more mailing lists