lkml.org 
[lkml]   [2018]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order
From
Date

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

\
 
 \ /
  Last update: 2018-03-09 15:05    [W:0.081 / U:1.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site