[RFC] Check if file_data is initialized

[RFC] Check if file_data is initialized

[Dmitrii Dolgov]

With combination of --fixedbufs and an old version of fio I've managed
to get a strange situation, when doing io_iopoll_complete NULL pointer
dereference on file_data was caused in io_free_req_many. Interesting
enough, the very same configuration doesn't fail on a newest version of
fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
guess it still makes sense to have this check if it's possible to craft
such request to io_uring.

More details about configuration:

[global]
filename=/dev/vda
rw=randread
bs=256k
direct=1
time_based=1
randrepeat=1
gtod_reduce=1

[fiotest]

fio test.fio \
    --readonly \
    --ioengine=io_uring \
    --iodepth 1024 \
    --fixedbufs \
    --hipri \
    --numjobs=1 \
    --runtime=10

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
 fs/io_uring.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

I'm not entirely sure if my analysis is correct, but since this change
fixes the issue for me, I've decided to post it.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c770c2c0eb52..c5e69dfc0221 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1232,7 +1232,8 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
 do_free:
 	kmem_cache_free_bulk(req_cachep, rb->to_free, rb->reqs);
 	percpu_ref_put_many(&ctx->refs, rb->to_free);
-	percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
+	if (ctx->file_data)
+		percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
 	rb->to_free = rb->need_iter = 0;
 }
 
-- 
2.21.0

Re: [RFC] Check if file_data is initialized

[Pavel Begunkov]

On 1/9/2020 4:17 PM, Dmitrii Dolgov wrote:
> With combination of --fixedbufs and an old version of fio I've managed
> to get a strange situation, when doing io_iopoll_complete NULL pointer
> dereference on file_data was caused in io_free_req_many. Interesting
> enough, the very same configuration doesn't fail on a newest version of
> fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
> guess it still makes sense to have this check if it's possible to craft
> such request to io_uring.

I didn't looked up why it could become NULL in the first place, but the
problem is probably deeper.

1. I don't see why it puts @rb->to_free @file_data->refs, even though
there could be non-fixed reqs. It needs to count REQ_F_FIXED_FILE reqs
and put only as much.

2. Jens, there is another line bothering me, could you take a look?

io_free_req_many()
{
...
	if (req->flags & REQ_F_INFLIGHT) ...;
	else
		rb->reqs[i] = NULL;
...
}

It zeroes rb->reqs[i], calls __io_req_aux_free(), but did not free
memory for the request itself. Is it as intended?

> 
> More details about configuration:
> 
> [global]
> filename=/dev/vda
> rw=randread
> bs=256k
> direct=1
> time_based=1
> randrepeat=1
> gtod_reduce=1
> 
> [fiotest]
> 
> fio test.fio \
>     --readonly \
>     --ioengine=io_uring \
>     --iodepth 1024 \
>     --fixedbufs \
>     --hipri \
>     --numjobs=1 \
>     --runtime=10
> 
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
>  fs/io_uring.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> I'm not entirely sure if my analysis is correct, but since this change
> fixes the issue for me, I've decided to post it.
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index c770c2c0eb52..c5e69dfc0221 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1232,7 +1232,8 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>  do_free:
>  	kmem_cache_free_bulk(req_cachep, rb->to_free, rb->reqs);
>  	percpu_ref_put_many(&ctx->refs, rb->to_free);
> -	percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
> +	if (ctx->file_data)
> +		percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
>  	rb->to_free = rb->need_iter = 0;
>  }
>  
> 

-- 
Pavel Begunkov

Re: [RFC] Check if file_data is initialized

[Jens Axboe]

On 1/9/20 6:17 AM, Dmitrii Dolgov wrote:
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index c770c2c0eb52..c5e69dfc0221 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1232,7 +1232,8 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>  do_free:
>  	kmem_cache_free_bulk(req_cachep, rb->to_free, rb->reqs);
>  	percpu_ref_put_many(&ctx->refs, rb->to_free);
> -	percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
> +	if (ctx->file_data)
> +		percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
>  	rb->to_free = rb->need_iter = 0;
>  }

Can you check with just the two ref_put lines swapped? Ala:

	percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
  	percpu_ref_put_many(&ctx->refs, rb->to_free);

-- 
Jens Axboe

Re: [RFC] Check if file_data is initialized

[Jens Axboe]

On 1/9/20 7:26 AM, Pavel Begunkov wrote:
> On 1/9/2020 4:17 PM, Dmitrii Dolgov wrote:
>> With combination of --fixedbufs and an old version of fio I've managed
>> to get a strange situation, when doing io_iopoll_complete NULL pointer
>> dereference on file_data was caused in io_free_req_many. Interesting
>> enough, the very same configuration doesn't fail on a newest version of
>> fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
>> guess it still makes sense to have this check if it's possible to craft
>> such request to io_uring.
> 
> I didn't looked up why it could become NULL in the first place, but the
> problem is probably deeper.
> 
> 1. I don't see why it puts @rb->to_free @file_data->refs, even though
> there could be non-fixed reqs. It needs to count REQ_F_FIXED_FILE reqs
> and put only as much.

Agree on the fixed file refs, there's a bug there where it assumes they
are all still fixed. See below - Dmitrii, use this patch for testing
instead of the other one!

> 2. Jens, there is another line bothering me, could you take a look?
> 
> io_free_req_many()
> {
> ...
> 	if (req->flags & REQ_F_INFLIGHT) ...;
> 	else
> 		rb->reqs[i] = NULL;
> ...
> }
> 
> It zeroes rb->reqs[i], calls __io_req_aux_free(), but did not free
> memory for the request itself. Is it as intended?

We free them at the end of that function, in bulk. But we can't do that
with the aux data.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 32aee149f652..b5dcf6c800ef 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1218,6 +1218,8 @@ struct req_batch {
 
 static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
 {
+	int fixed_refs = 0;
+
 	if (!rb->to_free)
 		return;
 	if (rb->need_iter) {
@@ -1227,8 +1229,10 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
 		for (i = 0; i < rb->to_free; i++) {
 			struct io_kiocb *req = rb->reqs[i];
 
-			if (req->flags & REQ_F_FIXED_FILE)
+			if (req->flags & REQ_F_FIXED_FILE) {
 				req->file = NULL;
+				fixed_refs++;
+			}
 			if (req->flags & REQ_F_INFLIGHT)
 				inflight++;
 			else
@@ -1255,8 +1259,9 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
 	}
 do_free:
 	kmem_cache_free_bulk(req_cachep, rb->to_free, rb->reqs);
+	if (fixed_refs)
+		percpu_ref_put_many(&ctx->file_data->refs, fixed_refs);
 	percpu_ref_put_many(&ctx->refs, rb->to_free);
-	percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
 	rb->to_free = rb->need_iter = 0;
 }

-- 
Jens Axboe

Re: [RFC] Check if file_data is initialized

[Pavel Begunkov]

On 1/9/2020 5:51 PM, Jens Axboe wrote:
> On 1/9/20 7:26 AM, Pavel Begunkov wrote:
>> On 1/9/2020 4:17 PM, Dmitrii Dolgov wrote:
>>> With combination of --fixedbufs and an old version of fio I've managed
>>> to get a strange situation, when doing io_iopoll_complete NULL pointer
>>> dereference on file_data was caused in io_free_req_many. Interesting
>>> enough, the very same configuration doesn't fail on a newest version of
>>> fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
>>> guess it still makes sense to have this check if it's possible to craft
>>> such request to io_uring.
>>
>> I didn't looked up why it could become NULL in the first place, but the
>> problem is probably deeper.
>>
>> 1. I don't see why it puts @rb->to_free @file_data->refs, even though
>> there could be non-fixed reqs. It needs to count REQ_F_FIXED_FILE reqs
>> and put only as much.
> 
> Agree on the fixed file refs, there's a bug there where it assumes they
> are all still fixed. See below - Dmitrii, use this patch for testing
> instead of the other one!
> 
>> 2. Jens, there is another line bothering me, could you take a look?
>>
>> io_free_req_many()
>> {
>> ...
>> 	if (req->flags & REQ_F_INFLIGHT) ...;
>> 	else
>> 		rb->reqs[i] = NULL;
>> ...
>> }
>>
>> It zeroes rb->reqs[i], calls __io_req_aux_free(), but did not free
>> memory for the request itself. Is it as intended?
> 
> We free them at the end of that function, in bulk. But we can't do that
> with the aux data.

Right, we can't do that with the aux data. But we NULL a req in the
array, which then passed to kmem_cache_free_bulk(). So, it won't be
visible to the *_free_bulk(). Am I missing something?

e.g.
1. initial reqs [req1 with files, ->io, etc]
2. set to NULL, so [NULL]
3. __io_req_aux_free(req)
4. bulk_free([NULL]);

> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 32aee149f652..b5dcf6c800ef 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1218,6 +1218,8 @@ struct req_batch {
>  
>  static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>  {
> +	int fixed_refs = 0;
> +

If all are fixed, then @rb->need_iter == false (see
io_req_multi_free()), and @fixed_refs will be left 0. How about to set
it to rb->to_free, and zero+count for rb->need_iter == true?

>  	if (!rb->to_free)
>  		return;
>  	if (rb->need_iter) {
> @@ -1227,8 +1229,10 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>  		for (i = 0; i < rb->to_free; i++) {
>  			struct io_kiocb *req = rb->reqs[i];
>  
> -			if (req->flags & REQ_F_FIXED_FILE)
> +			if (req->flags & REQ_F_FIXED_FILE) {
>  				req->file = NULL;
> +				fixed_refs++;
> +			}
>  			if (req->flags & REQ_F_INFLIGHT)
>  				inflight++;
>  			else
> @@ -1255,8 +1259,9 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>  	}
>  do_free:
>  	kmem_cache_free_bulk(req_cachep, rb->to_free, rb->reqs);
> +	if (fixed_refs)
> +		percpu_ref_put_many(&ctx->file_data->refs, fixed_refs);
>  	percpu_ref_put_many(&ctx->refs, rb->to_free);
> -	percpu_ref_put_many(&ctx->file_data->refs, rb->to_free);
>  	rb->to_free = rb->need_iter = 0;
>  }
> 

-- 
Pavel Begunkov

Re: [RFC] Check if file_data is initialized

[Jens Axboe]

On 1/9/20 8:17 AM, Pavel Begunkov wrote:
> On 1/9/2020 5:51 PM, Jens Axboe wrote:
>> On 1/9/20 7:26 AM, Pavel Begunkov wrote:
>>> On 1/9/2020 4:17 PM, Dmitrii Dolgov wrote:
>>>> With combination of --fixedbufs and an old version of fio I've managed
>>>> to get a strange situation, when doing io_iopoll_complete NULL pointer
>>>> dereference on file_data was caused in io_free_req_many. Interesting
>>>> enough, the very same configuration doesn't fail on a newest version of
>>>> fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
>>>> guess it still makes sense to have this check if it's possible to craft
>>>> such request to io_uring.
>>>
>>> I didn't looked up why it could become NULL in the first place, but the
>>> problem is probably deeper.
>>>
>>> 1. I don't see why it puts @rb->to_free @file_data->refs, even though
>>> there could be non-fixed reqs. It needs to count REQ_F_FIXED_FILE reqs
>>> and put only as much.
>>
>> Agree on the fixed file refs, there's a bug there where it assumes they
>> are all still fixed. See below - Dmitrii, use this patch for testing
>> instead of the other one!
>>
>>> 2. Jens, there is another line bothering me, could you take a look?
>>>
>>> io_free_req_many()
>>> {
>>> ...
>>> 	if (req->flags & REQ_F_INFLIGHT) ...;
>>> 	else
>>> 		rb->reqs[i] = NULL;
>>> ...
>>> }
>>>
>>> It zeroes rb->reqs[i], calls __io_req_aux_free(), but did not free
>>> memory for the request itself. Is it as intended?
>>
>> We free them at the end of that function, in bulk. But we can't do that
>> with the aux data.
> 
> Right, we can't do that with the aux data. But we NULL a req in the
> array, which then passed to kmem_cache_free_bulk(). So, it won't be
> visible to the *_free_bulk(). Am I missing something?
> 
> e.g.
> 1. initial reqs [req1 with files, ->io, etc]
> 2. set to NULL, so [NULL]
> 3. __io_req_aux_free(req)
> 4. bulk_free([NULL]);

Yeah that looks wrong, I don't think you're missing something. We
should just use the flags check again. I'll double check this in
testing now.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 49622a320317..d7a77830a2f2 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1235,8 +1235,6 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
 			}
 			if (req->flags & REQ_F_INFLIGHT)
 				inflight++;
-			else
-				rb->reqs[i] = NULL;
 			__io_req_aux_free(req);
 		}
 		if (!inflight)
@@ -1246,7 +1244,7 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
 		for (i = 0; i < rb->to_free; i++) {
 			struct io_kiocb *req = rb->reqs[i];
 
-			if (req) {
+			if (req->flags & REQ_F_INFLIGHT)
 				list_del(&req->inflight_entry);
 				if (!--inflight)
 					break;

-- 
Jens Axboe

Re: [RFC] Check if file_data is initialized

[Pavel Begunkov]

On 1/9/2020 6:23 PM, Jens Axboe wrote:
> On 1/9/20 8:17 AM, Pavel Begunkov wrote:
>> On 1/9/2020 5:51 PM, Jens Axboe wrote:
>>> On 1/9/20 7:26 AM, Pavel Begunkov wrote:
>>>> On 1/9/2020 4:17 PM, Dmitrii Dolgov wrote:
>>>>> With combination of --fixedbufs and an old version of fio I've managed
>>>>> to get a strange situation, when doing io_iopoll_complete NULL pointer
>>>>> dereference on file_data was caused in io_free_req_many. Interesting
>>>>> enough, the very same configuration doesn't fail on a newest version of
>>>>> fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
>>>>> guess it still makes sense to have this check if it's possible to craft
>>>>> such request to io_uring.
>>>>
>>>> I didn't looked up why it could become NULL in the first place, but the
>>>> problem is probably deeper.
>>>>
>>>> 1. I don't see why it puts @rb->to_free @file_data->refs, even though
>>>> there could be non-fixed reqs. It needs to count REQ_F_FIXED_FILE reqs
>>>> and put only as much.
>>>
>>> Agree on the fixed file refs, there's a bug there where it assumes they
>>> are all still fixed. See below - Dmitrii, use this patch for testing
>>> instead of the other one!
>>>
>>>> 2. Jens, there is another line bothering me, could you take a look?
>>>>
>>>> io_free_req_many()
>>>> {
>>>> ...
>>>> 	if (req->flags & REQ_F_INFLIGHT) ...;
>>>> 	else
>>>> 		rb->reqs[i] = NULL;
>>>> ...
>>>> }
>>>>
>>>> It zeroes rb->reqs[i], calls __io_req_aux_free(), but did not free
>>>> memory for the request itself. Is it as intended?
>>>
>>> We free them at the end of that function, in bulk. But we can't do that
>>> with the aux data.
>>
>> Right, we can't do that with the aux data. But we NULL a req in the
>> array, which then passed to kmem_cache_free_bulk(). So, it won't be
>> visible to the *_free_bulk(). Am I missing something?
>>
>> e.g.
>> 1. initial reqs [req1 with files, ->io, etc]
>> 2. set to NULL, so [NULL]
>> 3. __io_req_aux_free(req)
>> 4. bulk_free([NULL]);
> 
> Yeah that looks wrong, I don't think you're missing something. We
> should just use the flags check again. I'll double check this in
> testing now.

Great, thanks!

BTW, if by any chance you missed it, there was another comment in my
previous mail regarding your fix for the put problem.

> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 49622a320317..d7a77830a2f2 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1235,8 +1235,6 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>  			}
>  			if (req->flags & REQ_F_INFLIGHT)
>  				inflight++;
> -			else
> -				rb->reqs[i] = NULL;
>  			__io_req_aux_free(req);
>  		}
>  		if (!inflight)
> @@ -1246,7 +1244,7 @@ static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>  		for (i = 0; i < rb->to_free; i++) {
>  			struct io_kiocb *req = rb->reqs[i];
>  
> -			if (req) {
> +			if (req->flags & REQ_F_INFLIGHT)
>  				list_del(&req->inflight_entry);
>  				if (!--inflight)
>  					break;
> 

-- 
Pavel Begunkov

Re: [RFC] Check if file_data is initialized

[Jens Axboe]

On 1/9/20 8:17 AM, Pavel Begunkov wrote:
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 32aee149f652..b5dcf6c800ef 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -1218,6 +1218,8 @@ struct req_batch {
>>  
>>  static void io_free_req_many(struct io_ring_ctx *ctx, struct req_batch *rb)
>>  {
>> +	int fixed_refs = 0;
>> +
> 
> If all are fixed, then @rb->need_iter == false (see
> io_req_multi_free()), and @fixed_refs will be left 0. How about to set
> it to rb->to_free, and zero+count for rb->need_iter == true?

Good catch, I'll make that change.

-- 
Jens Axboe

Re: [RFC] Check if file_data is initialized

[Dmitry Dolgov]

> On Thu, Jan 09, 2020 at 07:51:28AM -0700, Jens Axboe wrote:
> On 1/9/20 7:26 AM, Pavel Begunkov wrote:
> > On 1/9/2020 4:17 PM, Dmitrii Dolgov wrote:
> >> With combination of --fixedbufs and an old version of fio I've managed
> >> to get a strange situation, when doing io_iopoll_complete NULL pointer
> >> dereference on file_data was caused in io_free_req_many. Interesting
> >> enough, the very same configuration doesn't fail on a newest version of
> >> fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
> >> guess it still makes sense to have this check if it's possible to craft
> >> such request to io_uring.
> >
> > I didn't looked up why it could become NULL in the first place, but the
> > problem is probably deeper.
> >
> > 1. I don't see why it puts @rb->to_free @file_data->refs, even though
> > there could be non-fixed reqs. It needs to count REQ_F_FIXED_FILE reqs
> > and put only as much.
>
> Agree on the fixed file refs, there's a bug there where it assumes they
> are all still fixed. See below - Dmitrii, use this patch for testing
> instead of the other one!

Yes, the patch from this email also fixes the issue.

Re: [RFC] Check if file_data is initialized

[Jens Axboe]

On 1/9/20 9:04 AM, Dmitry Dolgov wrote:
>> On Thu, Jan 09, 2020 at 07:51:28AM -0700, Jens Axboe wrote:
>> On 1/9/20 7:26 AM, Pavel Begunkov wrote:
>>> On 1/9/2020 4:17 PM, Dmitrii Dolgov wrote:
>>>> With combination of --fixedbufs and an old version of fio I've managed
>>>> to get a strange situation, when doing io_iopoll_complete NULL pointer
>>>> dereference on file_data was caused in io_free_req_many. Interesting
>>>> enough, the very same configuration doesn't fail on a newest version of
>>>> fio (the old one is fc220349e4514, the new one is 2198a6b5a9f4), but I
>>>> guess it still makes sense to have this check if it's possible to craft
>>>> such request to io_uring.
>>>
>>> I didn't looked up why it could become NULL in the first place, but the
>>> problem is probably deeper.
>>>
>>> 1. I don't see why it puts @rb->to_free @file_data->refs, even though
>>> there could be non-fixed reqs. It needs to count REQ_F_FIXED_FILE reqs
>>> and put only as much.
>>
>> Agree on the fixed file refs, there's a bug there where it assumes they
>> are all still fixed. See below - Dmitrii, use this patch for testing
>> instead of the other one!
> 
> Yes, the patch from this email also fixes the issue.

Great, thanks for testing, I'll add your Tested-by to the commit.

-- 
Jens Axboe