Virtualization - Jun 2022 - [PATCH V2] virtio: disable notification hardening by default

On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck at redhat.com>
wrote:
>
> On Mon, Jun 20 2022, Jason Wang <jasowang at redhat.com> wrote:
>
> > We try to harden virtio device notifications in 8b4ec69d7e09
("virtio:
> > harden vring IRQ"). It works with the assumption that the driver
or
> > core can properly call virtio_device_ready() at the right
> > place. Unfortunately, this seems to be not true and uncover various
> > bugs of the existing drivers, mainly the issue of using
> > virtio_device_ready() incorrectly.
> >
> > So let's having a Kconfig option and disable it by default. It
gives
> > us a breath to fix the drivers and then we can consider to enable it
> > by default.
> >
> > Signed-off-by: Jason Wang <jasowang at redhat.com>
> > ---
> > Changes since V1:
> > - tweak the Kconfig prompt
> > - don't hold spinlock for IRQ path in s390
> > ---
> >  drivers/s390/virtio/virtio_ccw.c |  4 ++++
> >  drivers/virtio/Kconfig           | 11 +++++++++++
> >  drivers/virtio/virtio.c          |  2 ++
> >  drivers/virtio/virtio_ring.c     | 12 ++++++++++++
> >  include/linux/virtio_config.h    |  2 ++
> >  5 files changed, 31 insertions(+)
> >
> > diff --git a/drivers/s390/virtio/virtio_ccw.c
b/drivers/s390/virtio/virtio_ccw.c
> > index 97e51c34e6cf..89bbf7ccfdd1 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -1136,8 +1136,10 @@ static void virtio_ccw_int_handler(struct
ccw_device *cdev,
> >                       vcdev->err = -EIO;
> >       }
> >       virtio_ccw_check_activity(vcdev, activity);
> > +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> >       /* Interrupts are disabled here */
> >       read_lock(&vcdev->irq_lock);
>
> Should we add a comment that this pairs with
> virtio_ccw_synchronize_cbs()? Just to avoid future headscratching as to
> why this lock is only needed when notification hardening is enabled.
Fine.
>
> > +#endif
> >       for_each_set_bit(i, indicators(vcdev),
> >                        sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> >               /* The bit clear must happen before the vring kick. */
> > @@ -1146,7 +1148,9 @@ static void virtio_ccw_int_handler(struct
ccw_device *cdev,
> >               vq = virtio_ccw_vq_by_ind(vcdev, i);
> >               vring_interrupt(0, vq);
> >       }
> > +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> >       read_unlock(&vcdev->irq_lock);
> > +#endif
> >       if (test_bit(0, indicators2(vcdev))) {
> >               virtio_config_changed(&vcdev->vdev);
> >               clear_bit(0, indicators2(vcdev));
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index b5adf6abd241..96ec56d44b91 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -35,6 +35,17 @@ menuconfig VIRTIO_MENU
> >
> >  if VIRTIO_MENU
> >
> > +config VIRTIO_HARDEN_NOTIFICATION
> > +        bool "Harden virtio notification"
> > +        help
> > +          Enable this to harden the device notifications and supress
> > +          the ones that are illegal.
>
> "...and suppress those that happen at a time where notifications are
> illegal." ?
Ok.
>
> > +
> > +          Experimental: not all drivers handle this correctly at this
> > +          point.
>
> "Note that several drivers still have bugs that may cause crashes or
> hangs when correct handling of notifications is enforced; depending on
> the subset of drivers and devices you use, this may or may not work."
>
> Or is that too verbose?
Looks fine.
>
> > +
> > +          If unsure, say N.
> > +
> >  config VIRTIO_PCI
> >       tristate "PCI driver for virtio devices"
> >       depends on PCI
>
> The ifdeffery looks a big ugly, but I don't have a better idea.
I guess you meant the ccw part, I leave the spinlock here in V1, but
Michael prefers to have that.

In the future, we may consider removing that, one possible way is to
have a per driver boolean for the hardening.

Tanks
>

On Tue, Jun 21 2022, Jason Wang <jasowang at redhat.com> wrote:
> On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck at redhat.com>
wrote:
>>
>> The ifdeffery looks a big ugly, but I don't have a better idea.
>
> I guess you meant the ccw part, I leave the spinlock here in V1, but
> Michael prefers to have that.
Not doing the locking dance is good; I think the #ifdefs all over are a
bit ugly, but as I said, I can't think of a good, less-ugly way...
> In the future, we may consider removing that, one possible way is to
> have a per driver boolean for the hardening.
As in "we've reviewed and tested this driver, so let's turn it on
for
every device bound to it"?