Messages in this thread Patch in this message | | | From | Bart Van Assche <> | Subject | Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order | Date | Wed, 7 Mar 2018 16:10:37 +0000 |
| |
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. | |