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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Nov 15, 2022 at 07:20:13PM -0500, Douglas Gilbert wrote:
> 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)

32 bit machines can't kmap more than size_t - so this is not
correct. We can't put sgl tables into highmem.

>   2) making the last (fifth) argument of type size_t is overkill on a
>      64 bit machine. IMO 32 bits is sufficient. 

IIRC, I changed it to obviously avoid integer promotion/truncation
issues. It is better to handle those with correct typing than
introducing a bunch of frail checks. We don't need to worry about the
extra 32 bits in something like this.

>   3) it changes the signature of an existing exported kernel function
>      requiring changes in several call sites. 

So fix them. It is why we have one git tree. You'll get sympathy if it
is more than 5-10 :)

>      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.

Uh, if someone is "peeved" they are not understanding how kernel APIs
are expected to evolve, I think.

It should be two patches, one to correct the types in the function
signature, and another to resolve the 4G problem.

>     [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.]

Which is consistent with how the community views in-kernel APIs.

Jason