[PATCH 0/2] syzbot warning reports

[PATCH 0/2] syzbot warning reports

[Pavel Begunkov]

Two more false positive WARN_ON_ONCE() because of sqo_dead. For 2/2
issue, there is an easy test to trigger, so I assume it's a false
positive, but let's see if syzbot can hit it somehow else.

Pavel Begunkov (2):
  io_uring: fix false positive sqo warning on flush
  io_uring: fix uring_flush in exit_files() warning

 fs/io_uring.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.24.0

[PATCH 1/2] io_uring: fix false positive sqo warning on flush

[Pavel Begunkov]

WARNING: CPU: 1 PID: 9094 at fs/io_uring.c:8884
	io_disable_sqo_submit+0x106/0x130 fs/io_uring.c:8884
Call Trace:
 io_uring_flush+0x28b/0x3a0 fs/io_uring.c:9099
 filp_close+0xb4/0x170 fs/open.c:1280
 close_fd+0x5c/0x80 fs/file.c:626
 __do_sys_close fs/open.c:1299 [inline]
 __se_sys_close fs/open.c:1297 [inline]
 __x64_sys_close+0x2f/0xa0 fs/open.c:1297
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

io_uring's final close() may be triggered by any task not only the
creator. It's well handled by io_uring_flush() including SQPOLL case,
though a warning in io_disable_sqo_submit() will fallaciously fire by
moving this warning out to the only call site that matters.

Reported-by: syzbot+2f5d1785dc624932da78@syzkaller.appspotmail.com
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 06cc79d39586..9a67da50ae25 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8914,8 +8914,6 @@ static void __io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
 
 static void io_disable_sqo_submit(struct io_ring_ctx *ctx)
 {
-	WARN_ON_ONCE(ctx->sqo_task != current);
-
 	mutex_lock(&ctx->uring_lock);
 	ctx->sqo_dead = 1;
 	mutex_unlock(&ctx->uring_lock);
@@ -8937,6 +8935,7 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
 
 	if ((ctx->flags & IORING_SETUP_SQPOLL) && ctx->sq_data) {
 		/* for SQPOLL only sqo_task has task notes */
+		WARN_ON_ONCE(ctx->sqo_task != current);
 		io_disable_sqo_submit(ctx);
 		task = ctx->sq_data->thread;
 		atomic_inc(&task->io_uring->in_idle);
-- 
2.24.0

[PATCH 2/2] io_uring: fix uring_flush in exit_files() warning

[Pavel Begunkov]

WARNING: CPU: 1 PID: 11100 at fs/io_uring.c:9096
	io_uring_flush+0x326/0x3a0 fs/io_uring.c:9096
RIP: 0010:io_uring_flush+0x326/0x3a0 fs/io_uring.c:9096
Call Trace:
 filp_close+0xb4/0x170 fs/open.c:1280
 close_files fs/file.c:401 [inline]
 put_files_struct fs/file.c:416 [inline]
 put_files_struct+0x1cc/0x350 fs/file.c:413
 exit_files+0x7e/0xa0 fs/file.c:433
 do_exit+0xc22/0x2ae0 kernel/exit.c:820
 do_group_exit+0x125/0x310 kernel/exit.c:922
 get_signal+0x3e9/0x20a0 kernel/signal.c:2770
 arch_do_signal_or_restart+0x2a8/0x1eb0 arch/x86/kernel/signal.c:811
 handle_signal_work kernel/entry/common.c:147 [inline]
 exit_to_user_mode_loop kernel/entry/common.c:171 [inline]
 exit_to_user_mode_prepare+0x148/0x250 kernel/entry/common.c:201
 __syscall_exit_to_user_mode_work kernel/entry/common.c:291 [inline]
 syscall_exit_to_user_mode+0x19/0x50 kernel/entry/common.c:302
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

An SQPOLL ring creator task may have gotten rid of its file note during
exit and called io_disable_sqo_submit(), but the io_uring is still left
referenced through fdtable, which will be put during close_files() and
cause a false positive warning.

First split the warning into two for more clarity when is hit, and the
add sqo_dead check to handle the described case.

Reported-by: syzbot+a32b546d58dde07875a1@syzkaller.appspotmail.com
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 9a67da50ae25..b32bdd159e85 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -9126,7 +9126,10 @@ static int io_uring_flush(struct file *file, void *data)
 
 	if (ctx->flags & IORING_SETUP_SQPOLL) {
 		/* there is only one file note, which is owned by sqo_task */
-		WARN_ON_ONCE((ctx->sqo_task == current) ==
+		WARN_ON_ONCE(ctx->sqo_task != current &&
+			     xa_load(&tctx->xa, (unsigned long)file));
+		/* sqo_dead check is for when this happens after cancellation */
+		WARN_ON_ONCE(ctx->sqo_task == current && !ctx->sqo_dead &&
 			     !xa_load(&tctx->xa, (unsigned long)file));
 
 		io_disable_sqo_submit(ctx);
-- 
2.24.0

Re: [PATCH 0/2] syzbot warning reports

[Jens Axboe]

On 1/15/21 10:32 PM, Pavel Begunkov wrote:
> Two more false positive WARN_ON_ONCE() because of sqo_dead. For 2/2
> issue, there is an easy test to trigger, so I assume it's a false
> positive, but let's see if syzbot can hit it somehow else.

I'll apply these for now, also makes it easier to point syzbot at
a branch.

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: fix uring_flush in exit_files() warning

[Pavel Begunkov]

On 17/01/2021 02:31, Hillf Danton wrote:
> On Sat, 16 Jan 2021 05:32:30 +0000 Pavel Begunkov wrote:
>>
>> @@ -9126,7 +9126,10 @@ static int io_uring_flush(struct file *file, void *data)
>>  
>>  	if (ctx->flags & IORING_SETUP_SQPOLL) {
>>  		/* there is only one file note, which is owned by sqo_task */
>> -		WARN_ON_ONCE((ctx->sqo_task == current) ==
>> +		WARN_ON_ONCE(ctx->sqo_task != current &&
>> +			     xa_load(&tctx->xa, (unsigned long)file));
>> +		/* sqo_dead check is for when this happens after cancellation */
>> +		WARN_ON_ONCE(ctx->sqo_task == current && !ctx->sqo_dead &&
>>  			     !xa_load(&tctx->xa, (unsigned long)file));
>>  
>>  		io_disable_sqo_submit(ctx);
> 
> The added sqo_dead flag can not only quiesce a warning but save a
> disabling dryrun.

Don't think I get the sentence. Do you see any issue?

sqo_dead has a practical meaning, it prevents SQPOLL task from poking
into the creator task when it's racy. But yes, also in some cases makes
draining and killing rings nicer. 

-- 
Pavel Begunkov