Messages in this thread | | | Subject | Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order | From | Tvrtko Ursulin <> | Date | Fri, 9 Mar 2018 14:04:55 +0000 |
| |
On 07/03/18 17:06, Tvrtko Ursulin wrote: > On 07/03/18 16:10, Bart Van Assche wrote: >> 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. > > If it can work on 32-bit (it can DMA from highmem or what?) and > allocation can realistically succeed then I'm happy to defer to storage > experts on this one.
Furthermore on this specific point, the only caller of sgl_alloc_order in the tree passes in u32 for length.
So even if there will be some realistic use case for >4GiB allocations on 32-bit systems (where unsigned long is 32-bit, to be more precise) I don't see a problem with my patch right now.
First five patches from the series are all IMO fixes and cleanup of unused parts of the API.
Regards,
Tvrtko
>> >>> 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. > > Not sure what you are getting at here. > > There are not explicit width types anywhere in the SGL API apart this > u32 elem_lem. > > So I changed it to unsigned int not to confuse. It gets passed in to > sg_set_page which takes unsigned int. So no reason for it to be u32. > >> >>> 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. > > Name change only? Yeah can do that. Even though chunk as a term is > somewhat established elsewhere in lib/scatterlist.c. > >>> -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. > > There is no reason to used signed int here and it is even inconsistent > with itself because sgl_alloc_order returns you nents in an unsigned > int. And sg_init_table takes unsigned int for nents. So really I see no > reason to have signed types for nents on sgl_free side of the API. > > Regards, > > Tvrtko
| |