[PATCH] io_uring: fix io_wqe->work_list corruption

[PATCH] io_uring: fix io_wqe->work_list corruption

[Xiaoguang Wang]

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 <xiaoguang.wang@linux.alibaba.com>
---
 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

Re: [PATCH] io_uring: fix io_wqe->work_list corruption

[Pavel Begunkov]

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 <asml.silence@gmail.com>

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix io_wqe->work_list corruption

[Jens Axboe]

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