From: Jason Wang <jasowang@redhat.com> To: cohuck@redhat.com, pasic@linux.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com, borntraeger@de.ibm.com, agordeev@linux.ibm.com, mst@redhat.com, jasowang@redhat.com, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: ben@decadent.org.uk, david@redhat.com Subject: [PATCH V3] virtio: disable notification hardening by default Date: Wed, 22 Jun 2022 09:29:40 +0800 [thread overview] Message-ID: <20220622012940.21441-1-jasowang@redhat.com> (raw) 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 V2: - Tweak the Kconfig help - Add comment for the read_lock() pairing in virtio_ccw --- drivers/s390/virtio/virtio_ccw.c | 9 ++++++++- drivers/virtio/Kconfig | 13 +++++++++++++ drivers/virtio/virtio.c | 2 ++ drivers/virtio/virtio_ring.c | 12 ++++++++++++ include/linux/virtio_config.h | 2 ++ 5 files changed, 37 insertions(+), 1 deletion(-) diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 97e51c34e6cf..1f6a358f65f0 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -1136,8 +1136,13 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev, vcdev->err = -EIO; } virtio_ccw_check_activity(vcdev, activity); - /* Interrupts are disabled here */ +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION + /* + * Paried with virtio_ccw_synchronize_cbs() and interrupts are + * disabled here. + */ read_lock(&vcdev->irq_lock); +#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 +1151,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..c04f370a1e5c 100644 --- a/drivers/virtio/Kconfig +++ b/drivers/virtio/Kconfig @@ -35,6 +35,19 @@ menuconfig VIRTIO_MENU if VIRTIO_MENU +config VIRTIO_HARDEN_NOTIFICATION + bool "Harden virtio notification" + help + Enable this to harden the device notifications and suppress + those that happen at a time where notifications are illegal. + + Experimental: 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. + + If unsure, say N. + config VIRTIO_PCI tristate "PCI driver for virtio devices" depends on PCI diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index ef04a96942bf..21dc08d2f32d 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -220,6 +220,7 @@ static int virtio_features_ok(struct virtio_device *dev) * */ void virtio_reset_device(struct virtio_device *dev) { +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION /* * The below virtio_synchronize_cbs() guarantees that any * interrupt for this line arriving after @@ -228,6 +229,7 @@ void virtio_reset_device(struct virtio_device *dev) */ virtio_break_device(dev); virtio_synchronize_cbs(dev); +#endif dev->config->reset(dev); } diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 13a7348cedff..d9d3b6e201fb 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -1688,7 +1688,11 @@ static struct virtqueue *vring_create_virtqueue_packed( vq->we_own_ring = true; vq->notify = notify; vq->weak_barriers = weak_barriers; +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION vq->broken = true; +#else + vq->broken = false; +#endif vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; @@ -2135,9 +2139,13 @@ irqreturn_t vring_interrupt(int irq, void *_vq) } if (unlikely(vq->broken)) { +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION dev_warn_once(&vq->vq.vdev->dev, "virtio vring IRQ raised before DRIVER_OK"); return IRQ_NONE; +#else + return IRQ_HANDLED; +#endif } /* Just a hint for performance: so it's ok that this can be racy! */ @@ -2180,7 +2188,11 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, vq->we_own_ring = false; vq->notify = notify; vq->weak_barriers = weak_barriers; +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION vq->broken = true; +#else + vq->broken = false; +#endif vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 9a36051ceb76..d15c3cdda2d2 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -257,6 +257,7 @@ void virtio_device_ready(struct virtio_device *dev) WARN_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION /* * The virtio_synchronize_cbs() makes sure vring_interrupt() * will see the driver specific setup if it sees vq->broken @@ -264,6 +265,7 @@ void virtio_device_ready(struct virtio_device *dev) */ virtio_synchronize_cbs(dev); __virtio_unbreak_device(dev); +#endif /* * The transport should ensure the visibility of vq->broken * before setting DRIVER_OK. See the comments for the transport -- 2.25.1
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: cohuck@redhat.com, pasic@linux.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com, borntraeger@de.ibm.com, agordeev@linux.ibm.com, mst@redhat.com, jasowang@redhat.com, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: ben@decadent.org.uk Subject: [PATCH V3] virtio: disable notification hardening by default Date: Wed, 22 Jun 2022 09:29:40 +0800 [thread overview] Message-ID: <20220622012940.21441-1-jasowang@redhat.com> (raw) 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 V2: - Tweak the Kconfig help - Add comment for the read_lock() pairing in virtio_ccw --- drivers/s390/virtio/virtio_ccw.c | 9 ++++++++- drivers/virtio/Kconfig | 13 +++++++++++++ drivers/virtio/virtio.c | 2 ++ drivers/virtio/virtio_ring.c | 12 ++++++++++++ include/linux/virtio_config.h | 2 ++ 5 files changed, 37 insertions(+), 1 deletion(-) diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 97e51c34e6cf..1f6a358f65f0 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -1136,8 +1136,13 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev, vcdev->err = -EIO; } virtio_ccw_check_activity(vcdev, activity); - /* Interrupts are disabled here */ +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION + /* + * Paried with virtio_ccw_synchronize_cbs() and interrupts are + * disabled here. + */ read_lock(&vcdev->irq_lock); +#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 +1151,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..c04f370a1e5c 100644 --- a/drivers/virtio/Kconfig +++ b/drivers/virtio/Kconfig @@ -35,6 +35,19 @@ menuconfig VIRTIO_MENU if VIRTIO_MENU +config VIRTIO_HARDEN_NOTIFICATION + bool "Harden virtio notification" + help + Enable this to harden the device notifications and suppress + those that happen at a time where notifications are illegal. + + Experimental: 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. + + If unsure, say N. + config VIRTIO_PCI tristate "PCI driver for virtio devices" depends on PCI diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index ef04a96942bf..21dc08d2f32d 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -220,6 +220,7 @@ static int virtio_features_ok(struct virtio_device *dev) * */ void virtio_reset_device(struct virtio_device *dev) { +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION /* * The below virtio_synchronize_cbs() guarantees that any * interrupt for this line arriving after @@ -228,6 +229,7 @@ void virtio_reset_device(struct virtio_device *dev) */ virtio_break_device(dev); virtio_synchronize_cbs(dev); +#endif dev->config->reset(dev); } diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 13a7348cedff..d9d3b6e201fb 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -1688,7 +1688,11 @@ static struct virtqueue *vring_create_virtqueue_packed( vq->we_own_ring = true; vq->notify = notify; vq->weak_barriers = weak_barriers; +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION vq->broken = true; +#else + vq->broken = false; +#endif vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; @@ -2135,9 +2139,13 @@ irqreturn_t vring_interrupt(int irq, void *_vq) } if (unlikely(vq->broken)) { +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION dev_warn_once(&vq->vq.vdev->dev, "virtio vring IRQ raised before DRIVER_OK"); return IRQ_NONE; +#else + return IRQ_HANDLED; +#endif } /* Just a hint for performance: so it's ok that this can be racy! */ @@ -2180,7 +2188,11 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, vq->we_own_ring = false; vq->notify = notify; vq->weak_barriers = weak_barriers; +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION vq->broken = true; +#else + vq->broken = false; +#endif vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 9a36051ceb76..d15c3cdda2d2 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -257,6 +257,7 @@ void virtio_device_ready(struct virtio_device *dev) WARN_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION /* * The virtio_synchronize_cbs() makes sure vring_interrupt() * will see the driver specific setup if it sees vq->broken @@ -264,6 +265,7 @@ void virtio_device_ready(struct virtio_device *dev) */ virtio_synchronize_cbs(dev); __virtio_unbreak_device(dev); +#endif /* * The transport should ensure the visibility of vq->broken * before setting DRIVER_OK. See the comments for the transport -- 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next reply other threads:[~2022-06-22 1:29 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-22 1:29 Jason Wang [this message] 2022-06-22 1:29 ` [PATCH V3] virtio: disable notification hardening by default Jason Wang 2022-06-22 7:02 ` Michael S. Tsirkin 2022-06-22 7:02 ` Michael S. Tsirkin 2022-06-22 7:09 ` Jason Wang 2022-06-22 7:09 ` Jason Wang 2022-06-24 6:31 ` Michael S. Tsirkin 2022-06-24 6:31 ` Michael S. Tsirkin 2022-06-27 2:50 ` Jason Wang 2022-06-27 2:50 ` Jason Wang 2022-06-27 7:33 ` Michael S. Tsirkin 2022-06-27 7:33 ` Michael S. Tsirkin 2022-06-27 8:11 ` Jason Wang 2022-06-27 8:11 ` Jason Wang 2022-06-27 9:52 ` Michael S. Tsirkin 2022-06-27 9:52 ` Michael S. Tsirkin 2022-06-28 3:49 ` Jason Wang 2022-06-28 3:49 ` Jason Wang 2022-06-28 4:59 ` Michael S. Tsirkin 2022-06-28 4:59 ` Michael S. Tsirkin 2022-06-28 6:17 ` Jason Wang 2022-06-28 6:17 ` Jason Wang 2022-06-28 6:24 ` Michael S. Tsirkin 2022-06-28 6:24 ` Michael S. Tsirkin 2022-06-28 6:32 ` Jason Wang 2022-06-28 6:32 ` Jason Wang 2022-06-28 6:44 ` Michael S. Tsirkin 2022-06-28 6:44 ` Michael S. Tsirkin 2022-06-29 4:07 ` Jason Wang 2022-06-29 4:07 ` Jason Wang 2022-06-29 6:31 ` Michael S. Tsirkin 2022-06-29 6:31 ` Michael S. Tsirkin 2022-06-29 7:02 ` Jason Wang 2022-06-29 7:02 ` Jason Wang 2022-06-29 7:15 ` Michael S. Tsirkin 2022-06-29 7:15 ` Michael S. Tsirkin 2022-06-29 8:34 ` Jason Wang 2022-06-29 8:34 ` Jason Wang 2022-06-29 8:52 ` Michael S. Tsirkin 2022-06-29 8:52 ` Michael S. Tsirkin 2022-06-30 2:01 ` Jason Wang 2022-06-30 2:01 ` Jason Wang 2022-06-30 8:38 ` Michael S. Tsirkin 2022-06-30 8:38 ` Michael S. Tsirkin 2022-07-04 4:23 ` Jason Wang 2022-07-04 4:23 ` Jason Wang 2022-07-04 6:22 ` Michael S. Tsirkin 2022-07-04 6:22 ` Michael S. Tsirkin 2022-07-04 6:40 ` Jason Wang 2022-07-04 6:40 ` Jason Wang 2022-07-04 7:00 ` Michael S. Tsirkin 2022-07-04 7:00 ` Michael S. Tsirkin 2022-06-22 8:32 ` Cornelia Huck 2022-06-22 8:32 ` Cornelia Huck 2022-06-24 8:41 ` Xuan Zhuo 2022-06-24 8:41 ` Xuan Zhuo 2022-06-24 9:05 ` Michael S. Tsirkin 2022-06-24 9:05 ` Michael S. Tsirkin
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=20220622012940.21441-1-jasowang@redhat.com \ --to=jasowang@redhat.com \ --cc=agordeev@linux.ibm.com \ --cc=ben@decadent.org.uk \ --cc=borntraeger@de.ibm.com \ --cc=cohuck@redhat.com \ --cc=david@redhat.com \ --cc=gor@linux.ibm.com \ --cc=hca@linux.ibm.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.