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.
> +#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.
