On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
>
> On Mon, Jun 20 2022, Jason Wang <jasowang@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
