| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220406080135-mutt-send-email-mst@kernel.org>
Date: Wed, 6 Apr 2022 08:04:46 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org, maz@...nel.org, tglx@...utronix.de,
peterz@...radead.org, sgarzare@...hat.com,
"Paul E. McKenney" <paulmck@...nel.org>
Subject: Re: [PATCH V2 5/5] virtio: harden vring IRQ
On Wed, Apr 06, 2022 at 04:35:38PM +0800, Jason Wang wrote:
> This is a rework on the previous IRQ hardening that is done for
> virtio-pci where several drawbacks were found and were reverted:
>
> 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ
> that is used by some device such as virtio-blk
> 2) done only for PCI transport
>
> In this patch, we tries to borrow the idea from the INTX IRQ hardening
> in the reverted commit 080cd7c3ac87 ("virtio-pci: harden INTX interrupts")
> by introducing a global device_ready variable for each
> virtio_device. Then we can to toggle it during
> virtio_reset_device()/virtio_device_ready(). A
> virtio_synchornize_vqs() is used in both virtio_device_ready() and
> virtio_reset_device() to synchronize with the vring callbacks. With
> this, vring_interrupt() can return check and early if driver_ready is
> false.
>
> Note that the hardening is only done for vring interrupt since the
> config interrupt hardening is already done in commit 22b7050a024d7
> ("virtio: defer config changed notifications"). But the method that is
> used by config interrupt can't be reused by the vring interrupt
> handler because it uses spinlock to do the synchronization which is
> expensive.
>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Marc Zyngier <maz@...nel.org>
> Signed-off-by: Jason Wang <jasowang@...hat.com>
> ---
> drivers/virtio/virtio.c | 11 +++++++++++
> drivers/virtio/virtio_ring.c | 9 ++++++++-
> include/linux/virtio.h | 2 ++
> include/linux/virtio_config.h | 8 ++++++++
> 4 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 8dde44ea044a..2f3a6f8e3d9c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -220,6 +220,17 @@ static int virtio_features_ok(struct virtio_device *dev)
> * */
> void virtio_reset_device(struct virtio_device *dev)
> {
> + if (READ_ONCE(dev->driver_ready)) {
> + /*
> + * The below virtio_synchronize_vqs() guarantees that any
> + * interrupt for this line arriving after
> + * virtio_synchronize_vqs() has completed is guaranteed to see
> + * driver_ready == false.
> + */
> + WRITE_ONCE(dev->driver_ready, false);
> + virtio_synchronize_vqs(dev);
> + }
> +
> dev->config->reset(dev);
> }
> EXPORT_SYMBOL_GPL(virtio_reset_device);
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index cfb028ca238e..a4592e55c9f8 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2127,10 +2127,17 @@ static inline bool more_used(const struct vring_virtqueue *vq)
> return vq->packed_ring ? more_used_packed(vq) : more_used_split(vq);
> }
>
> -irqreturn_t vring_interrupt(int irq, void *_vq)
> +irqreturn_t vring_interrupt(int irq, void *v)
> {
> + struct virtqueue *_vq = v;
> + struct virtio_device *vdev = _vq->vdev;
> struct vring_virtqueue *vq = to_vvq(_vq);
>
> + if (!READ_ONCE(vdev->driver_ready)) {
I am not sure why we need READ_ONCE here, it's done under lock.
Accrdingly, same thing above for READ_ONCE and WRITE_ONCE.
> + dev_warn_once(&vdev->dev, "virtio vring IRQ raised before DRIVER_OK");
> + return IRQ_NONE;
> + }
> +
> if (!more_used(vq)) {
> pr_debug("virtqueue interrupt with no work for %p\n", vq);
> return IRQ_NONE;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 5464f398912a..dfa2638a293e 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -95,6 +95,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
> * @failed: saved value for VIRTIO_CONFIG_S_FAILED bit (for restore)
> * @config_enabled: configuration change reporting enabled
> * @config_change_pending: configuration change reported while disabled
> + * @driver_ready: whehter the driver is ready (e.g for vring callbacks)
> * @config_lock: protects configuration change reporting
> * @dev: underlying device.
> * @id: the device type identification (used to match it with a driver).
> @@ -109,6 +110,7 @@ struct virtio_device {
> bool failed;
> bool config_enabled;
> bool config_change_pending;
> + bool driver_ready;
> spinlock_t config_lock;
> spinlock_t vqs_list_lock; /* Protects VQs list access */
> struct device dev;
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 08b73d9bbff2..c9e207bf2c9c 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -246,6 +246,14 @@ void virtio_device_ready(struct virtio_device *dev)
> {
> unsigned status = dev->config->get_status(dev);
>
> + virtio_synchronize_vqs(dev);
> + /*
> + * The above virtio_synchronize_vqs() make sure
makes sure
> + * vring_interrupt() will see the driver specific setup if it
> + * see driver_ready as true.
sees
> + */
> + WRITE_ONCE(dev->driver_ready, true);
> +
> BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
> dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
> }
> --
> 2.25.1
Powered by blists - more mailing lists