Cornelia Huck
2022-Jun-21 09:16 UTC
[PATCH V2] virtio: disable notification hardening by default
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.> +#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." ?> + > + 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?> + > + If unsure, say N. > + > config VIRTIO_PCI > tristate "PCI driver for virtio devices" > depends on PCIThe ifdeffery looks a big ugly, but I don't have a better idea.
Jason Wang
2022-Jun-21 09:36 UTC
[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>