From: Cornelia Huck <cohuck@redhat.com> To: Jason Wang <jasowang@redhat.com>, pasic@linux.ibm.com, jasowang@redhat.com, mst@redhat.com Cc: gor@linux.ibm.com, borntraeger@de.ibm.com, agordeev@linux.ibm.com, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V2] virtio: disable notification hardening by default Date: Tue, 21 Jun 2022 11:16:31 +0200 [thread overview] Message-ID: <87y1xq8jgw.fsf@redhat.com> (raw) In-Reply-To: <20220620024158.2505-1-jasowang@redhat.com> On Mon, Jun 20 2022, Jason Wang <jasowang@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@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 PCI The ifdeffery looks a big ugly, but I don't have a better idea.
WARNING: multiple messages have this Message-ID (diff)
From: Cornelia Huck <cohuck@redhat.com> To: Jason Wang <jasowang@redhat.com>, pasic@linux.ibm.com, jasowang@redhat.com, mst@redhat.com Cc: linux-s390@vger.kernel.org, gor@linux.ibm.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, borntraeger@de.ibm.com, agordeev@linux.ibm.com Subject: Re: [PATCH V2] virtio: disable notification hardening by default Date: Tue, 21 Jun 2022 11:16:31 +0200 [thread overview] Message-ID: <87y1xq8jgw.fsf@redhat.com> (raw) In-Reply-To: <20220620024158.2505-1-jasowang@redhat.com> On Mon, Jun 20 2022, Jason Wang <jasowang@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@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 PCI The ifdeffery looks a big ugly, but I don't have a better idea. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-06-21 9:16 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-20 2:41 [PATCH V2] virtio: disable notification hardening by default Jason Wang 2022-06-20 2:41 ` Jason Wang 2022-06-21 9:16 ` Cornelia Huck [this message] 2022-06-21 9:16 ` Cornelia Huck 2022-06-21 9:36 ` Jason Wang 2022-06-21 9:36 ` Jason Wang 2022-06-21 9:58 ` Cornelia Huck 2022-06-21 9:58 ` Cornelia Huck 2022-06-22 1:17 ` Jason Wang 2022-06-22 1:17 ` Jason Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87y1xq8jgw.fsf@redhat.com \ --to=cohuck@redhat.com \ --cc=agordeev@linux.ibm.com \ --cc=borntraeger@de.ibm.com \ --cc=gor@linux.ibm.com \ --cc=jasowang@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=mst@redhat.com \ --cc=pasic@linux.ibm.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.