Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> sgl_alloc_order explicitly takes a 64-bit length (unsigned long long) but
> then rejects it in overflow checking if greater than 4GiB allocation was
> requested. This is a consequence of using unsigned int for the right hand
> side condition which then natuarally overflows when shifted left, earlier
> than nent otherwise would.
> 
> Fix is to promote the right hand side of the conditional to unsigned long.

Agreed.

> It is also not useful to allow for 64-bit lenght on 32-bit platforms so
> I have changed this type to a natural unsigned long. Like this it changes
> size naturally depending on the architecture.

I do not agree. Although uncommon, it is possible that e.g. a SCSI initiator
sends a transfer of more than 4 GB to a target system and that that transfer
must not be split. Since this code is used by the SCSI target, I think that's
an example of an application where it is useful to allow allocations of more
than 4 GB at once on a 32-bit system.

> 2.
> 
> elem_len should not be explicitly sized u32 but unsigned int, to match
> the underlying struct scatterlist nents type. Same for the nent_p output
> parameter type.

Are you sure it is useful to support allocations with an order that exceeds
(31 - PAGE_SHIFT)? Since memory gets fragmented easily in the Linux kernel I
think that it's unlikely that such allocations will succeed.

> I renamed this to chunk_len and consolidated its use throughout the
> function.

Please undo this change such that the diff remains as short as possible.

> -void sgl_free_n_order(struct scatterlist *sgl, int nents, int order)
> +void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
> +		      unsigned int order)
>  {
>  	struct scatterlist *sg;
>  	struct page *page;
> -	int i;
> +	unsigned int i;
>  
>  	for_each_sg(sgl, sg, nents, i) {
>  		if (!sg)
> @@ -583,9 +587,9 @@ EXPORT_SYMBOL(sgl_free_n_order);
>   * @sgl: Scatterlist with one or more elements
>   * @order: Second argument for __free_pages()
>   */
> -void sgl_free_order(struct scatterlist *sgl, int order)
> +void sgl_free_order(struct scatterlist *sgl, unsigned int order)
>  {
> -	sgl_free_n_order(sgl, INT_MAX, order);
> +	sgl_free_n_order(sgl, UINT_MAX, order);
>  }
>  EXPORT_SYMBOL(sgl_free_order);

Do you have an application that calls these functions to allocate more than
INT_MAX * PAGE_SIZE bytes at once? If not, please leave these changes out.

Thanks,

Bart.