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

[Bodo Stroesser <bostroesser@gmail.com>]

On 24.10.22 14:21, Jason Gunthorpe wrote:
> On Sun, Oct 23, 2022 at 09:02:40PM -0400, 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.
>>
>> Solutions to this issue were discussed by Jason Gunthorpe
>> <jgg@ziepe.ca> and Bodo Stroesser <bostroesser@gmail.com>. This
>> version is base on a linux-scsi post by Jason titled: "Re:
>> [PATCH v7 1/4] sgl_alloc_order: remove 4 GiB limit" dated 20220201.
> 
> You should link to lore here..
> 
> I think I prefer we just fix this so it doesn't have a problem in the
> first place - nothing needs the weird unsigned long long argument type:
> 
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index c8c3d675845c37..c39e19fa174bca 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -595,19 +595,17 @@ EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);
>    *
>    * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
>    */
> -struct scatterlist *sgl_alloc_order(unsigned long long length,
> -				    unsigned int order, bool chainable,
> -				    gfp_t gfp, unsigned int *nent_p)
> +struct scatterlist *sgl_alloc_order(size_t length, unsigned int order,
> +				    bool chainable, gfp_t gfp, size_t *nent_p)
>   {
>   	struct scatterlist *sgl, *sg;
>   	struct page *page;
> -	unsigned int nent, nalloc;
> +	size_t 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)))
> -		return NULL;
> +	nent = length >> (PAGE_SHIFT + order);
> +	if (length % (1 << (PAGE_SHIFT + order)))

This might end up doing a modulo operation for divisor 0, if caller
specifies a too high order parameter, right?

To be on the safe side I'd prefer to use the following code instead:
     if (length & ((1 << (PAGE_SHIFT + order)) - 1))

Thank you
Bodo

> +		nent++;
>   	nalloc = nent;
>   	if (chainable) {
>   		/* Check for integer overflow */
> @@ -649,8 +647,7 @@ EXPORT_SYMBOL(sgl_alloc_order);
>    *
>    * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
>    */
> -struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
> -			      unsigned int *nent_p)
> +struct scatterlist *sgl_alloc(size_t length, gfp_t gfp, size_t *nent_p)
>   {
>   	return sgl_alloc_order(length, 0, false, gfp, nent_p);
>   }
> 
> Jason