On 8/31/20 10:59 PM, yinxin_1989 wrote:
>
>>On 8/31/20 7:54 PM, Xin Yin wrote:
>>> the commit <1c4404efcf2c0> ("<io_uring: make sure async workqueue
>>> is canceled on exit>") caused a crash in io_sq_wq_submit_work().
>>> when io_ring-wq get a req form async_list, which may not have been
>>> added to task_list. Then try to delete the req from task_list will caused
>>> a "NULL pointer dereference".
>>
>>Hmm, do you have a reproducer for this?
>
> I update code to linux5.4.y , and I can reproduce this issue on an arm
> board and my x86 pc by fio tools.
Right, I figured this was 5.4 stable, as that's the only version that
has this patch.
> fio -filename=/home/yinxin/testfile -direct=0 -ioengine=io_uring -iodepth 128 -rw=read -bs=16K -size=1G -numjobs=1 -runtime=60 -group_reporting -name=iops
Gotcha, thanks!
>>> @@ -2356,9 +2358,11 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>>> * running. We currently only allow this if the new request is sequential
>>> * to the previous one we punted.
>>> */
>>> -static bool io_add_to_prev_work(struct async_list *list, struct io_kiocb *req)
>>> +static bool io_add_to_prev_work(struct async_list *list, struct io_kiocb *req,
>>> + struct io_ring_ctx *ctx)
>>> {
>>> bool ret;
>>> + unsigned long flags;
>>>
>>> if (!list)
>>> return false;
>>> @@ -2378,6 +2382,13 @@ static bool io_add_to_prev_work(struct async_list *list, struct io_kiocb *req)
>>> list_del_init(&req->list);
>>> ret = false;
>>> }
>>> +
>>> + if (ret) {
>>> + spin_lock_irqsave(&ctx->task_lock, flags);
>>> + list_add(&req->task_list, &ctx->task_list);
>>> + req->work_task = NULL;
>>> + spin_unlock_irqrestore(&ctx->task_lock, flags);
>>> + }
>>> spin_unlock(&list->lock);
>>> return ret;
>>> }
>>> @@ -2454,7 +2465,7 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>> s->sqe = sqe_copy;
>>> memcpy(&req->submit, s, sizeof(*s));
>>> list = io_async_list_from_req(ctx, req);
>>> - if (!io_add_to_prev_work(list, req)) {
>>> + if (!io_add_to_prev_work(list, req, ctx)) {
>>> if (list)
>>> atomic_inc(&list->cnt);
>>> INIT_WORK(&req->work, io_sq_wq_submit_work);
>>>
>>ctx == req->ctx, so you should not need that change.
>
> In my test , the req have not been add to req->task_list(maybe waiting
> for the ctx->task_lock) , and in io_sq_wq_submit_work() try to delete
> it from req->task_list ,which will cause this issue.
Sure, but req->ctx is set when the req is initialized. If req->ctx !=
ctx here, then that would be pretty disastrous... So you can drop that
part of the patch.
Care to send with that changed? Then I'm fine with queueing this up for
5.4-stable. Thanks!
--
Jens Axboe
On 9/1/20 8:52 AM, Jens Axboe wrote:
> On 8/31/20 10:59 PM, yinxin_1989 wrote:
>>
>>> On 8/31/20 7:54 PM, Xin Yin wrote:
>>>> the commit <1c4404efcf2c0> ("<io_uring: make sure async workqueue
>>>> is canceled on exit>") caused a crash in io_sq_wq_submit_work().
>>>> when io_ring-wq get a req form async_list, which may not have been
>>>> added to task_list. Then try to delete the req from task_list will caused
>>>> a "NULL pointer dereference".
>>>
>>> Hmm, do you have a reproducer for this?
>>
>> I update code to linux5.4.y , and I can reproduce this issue on an arm
>> board and my x86 pc by fio tools.
>
> Right, I figured this was 5.4 stable, as that's the only version that
> has this patch.
I took a closer look, and I think your patch can basically be boiled down
to this single hunk. If you agree, can you resend your patch with the
description based on that, then I'll get it queued up for 5.4-stable.
Thanks!
diff --git a/fs/io_uring.c b/fs/io_uring.c
index fada14ee1cdc..cbbcd85780c4 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2378,6 +2378,16 @@ static bool io_add_to_prev_work(struct async_list *list, struct io_kiocb *req)
list_del_init(&req->list);
ret = false;
}
+
+ if (ret) {
+ struct io_ring_ctx *ctx = req->ctx;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ctx->task_lock, flags);
+ list_add(&req->task_list, &ctx->task_list);
+ req->work_task = NULL;
+ spin_unlock_irqrestore(&ctx->task_lock, flags);
+ }
spin_unlock(&list->lock);
return ret;
}
--
Jens Axboe
On 9/1/20 2:01 PM, Jens Axboe wrote:
> On 9/1/20 8:52 AM, Jens Axboe wrote:
>> On 8/31/20 10:59 PM, yinxin_1989 wrote:
>>>
>>>> On 8/31/20 7:54 PM, Xin Yin wrote:
>>>>> the commit <1c4404efcf2c0> ("<io_uring: make sure async workqueue
>>>>> is canceled on exit>") caused a crash in io_sq_wq_submit_work().
>>>>> when io_ring-wq get a req form async_list, which may not have been
>>>>> added to task_list. Then try to delete the req from task_list will caused
>>>>> a "NULL pointer dereference".
>>>>
>>>> Hmm, do you have a reproducer for this?
>>>
>>> I update code to linux5.4.y , and I can reproduce this issue on an arm
>>> board and my x86 pc by fio tools.
>>
>> Right, I figured this was 5.4 stable, as that's the only version that
>> has this patch.
>
> I took a closer look, and I think your patch can basically be boiled down
> to this single hunk. If you agree, can you resend your patch with the
> description based on that, then I'll get it queued up for 5.4-stable.
> Thanks!
Actually, we don't even need the irqsave, this should be enough:
diff --git a/fs/io_uring.c b/fs/io_uring.c
index fada14ee1cdc..2a539b794f3b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2378,6 +2378,15 @@ static bool io_add_to_prev_work(struct async_list *list, struct io_kiocb *req)
list_del_init(&req->list);
ret = false;
}
+
+ if (ret) {
+ struct io_ring_ctx *ctx = req->ctx;
+
+ spin_lock_irq(&ctx->task_lock);
+ list_add(&req->task_list, &ctx->task_list);
+ req->work_task = NULL;
+ spin_unlock_irq(&ctx->task_lock);
+ }
spin_unlock(&list->lock);
return ret;
}
--
Jens Axboe