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
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
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
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
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
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
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 <[email protected]>
And let me test it as both patches are ready.
--
Yours sincerely,
Pavel Begunkov
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
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
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
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 <[email protected]>
> And let me test it as both patches are ready.
Sure, I'll make that change and add your reviewed-by. Thanks!
--
Jens Axboe
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 <[email protected]>
>> 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 <[email protected]>
--
Yours sincerely,
Pavel Begunkov
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 <[email protected]>
>>> 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 <[email protected]>
Great, thanks for finding these, sending patches, and testing the ones
that I fixed!
--
Jens Axboe