public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io_uring: fix io_wqe->work_list corruption
@ 2020-12-18  7:26 Xiaoguang Wang
  2020-12-18 15:10 ` Pavel Begunkov
  2020-12-18 15:15 ` Jens Axboe
  0 siblings, 2 replies; 3+ messages in thread
From: Xiaoguang Wang @ 2020-12-18  7:26 UTC (permalink / raw)
  To: io-uring; +Cc: axboe, asml.silence, joseph.qi

For the first time a req punted to io-wq, we'll initialize io_wq_work's
list to be NULL, then insert req to io_wqe->work_list. If this req is not
inserted into tail of io_wqe->work_list, this req's io_wq_work list will
point to another req's io_wq_work. For splitted bio case, this req maybe
inserted to io_wqe->work_list repeatedly, once we insert it to tail of
io_wqe->work_list for the second time, now io_wq_work->list->next will be
invalid pointer, which then result in many strang error, panic, kernel
soft-lockup, rcu stall, etc.

In my vm, kernel doest not have commit cc29e1bf0d63f7 ("block: disable
iopoll for split bio"), below fio job can reproduce this bug steadily:
[global]
name=iouring-sqpoll-iopoll-1
ioengine=io_uring
iodepth=128
numjobs=1
thread
rw=randread
direct=1
registerfiles=1
hipri=1
bs=4m
size=100M
runtime=120
time_based
group_reporting
randrepeat=0

[device]
directory=/home/feiman.wxg/mntpoint/  # an ext4 mount point

If we have commit cc29e1bf0d63f7 ("block: disable iopoll for split bio"),
there will no splitted bio case for polled io, but I think we still to need
to fix this list corruption, it also should maybe go to stable branchs.

To fix this corruption, if a req is inserted into tail of io_wqe->work_list,
initialize req->io_wq_work->list->next to bu NULL.

Signed-off-by: Xiaoguang Wang <[email protected]>
---
 fs/io-wq.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/io-wq.h b/fs/io-wq.h
index 069496c6d4f9..75113bcd5889 100644
--- a/fs/io-wq.h
+++ b/fs/io-wq.h
@@ -59,6 +59,7 @@ static inline void wq_list_add_tail(struct io_wq_work_node *node,
 		list->last->next = node;
 		list->last = node;
 	}
+	node->next = NULL;
 }
 
 static inline void wq_list_cut(struct io_wq_work_list *list,
-- 
2.17.2


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

* Re: [PATCH] io_uring: fix io_wqe->work_list corruption
  2020-12-18  7:26 [PATCH] io_uring: fix io_wqe->work_list corruption Xiaoguang Wang
@ 2020-12-18 15:10 ` Pavel Begunkov
  2020-12-18 15:15 ` Jens Axboe
  1 sibling, 0 replies; 3+ messages in thread
From: Pavel Begunkov @ 2020-12-18 15:10 UTC (permalink / raw)
  To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi

On 18/12/2020 07:26, Xiaoguang Wang wrote:
> For the first time a req punted to io-wq, we'll initialize io_wq_work's
> list to be NULL, then insert req to io_wqe->work_list. If this req is not
> inserted into tail of io_wqe->work_list, this req's io_wq_work list will
> point to another req's io_wq_work. For splitted bio case, this req maybe
> inserted to io_wqe->work_list repeatedly, once we insert it to tail of
> io_wqe->work_list for the second time, now io_wq_work->list->next will be
> invalid pointer, which then result in many strang error, panic, kernel
> soft-lockup, rcu stall, etc.
[...]
> To fix this corruption, if a req is inserted into tail of io_wqe->work_list,
> initialize req->io_wq_work->list->next to bu NULL.

Looks fine, and the function is cold to not care about overhead.

Reviewed-by: Pavel Begunkov <[email protected]>

-- 
Pavel Begunkov

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

* Re: [PATCH] io_uring: fix io_wqe->work_list corruption
  2020-12-18  7:26 [PATCH] io_uring: fix io_wqe->work_list corruption Xiaoguang Wang
  2020-12-18 15:10 ` Pavel Begunkov
@ 2020-12-18 15:15 ` Jens Axboe
  1 sibling, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2020-12-18 15:15 UTC (permalink / raw)
  To: Xiaoguang Wang, io-uring; +Cc: asml.silence, joseph.qi

On 12/18/20 12:26 AM, Xiaoguang Wang wrote:
> For the first time a req punted to io-wq, we'll initialize io_wq_work's
> list to be NULL, then insert req to io_wqe->work_list. If this req is not
> inserted into tail of io_wqe->work_list, this req's io_wq_work list will
> point to another req's io_wq_work. For splitted bio case, this req maybe
> inserted to io_wqe->work_list repeatedly, once we insert it to tail of
> io_wqe->work_list for the second time, now io_wq_work->list->next will be
> invalid pointer, which then result in many strang error, panic, kernel
> soft-lockup, rcu stall, etc.
> 
> In my vm, kernel doest not have commit cc29e1bf0d63f7 ("block: disable
> iopoll for split bio"), below fio job can reproduce this bug steadily:
> [global]
> name=iouring-sqpoll-iopoll-1
> ioengine=io_uring
> iodepth=128
> numjobs=1
> thread
> rw=randread
> direct=1
> registerfiles=1
> hipri=1
> bs=4m
> size=100M
> runtime=120
> time_based
> group_reporting
> randrepeat=0
> 
> [device]
> directory=/home/feiman.wxg/mntpoint/  # an ext4 mount point
> 
> If we have commit cc29e1bf0d63f7 ("block: disable iopoll for split bio"),
> there will no splitted bio case for polled io, but I think we still to need
> to fix this list corruption, it also should maybe go to stable branchs.
> 
> To fix this corruption, if a req is inserted into tail of io_wqe->work_list,
> initialize req->io_wq_work->list->next to bu NULL.

Applied, and marked for stable.

-- 
Jens Axboe


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

end of thread, other threads:[~2020-12-18 15:16 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-18  7:26 [PATCH] io_uring: fix io_wqe->work_list corruption Xiaoguang Wang
2020-12-18 15:10 ` Pavel Begunkov
2020-12-18 15:15 ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox