* [PATCH V2] virtio: disable notification hardening by default
@ 2022-06-20 2:41 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2022-06-20 2:41 UTC (permalink / raw)
To: cohuck, pasic, jasowang, mst
Cc: gor, borntraeger, agordeev, linux-s390, virtualization, kvm,
linux-kernel
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);
+#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.
+
+ Experimental: not all drivers handle this correctly at this
+ point.
+
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH V2] virtio: disable notification hardening by default
@ 2022-06-20 2:41 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2022-06-20 2:41 UTC (permalink / raw)
To: cohuck, pasic, jasowang, mst
Cc: linux-s390, gor, kvm, linux-kernel, virtualization, borntraeger,
agordeev
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);
+#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.
+
+ Experimental: not all drivers handle this correctly at this
+ point.
+
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
2022-06-20 2:41 ` Jason Wang
@ 2022-06-21 9:16 ` Cornelia Huck
-1 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2022-06-21 9:16 UTC (permalink / raw)
To: Jason Wang, pasic, jasowang, mst
Cc: gor, borntraeger, agordeev, linux-s390, virtualization, kvm,
linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
@ 2022-06-21 9:16 ` Cornelia Huck
0 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2022-06-21 9:16 UTC (permalink / raw)
To: Jason Wang, pasic, jasowang, mst
Cc: linux-s390, gor, kvm, linux-kernel, virtualization, borntraeger,
agordeev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
2022-06-21 9:16 ` Cornelia Huck
@ 2022-06-21 9:36 ` Jason Wang
-1 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2022-06-21 9:36 UTC (permalink / raw)
To: Cornelia Huck
Cc: Halil Pasic, mst, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev, linux-s390, virtualization, kvm, linux-kernel
On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> 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.
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
@ 2022-06-21 9:36 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2022-06-21 9:36 UTC (permalink / raw)
To: Cornelia Huck
Cc: linux-s390, kvm, Vasily Gorbik, mst, linux-kernel,
virtualization, Halil Pasic, Christian Borntraeger,
Alexander Gordeev
On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> 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.
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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
2022-06-21 9:36 ` Jason Wang
@ 2022-06-21 9:58 ` Cornelia Huck
-1 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2022-06-21 9:58 UTC (permalink / raw)
To: Jason Wang
Cc: linux-s390, kvm, Vasily Gorbik, mst, linux-kernel,
virtualization, Halil Pasic, Christian Borntraeger,
Alexander Gordeev
On Tue, Jun 21 2022, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck@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"?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
@ 2022-06-21 9:58 ` Cornelia Huck
0 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2022-06-21 9:58 UTC (permalink / raw)
To: Jason Wang
Cc: Halil Pasic, mst, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev, linux-s390, virtualization, kvm, linux-kernel
On Tue, Jun 21 2022, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck@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"?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
2022-06-21 9:58 ` Cornelia Huck
@ 2022-06-22 1:17 ` Jason Wang
-1 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2022-06-22 1:17 UTC (permalink / raw)
To: Cornelia Huck
Cc: Halil Pasic, mst, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev, linux-s390, virtualization, kvm, linux-kernel
On Tue, Jun 21, 2022 at 5:58 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Tue, Jun 21 2022, Jason Wang <jasowang@redhat.com> wrote:
>
> > On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck@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...
Probably, but this is the way that is used by other subsystems. E.g
CONFIG_HARDEN_USERCOPY etc.
>
> > 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"?
Right.
Thanks
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] virtio: disable notification hardening by default
@ 2022-06-22 1:17 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2022-06-22 1:17 UTC (permalink / raw)
To: Cornelia Huck
Cc: linux-s390, kvm, Vasily Gorbik, mst, linux-kernel,
virtualization, Halil Pasic, Christian Borntraeger,
Alexander Gordeev
On Tue, Jun 21, 2022 at 5:58 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Tue, Jun 21 2022, Jason Wang <jasowang@redhat.com> wrote:
>
> > On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <cohuck@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...
Probably, but this is the way that is used by other subsystems. E.g
CONFIG_HARDEN_USERCOPY etc.
>
> > 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"?
Right.
Thanks
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-06-22 1:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.