| 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, 30 Jun 2022 13:08:53 +0300
From: Alexander Atanasov <alexander.atanasov@...tuozzo.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>, kernel@...nvz.org,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
"Paul E. McKenney" <paulmck@...nel.org>,
Marc Zyngier <maz@...nel.org>,
Halil Pasic <pasic@...ux.ibm.com>,
Cornelia Huck <cohuck@...hat.com>,
Vineeth Vijayan <vneethv@...ux.ibm.com>,
Peter Oberparleiter <oberpar@...ux.ibm.com>,
linux-s390@...r.kernel.org, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] virtio: Restore semantics of vq->broken in
virtqueues
Hello,
On 30/06/2022 12:46, Michael S. Tsirkin wrote:
> On Thu, Jun 30, 2022 at 09:36:46AM +0000, Alexander Atanasov wrote:
>> virtio: harden vring IRQ (8b4ec69d7e09) changed the use
>> of vq->broken. As result vring_interrupt handles IRQs for
>> broken drivers as IRQ_NONE and not IRQ_HANDLED and made impossible
>> to initiallize vqs before the driver is ready, i.e. in probe method.
>> Balloon driver does this and it can not load because it fails in
>> vqs_init with -EIO.
>>
>> So instead of changing the original intent ot the flag introduce
>> a new flag vq->ready which servers the purpose to check of early IRQs
>> and restore the behaviour of the vq->broken flag.
>>
>> Signed-off-by: Alexander Atanasov <alexander.atanasov@...tuozzo.com>
>
> Does
>
> commit c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
> Author: Jason Wang <jasowang@...hat.com>
> Date: Wed Jun 22 09:29:40 2022 +0800
>
> virtio: disable notification hardening by default
>
>
> solve the problem for you?
No, it won't if CONFIG_VIRTIO_HARDEN_NOTIFICATION is enabled - balloon
still won't be able to init vqs.
The problem is in virtqueue_add_split and virtqueue_add_packed - can not
set driver_ok without queues.
The return value of the vring_interrupt gets different - and iirc
IRQ_NONE for broken device can lead to interrupt storms - i am not sure
if that is valid for virtio devices yet but for real harware most
likely. Either way if you have a mix of drivers working differently
depending on return of the handler it would get really messy.
RR's original intent was to flag a driver as bad why reuse it like that ?
>> drivers/virtio/virtio_ring.c | 20 ++++++++++++++------
>> include/linux/virtio.h | 2 +-
>> include/linux/virtio_config.h | 10 +++++-----
>> 3 files changed, 20 insertions(+), 12 deletions(-)
>>
>> 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>
>> Cc: Halil Pasic <pasic@...ux.ibm.com>
>> Cc: Cornelia Huck <cohuck@...hat.com>
>> Cc: Vineeth Vijayan <vneethv@...ux.ibm.com>
>> Cc: Peter Oberparleiter <oberpar@...ux.ibm.com>
>> Cc: linux-s390@...r.kernel.org
>> Cc: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
>>
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 13a7348cedff..dca3cc774584 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -100,6 +100,9 @@ struct vring_virtqueue {
>> /* Other side has made a mess, don't try any more. */
>> bool broken;
>>
>> + /* the queue is ready to handle interrupts */
>> + bool ready;
>> +
>> /* Host supports indirect buffers */
>> bool indirect;
>>
>> @@ -1688,7 +1691,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
>> vq->we_own_ring = true;
>> vq->notify = notify;
>> vq->weak_barriers = weak_barriers;
>> - vq->broken = true;
>> + vq->broken = false;
>> + vq->ready = false;
>> vq->last_used_idx = 0;
>> vq->event_triggered = false;
>> vq->num_added = 0;
>> @@ -2134,7 +2138,10 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
>> return IRQ_NONE;
>> }
>>
>> - if (unlikely(vq->broken)) {
>> + if (unlikely(vq->broken))
>> + return IRQ_HANDLED;
>> +
>> + if (unlikely(!vq->ready)) {
>> dev_warn_once(&vq->vq.vdev->dev,
>> "virtio vring IRQ raised before DRIVER_OK");
>> return IRQ_NONE;
>> @@ -2180,7 +2187,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>> vq->we_own_ring = false;
>> vq->notify = notify;
>> vq->weak_barriers = weak_barriers;
>> - vq->broken = true;
>> + vq->broken = false;
>> + vq->ready = false;
>> vq->last_used_idx = 0;
>> vq->event_triggered = false;
>> vq->num_added = 0;
>> @@ -2405,7 +2413,7 @@ EXPORT_SYMBOL_GPL(virtio_break_device);
>> * (probing and restoring). This function should only be called by the
>> * core, not directly by the driver.
>> */
>> -void __virtio_unbreak_device(struct virtio_device *dev)
>> +void __virtio_device_ready(struct virtio_device *dev)
>> {
>> struct virtqueue *_vq;
>>
>> @@ -2414,11 +2422,11 @@ void __virtio_unbreak_device(struct virtio_device *dev)
>> struct vring_virtqueue *vq = to_vvq(_vq);
>>
>> /* Pairs with READ_ONCE() in virtqueue_is_broken(). */
>> - WRITE_ONCE(vq->broken, false);
>> + WRITE_ONCE(vq->ready, true);
>> }
>> spin_unlock(&dev->vqs_list_lock);
>> }
>> -EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
>> +EXPORT_SYMBOL_GPL(__virtio_device_ready);
>>
>> dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
>> {
>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
>> index d8fdf170637c..538c5959949a 100644
>> --- a/include/linux/virtio.h
>> +++ b/include/linux/virtio.h
>> @@ -131,7 +131,7 @@ void unregister_virtio_device(struct virtio_device *dev);
>> bool is_virtio_device(struct device *dev);
>>
>> void virtio_break_device(struct virtio_device *dev);
>> -void __virtio_unbreak_device(struct virtio_device *dev);
>> +void __virtio_device_ready(struct virtio_device *dev);
>>
>> void virtio_config_changed(struct virtio_device *dev);
>> #ifdef CONFIG_PM_SLEEP
>> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
>> index 49c7c32815f1..35cf1b26e05a 100644
>> --- a/include/linux/virtio_config.h
>> +++ b/include/linux/virtio_config.h
>> @@ -259,21 +259,21 @@ void virtio_device_ready(struct virtio_device *dev)
>>
>> /*
>> * The virtio_synchronize_cbs() makes sure vring_interrupt()
>> - * will see the driver specific setup if it sees vq->broken
>> + * will see the driver specific setup if it sees vq->ready
>> * as false (even if the notifications come before DRIVER_OK).
>> */
>> virtio_synchronize_cbs(dev);
>> - __virtio_unbreak_device(dev);
>> + __virtio_device_ready(dev);
>> /*
>> - * The transport should ensure the visibility of vq->broken
>> + * The transport should ensure the visibility of vq->ready
>> * before setting DRIVER_OK. See the comments for the transport
>> * specific set_status() method.
>> *
>> * A well behaved device will only notify a virtqueue after
>> * DRIVER_OK, this means the device should "see" the coherenct
>> - * memory write that set vq->broken as false which is done by
>> + * memory write that set vq->ready as true which is done by
>> * the driver when it sees DRIVER_OK, then the following
>> - * driver's vring_interrupt() will see vq->broken as false so
>> + * driver's vring_interrupt() will see vq->true as true so
>> * we won't lose any notification.
>> */
>> dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
>> --
>> 2.25.1
--
Regards,
Alexander Atanasov
Powered by blists - more mailing lists