On 2022-10-24 13:32, Jason Gunthorpe wrote:
On Mon, Oct 24, 2022 at 04:32:30PM +0200, Bodo Stroesser wrote:
+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?
If that happens then the first >> will be busted too and this is all
broken..
We assume the caller will pass a valid order paramter it seems, it is
not userspace controlled.
Hi,
I have been trying to remove the unstated 4 GB limit on this existing
function, _not_ change its interface. In practice, I don't believe limiting
'nent' to 2^32-1 (i.e. 'unsigned int') is an unreasonable restriction and
that is checked at runtime in my patch. Since each 'entity' is at least
PAGE_SIZE bytes, that is at least 16 TB (with order==0).
The order_too_large issue I have tried to address with a pre-condition
(i.e. words in the description of the 'order' argument). That said, I did
notice:
include/linux/mmzone.h :
#ifndef CONFIG_ARCH_FORCE_MAX_ORDER
#define MAX_ORDER 11
#else
#define MAX_ORDER CONFIG_ARCH_FORCE_MAX_ORDER
#endif
Changing the interface of sgl_alloc_order() will break these callers:
drivers/scsi/ipr.c
drivers/target/target_core_transport.c
due to change the type (and size on 64 bit machines) of the fifth argument
(i.e. 'size_t *nent_p').
If you want to change sgl_alloc_order()'s interface, shouldn't that be done
in a separate patch that also fixes the breakages it would otherwise cause?
Doug Gilbert
