[PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Alexander Atanasov]

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@virtuozzo.com>
---
 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@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Vineeth Vijayan <vneethv@linux.ibm.com>
Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: Xuan Zhuo <xuanzhuo@linux.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

Re: [PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Michael S. Tsirkin]

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@virtuozzo.com>


Does

commit c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
Author: Jason Wang <jasowang@redhat.com>
Date:   Wed Jun 22 09:29:40 2022 +0800

    virtio: disable notification hardening by default


solve the problem for you?

> ---
>  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@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Vineeth Vijayan <vneethv@linux.ibm.com>
> Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> Cc: Xuan Zhuo <xuanzhuo@linux.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

Re: [PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Alexander Atanasov]

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@virtuozzo.com>
>
> Does
>
> commit c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
> Author: Jason Wang <jasowang@redhat.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@linutronix.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: "Paul E. McKenney" <paulmck@kernel.org>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Halil Pasic <pasic@linux.ibm.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: Vineeth Vijayan <vneethv@linux.ibm.com>
>> Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
>> Cc: linux-s390@vger.kernel.org
>> Cc: Xuan Zhuo <xuanzhuo@linux.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

Re: [PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Michael S. Tsirkin]

On Thu, Jun 30, 2022 at 01:08:53PM +0300, Alexander Atanasov wrote:
> 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@virtuozzo.com>
> > 
> > Does
> > 
> > commit c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
> > Author: Jason Wang <jasowang@redhat.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.

Yea I intend to make CONFIG_VIRTIO_HARDEN_NOTIFICATION 
depend on BROKEN for now.

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

No, I think it's the reverse. With IRQ_HANDLED an interrupt
storm will keep overloading the CPU since driver tells
kernel all is well. With IRQ_NONE kernel will eventually
intervene and disable the irq.

> 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@linutronix.de>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Halil Pasic <pasic@linux.ibm.com>
> > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > Cc: Vineeth Vijayan <vneethv@linux.ibm.com>
> > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> > > Cc: linux-s390@vger.kernel.org
> > > Cc: Xuan Zhuo <xuanzhuo@linux.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

Re: [PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Jason Wang]

On Thu, Jun 30, 2022 at 6:09 PM Alexander Atanasov
<alexander.atanasov@virtuozzo.com> wrote:
>
> 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@virtuozzo.com>
> >
> > Does
> >
> > commit c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
> > Author: Jason Wang <jasowang@redhat.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.

Valid but the interrupt will be noted and disabled by the kernel then.

> Either way if you have a mix of  drivers working differently
> depending on return of the handler  it would get really messy.

Yes, IRQ_HANDLED may break the driver that shares a single IRQ.

>
> RR's original intent was to flag a driver as bad why reuse it like that  ?

It's somehow the same, we want to prevent the driver from using the
malicious or buggy device.

Anyhow, I think using a dedicated variable is better.

We are discussing a better approach for hardening the notifications.
But in case, this will be merged:

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

>
>
> >>   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@linutronix.de>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: Halil Pasic <pasic@linux.ibm.com>
> >> Cc: Cornelia Huck <cohuck@redhat.com>
> >> Cc: Vineeth Vijayan <vneethv@linux.ibm.com>
> >> Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> >> Cc: linux-s390@vger.kernel.org
> >> Cc: Xuan Zhuo <xuanzhuo@linux.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
>

Re: [PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Jason Wang]

On Thu, Jun 30, 2022 at 11:44 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jun 30, 2022 at 01:08:53PM +0300, Alexander Atanasov wrote:
> > 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@virtuozzo.com>
> > >
> > > Does
> > >
> > > commit c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
> > > Author: Jason Wang <jasowang@redhat.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.
>
> Yea I intend to make CONFIG_VIRTIO_HARDEN_NOTIFICATION
> depend on BROKEN for now.
>
> > 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.
>
> No, I think it's the reverse. With IRQ_HANDLED an interrupt
> storm will keep overloading the CPU since driver tells
> kernel all is well. With IRQ_NONE kernel will eventually
> intervene and disable the irq.

Yes, and users may get a warn.

For IRQ_HANDLED, it has an issue when the driver is sharing IRQ with
other drivers.

Thanks

>
> > 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@linutronix.de>
> > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: Halil Pasic <pasic@linux.ibm.com>
> > > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > > Cc: Vineeth Vijayan <vneethv@linux.ibm.com>
> > > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> > > > Cc: linux-s390@vger.kernel.org
> > > > Cc: Xuan Zhuo <xuanzhuo@linux.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
>

Re: [PATCH v1 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Michael S. Tsirkin]

On Fri, Jul 01, 2022 at 09:12:58AM +0800, Jason Wang wrote:
> On Thu, Jun 30, 2022 at 11:44 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 01:08:53PM +0300, Alexander Atanasov wrote:
> > > 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@virtuozzo.com>
> > > >
> > > > Does
> > > >
> > > > commit c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
> > > > Author: Jason Wang <jasowang@redhat.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.
> >
> > Yea I intend to make CONFIG_VIRTIO_HARDEN_NOTIFICATION
> > depend on BROKEN for now.
> >
> > > 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.
> >
> > No, I think it's the reverse. With IRQ_HANDLED an interrupt
> > storm will keep overloading the CPU since driver tells
> > kernel all is well. With IRQ_NONE kernel will eventually
> > intervene and disable the irq.
> 
> Yes, and users may get a warn.
> 
> For IRQ_HANDLED, it has an issue when the driver is sharing IRQ with
> other drivers.
> 
> Thanks


Couldn't tell whether you are agreeing or disagreeing with me here.

> >
> > > 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@linutronix.de>
> > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > Cc: Halil Pasic <pasic@linux.ibm.com>
> > > > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > > > Cc: Vineeth Vijayan <vneethv@linux.ibm.com>
> > > > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> > > > > Cc: linux-s390@vger.kernel.org
> > > > > Cc: Xuan Zhuo <xuanzhuo@linux.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
> >

[PATCH v2 1/1] virtio: Restore semantics of vq->broken in virtqueues

[Alexander Atanasov]

virtio: harden vring IRQ (8b4ec69d7e09) changed the meaning
of vq->broken which results in 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.

Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com>
---
 drivers/virtio/virtio_ring.c  | 33 ++++++++++++++++++---------------
 include/linux/virtio.h        |  4 +++-
 include/linux/virtio_config.h | 12 ++++++------
 3 files changed, 27 insertions(+), 22 deletions(-)

V1->V2: 
  Reworked on top of config option to disable the hardening.
  

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 643ca779fcc6..f0e645e17848 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;
 
@@ -1708,10 +1711,9 @@ 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;
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
+	vq->ready = false;
 #endif
 	vq->last_used_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);
 	vq->event_triggered = false;
@@ -2157,15 +2159,16 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 		return IRQ_NONE;
 	}
 
-	if (unlikely(vq->broken)) {
+	if (unlikely(vq->broken))
+		return IRQ_HANDLED;
+
 #ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
+	if (unlikely(!vq->ready)) {
 		dev_warn_once(&vq->vq.vdev->dev,
 			      "virtio vring IRQ raised before DRIVER_OK");
 		return IRQ_NONE;
-#else
-		return IRQ_HANDLED;
-#endif
 	}
+#endif
 
 	/* Just a hint for performance: so it's ok that this can be racy! */
 	if (vq->event)
@@ -2207,10 +2210,9 @@ 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;
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
+	vq->ready = false;
 #endif
 	vq->last_used_idx = 0;
 	vq->event_triggered = false;
@@ -2429,14 +2431,15 @@ void virtio_break_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(virtio_break_device);
 
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
 /*
  * This should allow the device to be used by the driver. You may
  * need to grab appropriate locks to flush the write to
- * vq->broken. This should only be used in some specific case e.g
+ * vq->ready. This should only be used in some specific case e.g
  * (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;
 
@@ -2444,12 +2447,12 @@ void __virtio_unbreak_device(struct virtio_device *dev)
 	list_for_each_entry(_vq, &dev->vqs, list) {
 		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);
+#endif
 
 dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
 {
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index d8fdf170637c..a63120477ae1 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -131,7 +131,9 @@ 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);
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
+void __virtio_device_ready(struct virtio_device *dev);
+#endif
 
 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 b47c2e7ed0ee..472d4703d499 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -260,22 +260,22 @@ void virtio_device_ready(struct virtio_device *dev)
 #ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
 	/*
 	 * The virtio_synchronize_cbs() makes sure vring_interrupt()
-	 * will see the driver specific setup if it sees vq->broken
-	 * as false (even if the notifications come before DRIVER_OK).
+	 * will see the driver specific setup if it sees vq->ready
+	 * as true (even if the notifications come before DRIVER_OK).
 	 */
 	virtio_synchronize_cbs(dev);
-	__virtio_unbreak_device(dev);
+	__virtio_device_ready(dev);
 #endif
 	/*
-	 * 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->ready as true so
 	 * we won't lose any notification.
 	 */
 	dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
-- 
2.25.1