| 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
| ||
|
Date: Thu, 7 Apr 2022 14:39:44 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...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
在 2022/4/6 下午8:04, Michael S. Tsirkin 写道:
> 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.
I may miss something but which lock did you mean here? (Note the irq
handler doesn't hold the irq descriptor lock).
Thanks
>
>
> 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