On 24.10.22 14:21, Jason Gunthorpe wrote:
On Sun, Oct 23, 2022 at 09:02:40PM -0400, 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.
You should link to lore here..
I think I prefer we just fix this so it doesn't have a problem in the
first place - nothing needs the weird unsigned long long argument type:
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index c8c3d675845c37..c39e19fa174bca 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -595,19 +595,17 @@ EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);
*
* Returns: A pointer to an initialized scatterlist or %NULL upon failure.
*/
-struct scatterlist *sgl_alloc_order(unsigned long long length,
- unsigned int order, bool chainable,
- gfp_t gfp, unsigned int *nent_p)
+struct scatterlist *sgl_alloc_order(size_t length, unsigned int order,
+ bool chainable, gfp_t gfp, size_t *nent_p)
{
struct scatterlist *sgl, *sg;
struct page *page;
- unsigned int nent, nalloc;
+ size_t nent, nalloc;
u32 elem_len;
- nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
- /* Check for integer overflow */
- if (length > (nent << (PAGE_SHIFT + order)))
- return NULL;
+ nent = length >> (PAGE_SHIFT + order);
+ if (length % (1 << (PAGE_SHIFT + order)))
This might end up doing a modulo operation for divisor 0, if caller
specifies a too high order parameter, right?
To be on the safe side I'd prefer to use the following code instead:
if (length & ((1 << (PAGE_SHIFT + order)) - 1))
Thank you
Bodo
+ nent++;
nalloc = nent;
if (chainable) {
/* Check for integer overflow */
@@ -649,8 +647,7 @@ EXPORT_SYMBOL(sgl_alloc_order);
*
* Returns: A pointer to an initialized scatterlist or %NULL upon failure.
*/
-struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
- unsigned int *nent_p)
+struct scatterlist *sgl_alloc(size_t length, gfp_t gfp, size_t *nent_p)
{
return sgl_alloc_order(length, 0, false, gfp, nent_p);
}
Jason
