[BUG] io_uring: defer logic based on shared data

[BUG] io_uring: defer logic based on shared data

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 343 bytes --]

I found 2 problems with __io_sequence_defer().

1. it uses @sq_dropped, but doesn't consider @cq_overflow
2. @sq_dropped and @cq_overflow are write-shared with userspace, so
it can be maliciously changed.

see sent liburing test (test/defer *_hung()), which left an unkillable
process for me

-- 
Yours sincerely,
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 3:55 AM, Pavel Begunkov wrote:
> I found 2 problems with __io_sequence_defer().
> 
> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
> it can be maliciously changed.
> 
> see sent liburing test (test/defer *_hung()), which left an unkillable
> process for me

OK, how about the below. I'll split this in two, as it's really two
separate fixes.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5d10984381cf..5d9d960c1c17 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -191,6 +191,7 @@ struct io_ring_ctx {
 		unsigned		sq_entries;
 		unsigned		sq_mask;
 		unsigned		sq_thread_idle;
+		unsigned		cached_sq_dropped;
 		struct io_uring_sqe	*sq_sqes;
 
 		struct list_head	defer_list;
@@ -208,6 +209,7 @@ struct io_ring_ctx {
 
 	struct {
 		unsigned		cached_cq_tail;
+		unsigned		cached_cq_overflow;
 		unsigned		cq_entries;
 		unsigned		cq_mask;
 		struct wait_queue_head	cq_wait;
@@ -419,7 +421,8 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
 				       struct io_kiocb *req)
 {
-	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
+	return req->sequence != ctx->cached_cq_tail + ctx->cached_sq_dropped
+					+ ctx->cached_cq_overflow;
 }
 
 static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
@@ -590,9 +593,8 @@ static void io_cqring_fill_event(struct io_ring_ctx *ctx, u64 ki_user_data,
 		WRITE_ONCE(cqe->res, res);
 		WRITE_ONCE(cqe->flags, 0);
 	} else {
-		unsigned overflow = READ_ONCE(ctx->rings->cq_overflow);
-
-		WRITE_ONCE(ctx->rings->cq_overflow, overflow + 1);
+		ctx->cached_cq_overflow++;
+		WRITE_ONCE(ctx->rings->cq_overflow, ctx->cached_cq_overflow);
 	}
 }
 
@@ -2601,7 +2603,8 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
 
 	/* drop invalid entries */
 	ctx->cached_sq_head++;
-	rings->sq_dropped++;
+	ctx->cached_sq_dropped++;
+	WRITE_ONCE(rings->sq_dropped, ctx->cached_sq_dropped);
 	return false;
 }
 
@@ -2685,6 +2688,7 @@ static int io_sq_thread(void *data)
 
 	timeout = inflight = 0;
 	while (!kthread_should_park()) {
+		unsigned prev_cq, cur_cq;
 		bool mm_fault = false;
 		unsigned int to_submit;
 
@@ -2767,8 +2771,12 @@ static int io_sq_thread(void *data)
 		}
 
 		to_submit = min(to_submit, ctx->sq_entries);
+		prev_cq = ctx->cached_cq_tail + ctx->cached_cq_overflow;
 		inflight += io_submit_sqes(ctx, to_submit, cur_mm != NULL,
 					   mm_fault);
+		cur_cq = ctx->cached_cq_tail + ctx->cached_cq_overflow;
+		if ((ctx->flags & IORING_SETUP_IOPOLL) && (prev_cq != cur_cq))
+			inflight -= cur_cq - prev_cq;
 
 		/* Commit SQ ring head once we've consumed all SQEs */
 		io_commit_sqring(ctx);

-- 
Jens Axboe

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 10:03 AM, Jens Axboe wrote:
> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>> I found 2 problems with __io_sequence_defer().
>>
>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>> it can be maliciously changed.
>>
>> see sent liburing test (test/defer *_hung()), which left an unkillable
>> process for me
> 
> OK, how about the below. I'll split this in two, as it's really two
> separate fixes.

Patch 1:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=9a9a21d9cf65cb621cce4052a4527868a80009ad

and patch 2:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=ed348662f74c4f63537b3c188585e39cdea22713

Let me know what you think, and if/when I can add your reviewed/test-by
to them.

-- 
Jens Axboe

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 10:09 AM, Jens Axboe wrote:
> On 10/25/19 10:03 AM, Jens Axboe wrote:
>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>> I found 2 problems with __io_sequence_defer().
>>>
>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>> it can be maliciously changed.
>>>
>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>> process for me
>>
>> OK, how about the below. I'll split this in two, as it's really two
>> separate fixes.
> 
> Patch 1:
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=9a9a21d9cf65cb621cce4052a4527868a80009ad
> 
> and patch 2:
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=ed348662f74c4f63537b3c188585e39cdea22713
> 
> Let me know what you think, and if/when I can add your reviewed/test-by
> to them.

Updated patch 2 as per the other discussion, here it is:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b6c2c446c0fca0318dec904821bd11f52d2445d3


-- 
Jens Axboe

Re: [BUG] io_uring: defer logic based on shared data

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 3294 bytes --]

On 25/10/2019 19:03, Jens Axboe wrote:
> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>> I found 2 problems with __io_sequence_defer().
>>
>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>> it can be maliciously changed.
>>
>> see sent liburing test (test/defer *_hung()), which left an unkillable
>> process for me
> 
> OK, how about the below. I'll split this in two, as it's really two
> separate fixes.
cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
io_cqring_fill_event() can be called in async, so shouldn't we do some
synchronisation then?

> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 5d10984381cf..5d9d960c1c17 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -191,6 +191,7 @@ struct io_ring_ctx {
>  		unsigned		sq_entries;
>  		unsigned		sq_mask;
>  		unsigned		sq_thread_idle;
> +		unsigned		cached_sq_dropped;
>  		struct io_uring_sqe	*sq_sqes;
>  
>  		struct list_head	defer_list;
> @@ -208,6 +209,7 @@ struct io_ring_ctx {
>  
>  	struct {
>  		unsigned		cached_cq_tail;
> +		unsigned		cached_cq_overflow;
>  		unsigned		cq_entries;
>  		unsigned		cq_mask;
>  		struct wait_queue_head	cq_wait;
> @@ -419,7 +421,8 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>  static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
>  				       struct io_kiocb *req)
>  {
> -	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
> +	return req->sequence != ctx->cached_cq_tail + ctx->cached_sq_dropped
> +					+ ctx->cached_cq_overflow;
>  }
>  
>  static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
> @@ -590,9 +593,8 @@ static void io_cqring_fill_event(struct io_ring_ctx *ctx, u64 ki_user_data,
>  		WRITE_ONCE(cqe->res, res);
>  		WRITE_ONCE(cqe->flags, 0);
>  	} else {
> -		unsigned overflow = READ_ONCE(ctx->rings->cq_overflow);
> -
> -		WRITE_ONCE(ctx->rings->cq_overflow, overflow + 1);
> +		ctx->cached_cq_overflow++;
> +		WRITE_ONCE(ctx->rings->cq_overflow, ctx->cached_cq_overflow);
>  	}
>  }
>  
> @@ -2601,7 +2603,8 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
>  
>  	/* drop invalid entries */
>  	ctx->cached_sq_head++;
> -	rings->sq_dropped++;
> +	ctx->cached_sq_dropped++;
> +	WRITE_ONCE(rings->sq_dropped, ctx->cached_sq_dropped);
>  	return false;
>  }
>  
> @@ -2685,6 +2688,7 @@ static int io_sq_thread(void *data)
>  
>  	timeout = inflight = 0;
>  	while (!kthread_should_park()) {
> +		unsigned prev_cq, cur_cq;
>  		bool mm_fault = false;
>  		unsigned int to_submit;
>  
> @@ -2767,8 +2771,12 @@ static int io_sq_thread(void *data)
>  		}
>  
>  		to_submit = min(to_submit, ctx->sq_entries);
> +		prev_cq = ctx->cached_cq_tail + ctx->cached_cq_overflow;
>  		inflight += io_submit_sqes(ctx, to_submit, cur_mm != NULL,
>  					   mm_fault);
> +		cur_cq = ctx->cached_cq_tail + ctx->cached_cq_overflow;
> +		if ((ctx->flags & IORING_SETUP_IOPOLL) && (prev_cq != cur_cq))
> +			inflight -= cur_cq - prev_cq;
>  
>  		/* Commit SQ ring head once we've consumed all SQEs */
>  		io_commit_sqring(ctx);
> 

-- 
Yours sincerely,
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 10:21 AM, Pavel Begunkov wrote:
> On 25/10/2019 19:03, Jens Axboe wrote:
>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>> I found 2 problems with __io_sequence_defer().
>>>
>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>> it can be maliciously changed.
>>>
>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>> process for me
>>
>> OK, how about the below. I'll split this in two, as it's really two
>> separate fixes.
> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
> io_cqring_fill_event() can be called in async, so shouldn't we do some
> synchronisation then?

We should probably make it an atomic just to be on the safe side, I'll
update the series.

-- 
Jens Axboe

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 10:27 AM, Jens Axboe wrote:
> On 10/25/19 10:21 AM, Pavel Begunkov wrote:
>> On 25/10/2019 19:03, Jens Axboe wrote:
>>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>>> I found 2 problems with __io_sequence_defer().
>>>>
>>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>>> it can be maliciously changed.
>>>>
>>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>>> process for me
>>>
>>> OK, how about the below. I'll split this in two, as it's really two
>>> separate fixes.
>> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
>> io_cqring_fill_event() can be called in async, so shouldn't we do some
>> synchronisation then?
> 
> We should probably make it an atomic just to be on the safe side, I'll
> update the series.

Here we go, patch 1:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=f2a241f596ed9e12b7c8f960e79ccda8053ea294

patch 2:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b7d0297d2df5bfa0d1ecf9d6c66d23676751ef6a

-- 
Jens Axboe

Re: [BUG] io_uring: defer logic based on shared data

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 1493 bytes --]

On 25/10/2019 19:32, Jens Axboe wrote:
> On 10/25/19 10:27 AM, Jens Axboe wrote:
>> On 10/25/19 10:21 AM, Pavel Begunkov wrote:
>>> On 25/10/2019 19:03, Jens Axboe wrote:
>>>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>>>> I found 2 problems with __io_sequence_defer().
>>>>>
>>>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>>>> it can be maliciously changed.
>>>>>
>>>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>>>> process for me
>>>>
>>>> OK, how about the below. I'll split this in two, as it's really two
>>>> separate fixes.
>>> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
>>> io_cqring_fill_event() can be called in async, so shouldn't we do some
>>> synchronisation then?
>>
>> We should probably make it an atomic just to be on the safe side, I'll
>> update the series.
> 
> Here we go, patch 1:
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=f2a241f596ed9e12b7c8f960e79ccda8053ea294
> 
> patch 2:
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b7d0297d2df5bfa0d1ecf9d6c66d23676751ef6a
> 
1. submit rqs (not yet completed)
2. poll_list is empty, inflight = 0
3. async completed and placed into poll_list

So, poll_list is not empty, but we won't get to polling again.
At least until someone submitted something.

-- 
Yours sincerely,
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 10:40 AM, Pavel Begunkov wrote:
> On 25/10/2019 19:32, Jens Axboe wrote:
>> On 10/25/19 10:27 AM, Jens Axboe wrote:
>>> On 10/25/19 10:21 AM, Pavel Begunkov wrote:
>>>> On 25/10/2019 19:03, Jens Axboe wrote:
>>>>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>>>>> I found 2 problems with __io_sequence_defer().
>>>>>>
>>>>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>>>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>>>>> it can be maliciously changed.
>>>>>>
>>>>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>>>>> process for me
>>>>>
>>>>> OK, how about the below. I'll split this in two, as it's really two
>>>>> separate fixes.
>>>> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
>>>> io_cqring_fill_event() can be called in async, so shouldn't we do some
>>>> synchronisation then?
>>>
>>> We should probably make it an atomic just to be on the safe side, I'll
>>> update the series.
>>
>> Here we go, patch 1:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=f2a241f596ed9e12b7c8f960e79ccda8053ea294
>>
>> patch 2:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b7d0297d2df5bfa0d1ecf9d6c66d23676751ef6a
>>
> 1. submit rqs (not yet completed)
> 2. poll_list is empty, inflight = 0
> 3. async completed and placed into poll_list
> 
> So, poll_list is not empty, but we won't get to polling again.
> At least until someone submitted something.

But if they are issued, the will sit in ->poll_list as well. That list
holds both "submitted, but pending" and completed entries.

-- 
Jens Axboe

Re: [BUG] io_uring: defer logic based on shared data

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 2086 bytes --]

On 25/10/2019 19:44, Jens Axboe wrote:
> On 10/25/19 10:40 AM, Pavel Begunkov wrote:
>> On 25/10/2019 19:32, Jens Axboe wrote:
>>> On 10/25/19 10:27 AM, Jens Axboe wrote:
>>>> On 10/25/19 10:21 AM, Pavel Begunkov wrote:
>>>>> On 25/10/2019 19:03, Jens Axboe wrote:
>>>>>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>>>>>> I found 2 problems with __io_sequence_defer().
>>>>>>>
>>>>>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>>>>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>>>>>> it can be maliciously changed.
>>>>>>>
>>>>>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>>>>>> process for me
>>>>>>
>>>>>> OK, how about the below. I'll split this in two, as it's really two
>>>>>> separate fixes.
>>>>> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
>>>>> io_cqring_fill_event() can be called in async, so shouldn't we do some
>>>>> synchronisation then?
>>>>
>>>> We should probably make it an atomic just to be on the safe side, I'll
>>>> update the series.
>>>
>>> Here we go, patch 1:
>>>
>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=f2a241f596ed9e12b7c8f960e79ccda8053ea294
>>>
>>> patch 2:
>>>
>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b7d0297d2df5bfa0d1ecf9d6c66d23676751ef6a
>>>
>> 1. submit rqs (not yet completed)
>> 2. poll_list is empty, inflight = 0
>> 3. async completed and placed into poll_list
>>
>> So, poll_list is not empty, but we won't get to polling again.
>> At least until someone submitted something.
> 
> But if they are issued, the will sit in ->poll_list as well. That list
> holds both "submitted, but pending" and completed entries.
> 
Missed it, then should work. Thanks!

> + ret = iters = 0;
A small suggestion, could we just initialise it in declaration
to be a bit more concise?
e.g. int ret = 0, iters = 0; 

Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
And let me test it as both patches are ready.

-- 
Yours sincerely,
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 10:55 AM, Pavel Begunkov wrote:
> On 25/10/2019 19:44, Jens Axboe wrote:
>> On 10/25/19 10:40 AM, Pavel Begunkov wrote:
>>> On 25/10/2019 19:32, Jens Axboe wrote:
>>>> On 10/25/19 10:27 AM, Jens Axboe wrote:
>>>>> On 10/25/19 10:21 AM, Pavel Begunkov wrote:
>>>>>> On 25/10/2019 19:03, Jens Axboe wrote:
>>>>>>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>>>>>>> I found 2 problems with __io_sequence_defer().
>>>>>>>>
>>>>>>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>>>>>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>>>>>>> it can be maliciously changed.
>>>>>>>>
>>>>>>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>>>>>>> process for me
>>>>>>>
>>>>>>> OK, how about the below. I'll split this in two, as it's really two
>>>>>>> separate fixes.
>>>>>> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
>>>>>> io_cqring_fill_event() can be called in async, so shouldn't we do some
>>>>>> synchronisation then?
>>>>>
>>>>> We should probably make it an atomic just to be on the safe side, I'll
>>>>> update the series.
>>>>
>>>> Here we go, patch 1:
>>>>
>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=f2a241f596ed9e12b7c8f960e79ccda8053ea294
>>>>
>>>> patch 2:
>>>>
>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b7d0297d2df5bfa0d1ecf9d6c66d23676751ef6a
>>>>
>>> 1. submit rqs (not yet completed)
>>> 2. poll_list is empty, inflight = 0
>>> 3. async completed and placed into poll_list
>>>
>>> So, poll_list is not empty, but we won't get to polling again.
>>> At least until someone submitted something.
>>
>> But if they are issued, the will sit in ->poll_list as well. That list
>> holds both "submitted, but pending" and completed entries.
>>
> Missed it, then should work. Thanks!

Glad we agree :-)

>> + ret = iters = 0;
> A small suggestion, could we just initialise it in declaration
> to be a bit more concise?
> e.g. int ret = 0, iters = 0;
> 
> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
> And let me test it as both patches are ready.

Sure, I'll make that change and add your reviewed-by. Thanks!

-- 
Jens Axboe

Re: [BUG] io_uring: defer logic based on shared data

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 2461 bytes --]

On 25/10/2019 19:57, Jens Axboe wrote:
> On 10/25/19 10:55 AM, Pavel Begunkov wrote:
>> On 25/10/2019 19:44, Jens Axboe wrote:
>>> On 10/25/19 10:40 AM, Pavel Begunkov wrote:
>>>> On 25/10/2019 19:32, Jens Axboe wrote:
>>>>> On 10/25/19 10:27 AM, Jens Axboe wrote:
>>>>>> On 10/25/19 10:21 AM, Pavel Begunkov wrote:
>>>>>>> On 25/10/2019 19:03, Jens Axboe wrote:
>>>>>>>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>>>>>>>> I found 2 problems with __io_sequence_defer().
>>>>>>>>>
>>>>>>>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>>>>>>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>>>>>>>> it can be maliciously changed.
>>>>>>>>>
>>>>>>>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>>>>>>>> process for me
>>>>>>>>
>>>>>>>> OK, how about the below. I'll split this in two, as it's really two
>>>>>>>> separate fixes.
>>>>>>> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
>>>>>>> io_cqring_fill_event() can be called in async, so shouldn't we do some
>>>>>>> synchronisation then?
>>>>>>
>>>>>> We should probably make it an atomic just to be on the safe side, I'll
>>>>>> update the series.
>>>>>
>>>>> Here we go, patch 1:
>>>>>
>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=f2a241f596ed9e12b7c8f960e79ccda8053ea294
>>>>>
>>>>> patch 2:
>>>>>
>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b7d0297d2df5bfa0d1ecf9d6c66d23676751ef6a
>>>>>
>>>> 1. submit rqs (not yet completed)
>>>> 2. poll_list is empty, inflight = 0
>>>> 3. async completed and placed into poll_list
>>>>
>>>> So, poll_list is not empty, but we won't get to polling again.
>>>> At least until someone submitted something.
>>>
>>> But if they are issued, the will sit in ->poll_list as well. That list
>>> holds both "submitted, but pending" and completed entries.
>>>
>> Missed it, then should work. Thanks!
> 
> Glad we agree :-)
> 
>>> + ret = iters = 0;
>> A small suggestion, could we just initialise it in declaration
>> to be a bit more concise?
>> e.g. int ret = 0, iters = 0;
>>
>> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
>> And let me test it as both patches are ready.
> 
> Sure, I'll make that change and add your reviewed-by. Thanks!
> 
Stress tested, works well!

Tested-by: Pavel Begunkov <asml.silence@gmail.com>

-- 
Yours sincerely,
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [BUG] io_uring: defer logic based on shared data

[Jens Axboe]

On 10/25/19 12:13 PM, Pavel Begunkov wrote:
> On 25/10/2019 19:57, Jens Axboe wrote:
>> On 10/25/19 10:55 AM, Pavel Begunkov wrote:
>>> On 25/10/2019 19:44, Jens Axboe wrote:
>>>> On 10/25/19 10:40 AM, Pavel Begunkov wrote:
>>>>> On 25/10/2019 19:32, Jens Axboe wrote:
>>>>>> On 10/25/19 10:27 AM, Jens Axboe wrote:
>>>>>>> On 10/25/19 10:21 AM, Pavel Begunkov wrote:
>>>>>>>> On 25/10/2019 19:03, Jens Axboe wrote:
>>>>>>>>> On 10/25/19 3:55 AM, Pavel Begunkov wrote:
>>>>>>>>>> I found 2 problems with __io_sequence_defer().
>>>>>>>>>>
>>>>>>>>>> 1. it uses @sq_dropped, but doesn't consider @cq_overflow
>>>>>>>>>> 2. @sq_dropped and @cq_overflow are write-shared with userspace, so
>>>>>>>>>> it can be maliciously changed.
>>>>>>>>>>
>>>>>>>>>> see sent liburing test (test/defer *_hung()), which left an unkillable
>>>>>>>>>> process for me
>>>>>>>>>
>>>>>>>>> OK, how about the below. I'll split this in two, as it's really two
>>>>>>>>> separate fixes.
>>>>>>>> cached_sq_dropped is good, but I was concerned about cached_cq_overflow.
>>>>>>>> io_cqring_fill_event() can be called in async, so shouldn't we do some
>>>>>>>> synchronisation then?
>>>>>>>
>>>>>>> We should probably make it an atomic just to be on the safe side, I'll
>>>>>>> update the series.
>>>>>>
>>>>>> Here we go, patch 1:
>>>>>>
>>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=f2a241f596ed9e12b7c8f960e79ccda8053ea294
>>>>>>
>>>>>> patch 2:
>>>>>>
>>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=b7d0297d2df5bfa0d1ecf9d6c66d23676751ef6a
>>>>>>
>>>>> 1. submit rqs (not yet completed)
>>>>> 2. poll_list is empty, inflight = 0
>>>>> 3. async completed and placed into poll_list
>>>>>
>>>>> So, poll_list is not empty, but we won't get to polling again.
>>>>> At least until someone submitted something.
>>>>
>>>> But if they are issued, the will sit in ->poll_list as well. That list
>>>> holds both "submitted, but pending" and completed entries.
>>>>
>>> Missed it, then should work. Thanks!
>>
>> Glad we agree :-)
>>
>>>> + ret = iters = 0;
>>> A small suggestion, could we just initialise it in declaration
>>> to be a bit more concise?
>>> e.g. int ret = 0, iters = 0;
>>>
>>> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
>>> And let me test it as both patches are ready.
>>
>> Sure, I'll make that change and add your reviewed-by. Thanks!
>>
> Stress tested, works well!
> 
> Tested-by: Pavel Begunkov <asml.silence@gmail.com>

Great, thanks for finding these, sending patches, and testing the ones
that I fixed!

-- 
Jens Axboe