Re: [PATCH v2 7/7] 9p/trans_virtio: resize sg lists to whatever is possible

[Christian Schoenebeck <linux_oss@crudebyte.com>]

On Montag, 20. September 2021 18:44:20 CEST Christian Schoenebeck wrote:
> Right now vq_sg_resize() used a lazy implementation following
> the all-or-nothing princible. So it either resized exactly to
> the requested new amount of sg lists, or it did not resize at
> all.
> 
> The problem with this is if a user supplies a very large msize
> value, resize would simply fail and the user would stick to
> the default maximum msize supported by the virtio transport.
> 
> To resolve this potential issue, change vq_sg_resize() to resize
> the passed sg list to whatever is possible on the machine.
> 
> Signed-off-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> ---
>  net/9p/trans_virtio.c | 65 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 4cb75f45aa15..b9bab7ed2768 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -205,24 +205,66 @@ static struct virtqueue_sg *vq_sg_alloc(unsigned int
> nsgl) * amount of lists
>   * @_vq_sg: scatter/gather lists to be resized
>   * @nsgl: new amount of scatter/gather lists
> + *
> + * Old scatter/gather lists are retained. Only growing the size is
> supported. + * If the requested amount cannot be satisfied, then lists are
> increased to + * whatever is possible.
>   */
>  static int vq_sg_resize(struct virtqueue_sg **_vq_sg, unsigned int nsgl)
>  {
>  	struct virtqueue_sg *vq_sg;
> +	unsigned int i;
> +	size_t sz;
> +	int ret = 0;
> 
>  	BUG_ON(!_vq_sg || !nsgl);
>  	vq_sg = *_vq_sg;
> +	if (nsgl > VIRTQUEUE_SG_NSGL_MAX)
> +		nsgl = VIRTQUEUE_SG_NSGL_MAX;
>  	if (vq_sg->nsgl == nsgl)
>  		return 0;
> +	if (vq_sg->nsgl > nsgl)
> +		return -ENOTSUPP;
> +
> +	vq_sg = kzalloc(sizeof(struct virtqueue_sg) +
> +			nsgl * sizeof(struct scatterlist *),
> +			GFP_KERNEL);
> 
> -	/* lazy resize implementation for now */
> -	vq_sg = vq_sg_alloc(nsgl);
>  	if (!vq_sg)
>  		return -ENOMEM;
> 
> -	kfree(*_vq_sg);
> +	/* copy over old scatter gather lists */
> +	sz = sizeof(struct virtqueue_sg) +
> +		(*_vq_sg)->nsgl * sizeof(struct scatterlist *);
> +	memcpy(vq_sg, *_vq_sg, sz);

Missing

	kfree(*_vq_sg);

here.

> +
> +	vq_sg->nsgl = nsgl;
> +
> +	for (i = (*_vq_sg)->nsgl; i < nsgl; ++i) {
> +		vq_sg->sgl[i] = kmalloc_array(
> +			SG_MAX_SINGLE_ALLOC, sizeof(struct scatterlist),
> +			GFP_KERNEL
> +		);
> +		/*
> +		 * handle failed allocation as soft error, we take whatever
> +		 * we get
> +		 */
> +		if (!vq_sg->sgl[i]) {
> +			ret = -ENOMEM;
> +			vq_sg->nsgl = nsgl = i;
> +			break;
> +		}
> +		sg_init_table(vq_sg->sgl[i], SG_MAX_SINGLE_ALLOC);
> +		if (i) {
> +			/* chain the lists */
> +			sg_chain(vq_sg->sgl[i - 1], SG_MAX_SINGLE_ALLOC,
> +				 vq_sg->sgl[i]);
> +		}
> +	}
> +	sg_mark_end(&vq_sg->sgl[nsgl - 1][SG_MAX_SINGLE_ALLOC - 1]);
> +
>  	*_vq_sg = vq_sg;
> -	return 0;
> +	return ret;
>  }
> 
>  /**
> @@ -839,13 +881,16 @@ p9_virtio_create(struct p9_client *client, const char
> *devname, char *args) if (nsgl > chan->vq_sg->nsgl) {
>  			/*
>  			 * if resize fails, no big deal, then just
> -			 * continue with default msize instead
> +			 * continue with whatever we got
>  			 */
> -			if (!vq_sg_resize(&chan->vq_sg, nsgl)) {
> -				client->trans_maxsize =
> -					PAGE_SIZE *
> -					((nsgl * SG_USER_PAGES_PER_LIST) - 3);
> -			}
> +			vq_sg_resize(&chan->vq_sg, nsgl);
> +			/*
> +			 * actual allocation size might be less than
> +			 * requested, so use vq_sg->nsgl instead of nsgl
> +			 */
> +			client->trans_maxsize =
> +				PAGE_SIZE * ((chan->vq_sg->nsgl *
> +				SG_USER_PAGES_PER_LIST) - 3);

As with patch 6, here should probably be an additional

		if (chan->vq_sg->nsgl < nsgl) {
			pr_inf(...);
		}

to explain the user that not all required sg lists could be allocated suiting 
user's requested msize option.

>  		}
>  #endif /* CONFIG_ARCH_NO_SG_CHAIN */
>  	}