Re: [PATCH] io_uring: fix sq array offset calculation

[Jens Axboe <axboe@kernel.dk>]

On 7/11/20 9:36 AM, Jens Axboe wrote:
> On 7/11/20 9:31 AM, Dmitry Vyukov wrote:
>> On Sat, Jul 11, 2020 at 5:16 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 7/11/20 3:31 AM, Dmitry Vyukov wrote:
>>>> rings_size() sets sq_offset to the total size of the rings
>>>> (the returned value which is used for memory allocation).
>>>> This is wrong: sq array should be located within the rings,
>>>> not after them. Set sq_offset to where it should be.
>>>>
>>>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>>> Cc: io-uring@vger.kernel.org
>>>> Cc: Hristo Venev <hristo@venev.name>
>>>> Fixes: 75b28affdd6a ("io_uring: allocate the two rings together")
>>>>
>>>> ---
>>>> This looks so wrong and yet io_uring works.
>>>> So I am either missing something very obvious here,
>>>> or io_uring worked only due to lucky side-effects
>>>> of rounding size to power-of-2 number of pages
>>>> (which gave it enough slack at the end),
>>>> maybe reading/writing some unrelated memory
>>>> with some sizes.
>>>> If I am wrong, please poke my nose into what I am not seeing.
>>>> Otherwise, we probably need to CC stable as well.
>>>
>>> Well that's a noodle scratcher, it's definitely been working fine,
>>> and I've never seen any out-of-bounds on any of the testing I do.
>>> I regularly run anything with KASAN enabled too.
>>
>> Looking at the code more, I am not sure how it may not corrupt memory.
>> There definitely should be some combinations where accessing
>> sq_entries*sizeof(u32) more memory won't be OK.
>> May be worth adding a test that allocates all possible sizes for sq/cq
>> and fills both rings.
> 
> Yeah, actually doing that right now just to verify it.

Did that, full utilization of the sq ring and the cq ring, and not
seeing anything trigger or wrong. I'd need to look closer, but it
just might be that the power-of-2 sizes end up saving us from doom
and gloom.

-- 
Jens Axboe