Re: [PATCH v3 2/2] virtio_ring: check desc == NULL when packed and indirect

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 19, 2021 at 06:56:57PM +0800, Xuan Zhuo wrote:
> In the case of packed, use indirect desc, since desc is allocated by
> kmalloc_array(), we should check whether its return value is NULL.
> 
> This patch alloc desc inside virtqueue_add_packe(), if desc == NULL,

Can we manage without typos in commit log please?

> fall back to not using indirect.


It should say why is the patch created, and how it's fixed,
as opposed to repating what patch does.

E.g.

when using indirect with packed, we don't check for
allocation failures. Check and fall back on direct.



> Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 91a46c4da87d..62323c27bfe4 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1051,20 +1051,19 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
>  
>  static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  					 struct scatterlist *sgs[],
> +					 struct vring_packed_desc *desc,
>  					 unsigned int total_sg,
>  					 unsigned int out_sgs,
>  					 unsigned int in_sgs,
>  					 void *data,
>  					 gfp_t gfp)

So this gets desc and will free it. I don't much like this.


>  {
> -	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
>  	unsigned int i, n, err_idx;
>  	u16 head, id;
>  	dma_addr_t addr;
>  
>  	head = vq->packed.next_avail_idx;
> -	desc = alloc_indirect_packed(total_sg, gfp);
>  
>  	if (unlikely(vq->vq.num_free < 1)) {
>  		pr_debug("Can't add buf len 1 - avail = 0\n");
> @@ -1191,9 +1190,14 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  
>  	BUG_ON(total_sg == 0);
>  
> -	if (virtqueue_use_indirect(_vq, total_sg))
> -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> -				out_sgs, in_sgs, data, gfp);
> +	if (virtqueue_use_indirect(_vq, total_sg)) {
> +		desc = alloc_indirect_packed(total_sg, gfp);
> +		if (desc)
> +			return virtqueue_add_indirect_packed(vq, sgs, desc,
> +							     total_sg,
> +							     out_sgs, in_sgs,
> +							     data, gfp);
> +	}
>  
>  	head = vq->packed.next_avail_idx;
>  	avail_used_flags = vq->packed.avail_used_flags;
> -- 
> 2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization