Re: [PATCH v2 1/5] sgl_alloc_order: remove 4 GiB limit

[Douglas Gilbert <dgilbert@interlog.com>]

On 2022-11-15 15:33, Jason Gunthorpe wrote:
> On Sat, Nov 12, 2022 at 02:49:35PM -0500, Douglas Gilbert wrote:
>> This patch fixes a check done by sgl_alloc_order() before it starts
>> any allocations. The comment in the original said: "Check for integer
>> overflow" but the right hand side of the expression in the condition
>> is resolved as u32 so it can not exceed UINT32_MAX (4 GiB) which
>> means 'length' can not exceed that value.
>>
>> This function may be used to replace vmalloc(unsigned long) for a
>> large allocation (e.g. a ramdisk). vmalloc has no limit at 4 GiB so
>> it seems unreasonable that sgl_alloc_order() whose length type is
>> unsigned long long should be limited to 4 GB.
>>
>> Solutions to this issue were discussed by Jason Gunthorpe
>> <jgg@ziepe.ca> and Bodo Stroesser <bostroesser@gmail.com>. This
>> version is base on a linux-scsi post by Jason titled: "Re:
>> [PATCH v7 1/4] sgl_alloc_order: remove 4 GiB limit" dated 20220201.
>>
>> An earlier patch fixed a memory leak in sg_alloc_order() due to the
>> misuse of sgl_free(). Take the opportunity to put a one line comment
>> above sgl_free()'s declaration warning that it is not suitable when
>> order > 0 .
>>
>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>> Cc: Bodo Stroesser <bostroesser@gmail.com>
>> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
>> ---
>>   include/linux/scatterlist.h |  1 +
>>   lib/scatterlist.c           | 23 ++++++++++++++---------
>>   2 files changed, 15 insertions(+), 9 deletions(-)
> 
> I still prefer the version I posted here:
> 
> https://lore.kernel.org/linux-scsi/Y1aDQznakNaWD8kd@ziepe.ca/

Three reasons that I don't:
   1) making the first argument of type size_t may constrict the size
      that can be allocated on a 32 bit machine (faint recollection of
      extended/expanded memory on 8086). uint64_t would be better
      than unsigned long long but see point 3)
   2) making the last (fifth) argument of type size_t is overkill on a
      64 bit machine. IMO 32 bits is sufficient. The maximum unsigned int
      is 2^32 - 1 and with a typical PAGE_SIZE of 4096 bytes and order 0,
      that is roughly 2^44 bytes or about 16 TB. If part of the kernel
      did want 16 TB in a single allocation, I hope it would choose a
      larger value for order. So then the maximum single allocation
      would be 2^(44+MAX_ORDER-1) bytes. Can I stop now?
   3) it changes the signature of an existing exported kernel function
      requiring changes in several call sites. Changing an output pointer
      type may require more than a one line change at the existing call
      sites. Due to the fact that this patch is removing an existing
      4 GB limit, those call sites have zero need for this. If I was
      maintaining the driver containing those call sites, I would be
      a bit peeved. [That said, maintaining out-of-tree patchsets, while
      trying to get them accepted in the mainline, is a considerable
      pain due to the constant changes in the block layer API.]

Doug Gilbert