Re: [PATCH v7 1/4] sgl_alloc_order: remove 4 GiB limit

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Jan 31, 2022 at 10:49:12PM -0500, Douglas Gilbert wrote:
> This patch fixes a check done by sgl_alloc_order() before it starts
> any allocations. The comment in the original said: "Check for integer
> overflow" but the right hand side of the expression in the condition
> is resolved as u32 so it can not exceed UINT32_MAX (4 GiB) which
> means 'length' can not exceed that value.
> 
> This function may be used to replace vmalloc(unsigned long) for a
> large allocation (e.g. a ramdisk). vmalloc has no limit at 4 GiB so
> it seems unreasonable that sgl_alloc_order() whose length type is
> unsigned long long should be limited to 4 GB.
> 
> In early 2021 there was discussion between Jason Gunthorpe
> <jgg@ziepe.ca> and Bodo Stroesser <bostroesser@gmail.com> about the
> way to check for overflow caused by order (an exponent) being
> too large. Take the solution proposed by Bodo in post dated
> 20210118 to the linux-scsi and linux-block lists.
> 
> An earlier patch fixed a memory leak in sg_alloc_order() due to the
> misuse of sgl_free(). Take the opportunity to put a one line comment
> above sgl_free()'s declaration warning that it is not suitable when
> order > 0 .
> 
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>
> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
> ---
>  include/linux/scatterlist.h |  1 +
>  lib/scatterlist.c           | 24 +++++++++++++-----------
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 7ff9d6386c12..03130be581bb 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -357,6 +357,7 @@ struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
>  			      unsigned int *nent_p);
>  void sgl_free_n_order(struct scatterlist *sgl, int nents, int order);
>  void sgl_free_order(struct scatterlist *sgl, int order);
> +/* Only use sgl_free() when order is 0 */
>  void sgl_free(struct scatterlist *sgl);
>  #endif /* CONFIG_SGL_ALLOC */
>  
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index d5e82e4a57ad..ed6d0465c78e 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -585,13 +585,16 @@ EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);
>  #ifdef CONFIG_SGL_ALLOC
>  
>  /**
> - * sgl_alloc_order - allocate a scatterlist and its pages
> + * sgl_alloc_order - allocate a scatterlist with equally sized elements each
> + *		     of which has 2^@order continuous pages
>   * @length: Length in bytes of the scatterlist. Must be at least one
> - * @order: Second argument for alloc_pages()
> + * @order:  Second argument for alloc_pages(). Each sgl element size will
> + *	    be (PAGE_SIZE*2^@order) bytes. @order must not exceed 16.
>   * @chainable: Whether or not to allocate an extra element in the scatterlist
> - *	for scatterlist chaining purposes
> + *	       for scatterlist chaining purposes
>   * @gfp: Memory allocation flags
> - * @nent_p: [out] Number of entries in the scatterlist that have pages
> + * @nent_p: [out] Number of entries in the scatterlist that have pages.
> + *		  Ignored if @nent_p is NULL.
>   *
>   * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
>   */
> @@ -604,16 +607,15 @@ struct scatterlist *sgl_alloc_order(unsigned long long length,
>  	unsigned int nent, nalloc;
>  	u32 elem_len;
>  
> -	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
> -	/* Check for integer overflow */
> -	if (length > (nent << (PAGE_SHIFT + order)))
> +	if (length >> (PAGE_SHIFT + order) >= UINT_MAX)
>  		return NULL;
> -	nalloc = nent;
> +	nent = DIV_ROUND_UP(length, PAGE_SIZE << order);
> +

This would be clearer to make nent/etc an unsigned long long. Then
check if nalloc is > SIZE_MAX before casting it to size_t for the
allocation. Avoids the wonky if statement.

Kaspm