IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] io_uring: fix sq array offset calculation
@ 2020-07-11  9:31 Dmitry Vyukov
  2020-07-11  9:37 ` Hristo Venev
  2020-07-11 15:15 ` Jens Axboe
  0 siblings, 2 replies; 14+ messages in thread
From: Dmitry Vyukov @ 2020-07-11  9:31 UTC (permalink / raw)
  To: axboe; +Cc: necip, Dmitry Vyukov, io-uring, Hristo Venev

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.
---
 fs/io_uring.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ca8abde48b6c7..c4c3731ed41e9 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7063,6 +7063,9 @@ static unsigned long rings_size(unsigned sq_entries, unsigned cq_entries,
 		return SIZE_MAX;
 #endif
 
+	if (sq_offset)
+		*sq_offset = off;
+
 	sq_array_size = array_size(sizeof(u32), sq_entries);
 	if (sq_array_size == SIZE_MAX)
 		return SIZE_MAX;
@@ -7070,9 +7073,6 @@ static unsigned long rings_size(unsigned sq_entries, unsigned cq_entries,
 	if (check_add_overflow(off, sq_array_size, &off))
 		return SIZE_MAX;
 
-	if (sq_offset)
-		*sq_offset = off;
-
 	return off;
 }
 
-- 
2.27.0.383.g050319c2ae-goog


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11  9:31 [PATCH] io_uring: fix sq array offset calculation Dmitry Vyukov
@ 2020-07-11  9:37 ` Hristo Venev
  2020-07-11 15:15 ` Jens Axboe
  1 sibling, 0 replies; 14+ messages in thread
From: Hristo Venev @ 2020-07-11  9:37 UTC (permalink / raw)
  To: Dmitry Vyukov, axboe; +Cc: necip, io-uring


[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On Sat, 2020-07-11 at 11:31 +0200, 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")

Oops.

Acked-by: Hristo Venev <hristo@venev.name>

> 
> ---
> 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.
> ---
>  fs/io_uring.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index ca8abde48b6c7..c4c3731ed41e9 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -7063,6 +7063,9 @@ static unsigned long rings_size(unsigned
> sq_entries, unsigned cq_entries,
>  		return SIZE_MAX;
>  #endif
>  
> +	if (sq_offset)
> +		*sq_offset = off;
> +
>  	sq_array_size = array_size(sizeof(u32), sq_entries);
>  	if (sq_array_size == SIZE_MAX)
>  		return SIZE_MAX;
> @@ -7070,9 +7073,6 @@ static unsigned long rings_size(unsigned
> sq_entries, unsigned cq_entries,
>  	if (check_add_overflow(off, sq_array_size, &off))
>  		return SIZE_MAX;
>  
> -	if (sq_offset)
> -		*sq_offset = off;
> -
>  	return off;
>  }
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 858 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11  9:31 [PATCH] io_uring: fix sq array offset calculation Dmitry Vyukov
  2020-07-11  9:37 ` Hristo Venev
@ 2020-07-11 15:15 ` Jens Axboe
  2020-07-11 15:31   ` Dmitry Vyukov
  1 sibling, 1 reply; 14+ messages in thread
From: Jens Axboe @ 2020-07-11 15:15 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: necip, io-uring, Hristo Venev

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.

In any case, the patch is obviously correct, I'll queue it up.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 15:15 ` Jens Axboe
@ 2020-07-11 15:31   ` Dmitry Vyukov
  2020-07-11 15:36     ` Jens Axboe
  2020-07-11 15:52     ` Hristo Venev
  0 siblings, 2 replies; 14+ messages in thread
From: Dmitry Vyukov @ 2020-07-11 15:31 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Necip Fazil Yildiran, io-uring, Hristo Venev

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 15:31   ` Dmitry Vyukov
@ 2020-07-11 15:36     ` Jens Axboe
  2020-07-11 15:47       ` Jens Axboe
  2020-07-11 15:52     ` Hristo Venev
  1 sibling, 1 reply; 14+ messages in thread
From: Jens Axboe @ 2020-07-11 15:36 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: Necip Fazil Yildiran, io-uring, Hristo Venev

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.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 15:36     ` Jens Axboe
@ 2020-07-11 15:47       ` Jens Axboe
  0 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2020-07-11 15:47 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: Necip Fazil Yildiran, io-uring, Hristo Venev

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 15:31   ` Dmitry Vyukov
  2020-07-11 15:36     ` Jens Axboe
@ 2020-07-11 15:52     ` Hristo Venev
  2020-07-11 15:55       ` Jens Axboe
                         ` (2 more replies)
  1 sibling, 3 replies; 14+ messages in thread
From: Hristo Venev @ 2020-07-11 15:52 UTC (permalink / raw)
  To: Dmitry Vyukov, Jens Axboe; +Cc: Necip Fazil Yildiran, io-uring


[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

On Sat, 2020-07-11 at 17:31 +0200, Dmitry Vyukov wrote:
> 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.

The layout (after the fix) is roughly as follows:

1. struct io_rings - ~192 bytes, maybe 256
2. cqes - (32 << n) bytes
3. sq_array - (4 << n) bytes

The bug was that the sq_array was offset by (4 << n) bytes. I think
issues can only occur when

    PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
    !=
    PAGE_ALIGN(192 + (32 << n) + (4 << n))

It looks like this never happens. We got lucky.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 858 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 15:52     ` Hristo Venev
@ 2020-07-11 15:55       ` Jens Axboe
  2020-07-11 15:56       ` Hristo Venev
  2020-07-11 16:16       ` Dmitry Vyukov
  2 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2020-07-11 15:55 UTC (permalink / raw)
  To: Hristo Venev, Dmitry Vyukov; +Cc: Necip Fazil Yildiran, io-uring

On 7/11/20 9:52 AM, Hristo Venev wrote:
> On Sat, 2020-07-11 at 17:31 +0200, Dmitry Vyukov wrote:
>> 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.
> 
> The layout (after the fix) is roughly as follows:
> 
> 1. struct io_rings - ~192 bytes, maybe 256
> 2. cqes - (32 << n) bytes
> 3. sq_array - (4 << n) bytes
> 
> The bug was that the sq_array was offset by (4 << n) bytes. I think
> issues can only occur when
> 
>     PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
>     !=
>     PAGE_ALIGN(192 + (32 << n) + (4 << n))
> 
> It looks like this never happens. We got lucky.

A bit of luck, but if that wasn't the case, then I'm sure we would
have found it when the original patch was tested. But thanks for
double checking!

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 15:52     ` Hristo Venev
  2020-07-11 15:55       ` Jens Axboe
@ 2020-07-11 15:56       ` Hristo Venev
  2020-07-11 16:16       ` Dmitry Vyukov
  2 siblings, 0 replies; 14+ messages in thread
From: Hristo Venev @ 2020-07-11 15:56 UTC (permalink / raw)
  To: Dmitry Vyukov, Jens Axboe; +Cc: Necip Fazil Yildiran, io-uring



On July 11, 2020 6:52:19 PM GMT+03:00, Hristo Venev <hristo@venev.name> wrote:
>On Sat, 2020-07-11 at 17:31 +0200, Dmitry 
>    PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
>    !=
>    PAGE_ALIGN(192 + (32 << n) + (4 << n))

* also power-of-2 aligned.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 15:52     ` Hristo Venev
  2020-07-11 15:55       ` Jens Axboe
  2020-07-11 15:56       ` Hristo Venev
@ 2020-07-11 16:16       ` Dmitry Vyukov
  2020-07-17 13:48         ` Dmitry Vyukov
  2 siblings, 1 reply; 14+ messages in thread
From: Dmitry Vyukov @ 2020-07-11 16:16 UTC (permalink / raw)
  To: Hristo Venev; +Cc: Jens Axboe, Necip Fazil Yildiran, io-uring

On Sat, Jul 11, 2020 at 5:52 PM Hristo Venev <hristo@venev.name> wrote:
>
> On Sat, 2020-07-11 at 17:31 +0200, Dmitry Vyukov wrote:
> > 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.
>
> The layout (after the fix) is roughly as follows:
>
> 1. struct io_rings - ~192 bytes, maybe 256
> 2. cqes - (32 << n) bytes
> 3. sq_array - (4 << n) bytes
>
> The bug was that the sq_array was offset by (4 << n) bytes. I think
> issues can only occur when
>
>     PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
>     !=
>     PAGE_ALIGN(192 + (32 << n) + (4 << n))
>
> It looks like this never happens. We got lucky.

Interesting. CQ entries are larger and we have at least that many of
them as SQ entries. I guess this + power-of-2-pages can make it never
overflow.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-11 16:16       ` Dmitry Vyukov
@ 2020-07-17 13:48         ` Dmitry Vyukov
  2020-07-17 14:05           ` Jens Axboe
  0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Vyukov @ 2020-07-17 13:48 UTC (permalink / raw)
  To: Hristo Venev; +Cc: Jens Axboe, Necip Fazil Yildiran, io-uring

On Sat, Jul 11, 2020 at 6:16 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Sat, Jul 11, 2020 at 5:52 PM Hristo Venev <hristo@venev.name> wrote:
> >
> > On Sat, 2020-07-11 at 17:31 +0200, Dmitry Vyukov wrote:
> > > 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.
> >
> > The layout (after the fix) is roughly as follows:
> >
> > 1. struct io_rings - ~192 bytes, maybe 256
> > 2. cqes - (32 << n) bytes
> > 3. sq_array - (4 << n) bytes
> >
> > The bug was that the sq_array was offset by (4 << n) bytes. I think
> > issues can only occur when
> >
> >     PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
> >     !=
> >     PAGE_ALIGN(192 + (32 << n) + (4 << n))
> >
> > It looks like this never happens. We got lucky.
>
> Interesting. CQ entries are larger and we have at least that many of
> them as SQ entries. I guess this + power-of-2-pages can make it never
> overflow.

Hi Jens,

I see this patch is in block/for-5.9/io_uring
Is this tree merged into linux-next? I don't see it in linux-next yet.
Or is it possible to get it into 5.8?

The reason I am asking is that we have an intern (Necip in CC) working
on significantly extending io_uring coverage in syzkaller:
https://github.com/google/syzkaller/pull/1926
Unfortunately we had to hardcode offset computation logic b/c the
intended way of using io_uring for normal programs represents an
additional obstacle for the fuzzer and the relations between syscalls
and writes to shared memory are even hard to express for the fuzzer.
We want to hardcode this new updated way of computing offsets, but
this means we probably won't get good coverage until the intern term
ends (+ may be good to discover some io_uring bugs before the
release).

If it won't get into linux-next/mainline until 5.9, it's not a big
deal, but I wanted to ask.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-17 13:48         ` Dmitry Vyukov
@ 2020-07-17 14:05           ` Jens Axboe
  2020-07-17 14:08             ` Jens Axboe
  2020-07-17 14:08             ` Dmitry Vyukov
  0 siblings, 2 replies; 14+ messages in thread
From: Jens Axboe @ 2020-07-17 14:05 UTC (permalink / raw)
  To: Dmitry Vyukov, Hristo Venev; +Cc: Necip Fazil Yildiran, io-uring

On 7/17/20 7:48 AM, Dmitry Vyukov wrote:
> On Sat, Jul 11, 2020 at 6:16 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> On Sat, Jul 11, 2020 at 5:52 PM Hristo Venev <hristo@venev.name> wrote:
>>>
>>> On Sat, 2020-07-11 at 17:31 +0200, Dmitry Vyukov wrote:
>>>> 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.
>>>
>>> The layout (after the fix) is roughly as follows:
>>>
>>> 1. struct io_rings - ~192 bytes, maybe 256
>>> 2. cqes - (32 << n) bytes
>>> 3. sq_array - (4 << n) bytes
>>>
>>> The bug was that the sq_array was offset by (4 << n) bytes. I think
>>> issues can only occur when
>>>
>>>     PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
>>>     !=
>>>     PAGE_ALIGN(192 + (32 << n) + (4 << n))
>>>
>>> It looks like this never happens. We got lucky.
>>
>> Interesting. CQ entries are larger and we have at least that many of
>> them as SQ entries. I guess this + power-of-2-pages can make it never
>> overflow.
> 
> Hi Jens,
> 
> I see this patch is in block/for-5.9/io_uring
> Is this tree merged into linux-next? I don't see it in linux-next yet.
> Or is it possible to get it into 5.8?

Yes, that tree is in linux-next, and I'm surprised you don't see it there
as it's been queued up for almost a week. Are you sure?

I'm not going to apply it to both 5.9 and 5.8 trees. The bug has
been there for a while, but doesn't really impact functionality.
Hence I just queued it up for 5.9. If this had been a 5.8 commit
that introduced it, I would have queued it up for 5.8.

> The reason I am asking is that we have an intern (Necip in CC) working
> on significantly extending io_uring coverage in syzkaller:
> https://github.com/google/syzkaller/pull/1926
> Unfortunately we had to hardcode offset computation logic b/c the
> intended way of using io_uring for normal programs represents an
> additional obstacle for the fuzzer and the relations between syscalls
> and writes to shared memory are even hard to express for the fuzzer.
> We want to hardcode this new updated way of computing offsets, but
> this means we probably won't get good coverage until the intern term
> ends (+ may be good to discover some io_uring bugs before the
> release).

Sounds good

> If it won't get into linux-next/mainline until 5.9, it's not a big
> deal, but I wanted to ask.

That's the plan, it'll go in as part of the 5.9 merge window.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-17 14:05           ` Jens Axboe
@ 2020-07-17 14:08             ` Jens Axboe
  2020-07-17 14:08             ` Dmitry Vyukov
  1 sibling, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2020-07-17 14:08 UTC (permalink / raw)
  To: Dmitry Vyukov, Hristo Venev; +Cc: Necip Fazil Yildiran, io-uring

On 7/17/20 8:05 AM, Jens Axboe wrote:
> On 7/17/20 7:48 AM, Dmitry Vyukov wrote:
>> On Sat, Jul 11, 2020 at 6:16 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>>>
>>> On Sat, Jul 11, 2020 at 5:52 PM Hristo Venev <hristo@venev.name> wrote:
>>>>
>>>> On Sat, 2020-07-11 at 17:31 +0200, Dmitry Vyukov wrote:
>>>>> 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.
>>>>
>>>> The layout (after the fix) is roughly as follows:
>>>>
>>>> 1. struct io_rings - ~192 bytes, maybe 256
>>>> 2. cqes - (32 << n) bytes
>>>> 3. sq_array - (4 << n) bytes
>>>>
>>>> The bug was that the sq_array was offset by (4 << n) bytes. I think
>>>> issues can only occur when
>>>>
>>>>     PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
>>>>     !=
>>>>     PAGE_ALIGN(192 + (32 << n) + (4 << n))
>>>>
>>>> It looks like this never happens. We got lucky.
>>>
>>> Interesting. CQ entries are larger and we have at least that many of
>>> them as SQ entries. I guess this + power-of-2-pages can make it never
>>> overflow.
>>
>> Hi Jens,
>>
>> I see this patch is in block/for-5.9/io_uring
>> Is this tree merged into linux-next? I don't see it in linux-next yet.
>> Or is it possible to get it into 5.8?
> 
> Yes, that tree is in linux-next, and I'm surprised you don't see it there
> as it's been queued up for almost a week. Are you sure?

I see it in there:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/fs/io_uring.c?id=a4968ff8b6314631a73fdc945a66fd8645dfe8cc

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] io_uring: fix sq array offset calculation
  2020-07-17 14:05           ` Jens Axboe
  2020-07-17 14:08             ` Jens Axboe
@ 2020-07-17 14:08             ` Dmitry Vyukov
  1 sibling, 0 replies; 14+ messages in thread
From: Dmitry Vyukov @ 2020-07-17 14:08 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Hristo Venev, Necip Fazil Yildiran, io-uring

On Fri, Jul 17, 2020 at 4:05 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 7/17/20 7:48 AM, Dmitry Vyukov wrote:
> > On Sat, Jul 11, 2020 at 6:16 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> >>
> >> On Sat, Jul 11, 2020 at 5:52 PM Hristo Venev <hristo@venev.name> wrote:
> >>>
> >>> On Sat, 2020-07-11 at 17:31 +0200, Dmitry Vyukov wrote:
> >>>> 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.
> >>>
> >>> The layout (after the fix) is roughly as follows:
> >>>
> >>> 1. struct io_rings - ~192 bytes, maybe 256
> >>> 2. cqes - (32 << n) bytes
> >>> 3. sq_array - (4 << n) bytes
> >>>
> >>> The bug was that the sq_array was offset by (4 << n) bytes. I think
> >>> issues can only occur when
> >>>
> >>>     PAGE_ALIGN(192 + (32 << n) + (4 << n) + (4 << n))
> >>>     !=
> >>>     PAGE_ALIGN(192 + (32 << n) + (4 << n))
> >>>
> >>> It looks like this never happens. We got lucky.
> >>
> >> Interesting. CQ entries are larger and we have at least that many of
> >> them as SQ entries. I guess this + power-of-2-pages can make it never
> >> overflow.
> >
> > Hi Jens,
> >
> > I see this patch is in block/for-5.9/io_uring
> > Is this tree merged into linux-next? I don't see it in linux-next yet.
> > Or is it possible to get it into 5.8?
>
> Yes, that tree is in linux-next, and I'm surprised you don't see it there
> as it's been queued up for almost a week. Are you sure?
>
> I'm not going to apply it to both 5.9 and 5.8 trees. The bug has
> been there for a while, but doesn't really impact functionality.
> Hence I just queued it up for 5.9. If this had been a 5.8 commit
> that introduced it, I would have queued it up for 5.8.
>
> > The reason I am asking is that we have an intern (Necip in CC) working
> > on significantly extending io_uring coverage in syzkaller:
> > https://github.com/google/syzkaller/pull/1926
> > Unfortunately we had to hardcode offset computation logic b/c the
> > intended way of using io_uring for normal programs represents an
> > additional obstacle for the fuzzer and the relations between syscalls
> > and writes to shared memory are even hard to express for the fuzzer.
> > We want to hardcode this new updated way of computing offsets, but
> > this means we probably won't get good coverage until the intern term
> > ends (+ may be good to discover some io_uring bugs before the
> > release).
>
> Sounds good
>
> > If it won't get into linux-next/mainline until 5.9, it's not a big
> > deal, but I wanted to ask.
>
> That's the plan, it'll go in as part of the 5.9 merge window.

Thanks.
linux-next is good enough, we test it. And the commit is actually
already there, now that I looked closer.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-11  9:31 [PATCH] io_uring: fix sq array offset calculation Dmitry Vyukov
2020-07-11  9:37 ` Hristo Venev
2020-07-11 15:15 ` Jens Axboe
2020-07-11 15:31   ` Dmitry Vyukov
2020-07-11 15:36     ` Jens Axboe
2020-07-11 15:47       ` Jens Axboe
2020-07-11 15:52     ` Hristo Venev
2020-07-11 15:55       ` Jens Axboe
2020-07-11 15:56       ` Hristo Venev
2020-07-11 16:16       ` Dmitry Vyukov
2020-07-17 13:48         ` Dmitry Vyukov
2020-07-17 14:05           ` Jens Axboe
2020-07-17 14:08             ` Jens Axboe
2020-07-17 14:08             ` Dmitry Vyukov

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git