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@xxxxxxxx> and Bodo Stroesser <bostroesser@xxxxxxxxx>. 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@xxxxxxxx>
Cc: Bodo Stroesser <bostroesser@xxxxxxxxx>
Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
---
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@xxxxxxxx/
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
