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.
