Re: lib/scatterlist.c : sgl_alloc_order promises more than it delivers

[Douglas Gilbert <dgilbert@interlog.com>]

On 2020-09-24 10:34 p.m., Bart Van Assche wrote:
> On 2020-09-24 18:46, Douglas Gilbert wrote:
>>          /* Check for integer overflow */
>>          if (length > (nent << (PAGE_SHIFT + order)))
>>                  return NULL;
>>
>> Well _integers_ don't wrap, but that pedantic point aside, 'nent' is an
>> unsigned int which means the rhs expression cannot represent 2^32 or
>> higher. So if length >= 2^32 the function fails (i.e. returns NULL).
>>
>> On 8 GiB and 16 GiB machines I can easily build 6 or 12 GiB sgl_s (with
>> scsi_debug) but only if no single allocation is >= 4 GiB due to the
>> above check.
>>
>> So is the above check intended to do that or is it a bug?
> 
> The above check verifies that nent << (PAGE_SHIFT + order) ==
> (uint64_t)nent << (PAGE_SHIFT + order). So I think it does what the
> comment says it does.

I modified sgl_alloc_order() like this:

         /* Check for integer overflow */
         if (length > (nent << (PAGE_SHIFT + order)))
{
pr_info("%s: (length > (nent << (PAGE_SHIFT + order))\n", __func__);
                 return NULL;
}
	...

Then I tried starting scsi_debug with dev_size_mb=4096

This is what I saw in the log:

scsi_debug:scsi_debug_init: fixing max submit queue depth to host max queue 
depth, 32
sgl_alloc_order: (length > (nent << (PAGE_SHIFT + order))
message repeated 2 times: [sgl_alloc_order: (length > (nent << (PAGE_SHIFT + 
order))]
scsi_debug:sdeb_store_sgat: sdeb_store_sgat: unable to obtain 4096 MiB, last 
element size: 256 kiB
scsi_debug:sdebug_add_store: sgat: user data oom
scsi_debug:sdebug_add_store: sdebug_add_store: failed, errno=12


My code steps down from 1024 KiB elements on failure to 512 KiB and if that
fails it tries 256 KiB. Then it gives up. The log output is consistent with
my analysis. So your stated equality is an inequality when length >= 4 GiB.
There is no promotion of unsigned int nent to uint64_t .

You can write your own test harness if you don't believe me. The test machine
doesn't need much ram. Without the call to sgl_free() corrected, if it really
did try to get that much ram and failed toward the end, then (partially)
freed up what it had obtained, then you would see a huge memory leak ...


Now your intention seems to be that a 4 GiB sgl should be valid. Correct?
Can that check just be dropped?

Doug Gilbert