lkml.org 
[lkml]   [2018]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc_order
    Date
    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.
    \
     
     \ /
      Last update: 2018-03-07 17:11    [W:4.058 / U:0.196 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site