The virtqueue_resize() function was not correctly propagating error codes
from its internal resize helper functions, specifically
virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
returned an error, but the subsequent call to virtqueue_enable_after_reset()
succeeded, the original error from the resize operation would be masked.
Consequently, virtqueue_resize() could incorrectly report success to its
caller despite an underlying resize failure.

This change restores the original code behavior:

       if (vdev->config->enable_vq_after_reset(_vq))
               return -EBUSY;

       return err;

Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
Cc: xuanzhuo@linux.alibaba.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/virtio/virtio_ring.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b784aab66867..4397392bfef0 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 		     void (*recycle_done)(struct virtqueue *vq))
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
-	int err;
+	int err, err_reset;
 
 	if (num > vq->vq.num_max)
 		return -E2BIG;
@@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 	else
 		err = virtqueue_resize_split(_vq, num);
 
-	return virtqueue_enable_after_reset(_vq);
+	err_reset = virtqueue_enable_after_reset(_vq);
+	if (err_reset)
+		return err_reset;
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(virtqueue_resize);
 
-- 
2.49.0

We should provide feedback to the caller indicating the queue's current status
whether it is still valid and whether its size has been successfully
modified. Here I selected the first. The caller can know the second by
virtqueue_get_vring_size().

Thanks.


On Tue, 20 May 2025 13:05:25 +0200, Laurent Vivier <lvivier@redhat.com> wrote:
> The virtqueue_resize() function was not correctly propagating error codes
> from its internal resize helper functions, specifically
> virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
> returned an error, but the subsequent call to virtqueue_enable_after_reset()
> succeeded, the original error from the resize operation would be masked.
> Consequently, virtqueue_resize() could incorrectly report success to its
> caller despite an underlying resize failure.
>
> This change restores the original code behavior:
>
>        if (vdev->config->enable_vq_after_reset(_vq))
>                return -EBUSY;
>
>        return err;
>
> Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
> Cc: xuanzhuo@linux.alibaba.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b784aab66867..4397392bfef0 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>  		     void (*recycle_done)(struct virtqueue *vq))
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> -	int err;
> +	int err, err_reset;
>
>  	if (num > vq->vq.num_max)
>  		return -E2BIG;
> @@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>  	else
>  		err = virtqueue_resize_split(_vq, num);
>
> -	return virtqueue_enable_after_reset(_vq);
> +	err_reset = virtqueue_enable_after_reset(_vq);
> +	if (err_reset)
> +		return err_reset;
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_resize);
>
> --
> 2.49.0
>

On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> The virtqueue_resize() function was not correctly propagating error codes
> from its internal resize helper functions, specifically
> virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
> returned an error, but the subsequent call to virtqueue_enable_after_reset()
> succeeded, the original error from the resize operation would be masked.
> Consequently, virtqueue_resize() could incorrectly report success to its
> caller despite an underlying resize failure.
>
> This change restores the original code behavior:
>
>        if (vdev->config->enable_vq_after_reset(_vq))
>                return -EBUSY;
>
>        return err;
>
> Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
> Cc: xuanzhuo@linux.alibaba.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b784aab66867..4397392bfef0 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>                      void (*recycle_done)(struct virtqueue *vq))
>  {
>         struct vring_virtqueue *vq = to_vvq(_vq);
> -       int err;
> +       int err, err_reset;
>
>         if (num > vq->vq.num_max)
>                 return -E2BIG;
> @@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>         else
>                 err = virtqueue_resize_split(_vq, num);
>
> -       return virtqueue_enable_after_reset(_vq);
> +       err_reset = virtqueue_enable_after_reset(_vq);

I wonder if we should call virtqueue_enable_after_reset() when
virtqueue_resize_xxx() fail.

Thanks

> +       if (err_reset)
> +               return err_reset;
> +
> +       return err;
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_resize);
>
> --
> 2.49.0
>

On 21/05/2025 03:00, Jason Wang wrote:
> On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> The virtqueue_resize() function was not correctly propagating error codes
>> from its internal resize helper functions, specifically
>> virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
>> returned an error, but the subsequent call to virtqueue_enable_after_reset()
>> succeeded, the original error from the resize operation would be masked.
>> Consequently, virtqueue_resize() could incorrectly report success to its
>> caller despite an underlying resize failure.
>>
>> This change restores the original code behavior:
>>
>>         if (vdev->config->enable_vq_after_reset(_vq))
>>                 return -EBUSY;
>>
>>         return err;
>>
>> Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
>> Cc: xuanzhuo@linux.alibaba.com
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   drivers/virtio/virtio_ring.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index b784aab66867..4397392bfef0 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>                       void (*recycle_done)(struct virtqueue *vq))
>>   {
>>          struct vring_virtqueue *vq = to_vvq(_vq);
>> -       int err;
>> +       int err, err_reset;
>>
>>          if (num > vq->vq.num_max)
>>                  return -E2BIG;
>> @@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>          else
>>                  err = virtqueue_resize_split(_vq, num);
>>
>> -       return virtqueue_enable_after_reset(_vq);
>> +       err_reset = virtqueue_enable_after_reset(_vq);
> 
> I wonder if we should call virtqueue_enable_after_reset() when
> virtqueue_resize_xxx() fail.

Original code modified by ad48d53b5b3f did the reset. And the commit removes it without 
explanation.

And as we did a virtqueue_disable_and_recycle(), I think we need the 
virtqueue_enable_after_reset() to restart the queue.

In virtnet_tx_resize(), we have virtnet_tx_resume() unconditionnaly, even in case of error 
of virtqueue_resize(). virtnet_tx_resize() is called by virtnet_set_ringparam(), that is 
the function called by 'ethtool -G' and I think a failure of ethtool should not break the 
virtqueue.

Thanks,
Laurent


> 
> Thanks
> 
>> +       if (err_reset)
>> +               return err_reset;
>> +
>> +       return err;
>>   }
>>   EXPORT_SYMBOL_GPL(virtqueue_resize);
>>
>> --
>> 2.49.0
>>
>