Re: [syzbot] [block?] WARNING in blk_mq_start_request

Re: [syzbot] [block?] WARNING in blk_mq_start_request

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+fcc47ba2476570cbbeb0@syzkaller.appspotmail.com

Tested on:

commit:         4652b8e4 Merge tag '6.7-rc-ksmbd-server-fixes' of git:..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=1629a00f680000
kernel config:  https://syzkaller.appspot.com/x/.config?x=5316ad879647e3c5
dashboard link: https://syzkaller.appspot.com/bug?extid=fcc47ba2476570cbbeb0
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=107e3ac0e80000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] [PATCH] Test for 4652b8e4f3ff

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: [PATCH] Test for 4652b8e4f3ff
Author: eadavis@qq.com

please test warning in blk_mq_start_request

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4652b8e4f3ff

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 22a3cf7f32e2..0726534a5a24 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1724,6 +1724,8 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
 	cmd->fake_timeout = should_timeout_request(rq) ||
 		blk_should_fake_timeout(rq->q);
 
+	if (READ_ONCE(rq->state))
+		WRITE_ONCE(rq->state, MQ_RQ_IDLE);
 	blk_mq_start_request(rq);
 
 	if (should_requeue_request(rq)) {

Re: [syzbot] [PATCH] Test for 4652b8e4f3ff

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: [PATCH] Test for 4652b8e4f3ff
Author: eadavis@qq.com

please test warning in blk_mq_start_request

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4652b8e4f3ff

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e2d11183f62e..26f2921cbc50 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2575,6 +2575,8 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
 	 * Any other error (busy), just add it to our list as we
 	 * previously would have done.
 	 */
+	if (READ_ONCE(rq->state))
+		WRITE_ONCE(rq->state, MQ_RQ_IDLE);
 	ret = q->mq_ops->queue_rq(hctx, &bd);
 	switch (ret) {
 	case BLK_STS_OK:

Re: [syzbot] [block?] WARNING in blk_mq_start_request

[syzbot]

syzbot has bisected this issue to:

commit d78bfa1346ab1fe04d20aa45a0678d1fc866f37c
Author: Chengming Zhou <zhouchengming@bytedance.com>
Date:   Wed Sep 13 15:16:16 2023 +0000

    block/null_blk: add queue_rqs() support

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=106414a8e80000
start commit:   13d88ac54ddd Merge tag 'vfs-6.7.fsid' of git://git.kernel...
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=126414a8e80000
console output: https://syzkaller.appspot.com/x/log.txt?x=146414a8e80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=fcc47ba2476570cbbeb0
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1465bb08e80000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=13e7881f680000

Reported-by: syzbot+fcc47ba2476570cbbeb0@syzkaller.appspotmail.com
Fixes: d78bfa1346ab ("block/null_blk: add queue_rqs() support")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

Re: [syzbot] [block?] WARNING in blk_mq_start_request

[Chengming Zhou]

On 2023/11/9 05:18, syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit d78bfa1346ab1fe04d20aa45a0678d1fc866f37c
> Author: Chengming Zhou <zhouchengming@bytedance.com>
> Date:   Wed Sep 13 15:16:16 2023 +0000
> 
>     block/null_blk: add queue_rqs() support
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=106414a8e80000
> start commit:   13d88ac54ddd Merge tag 'vfs-6.7.fsid' of git://git.kernel...
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=126414a8e80000
> console output: https://syzkaller.appspot.com/x/log.txt?x=146414a8e80000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
> dashboard link: https://syzkaller.appspot.com/bug?extid=fcc47ba2476570cbbeb0
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1465bb08e80000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=13e7881f680000
> 
> Reported-by: syzbot+fcc47ba2476570cbbeb0@syzkaller.appspotmail.com
> Fixes: d78bfa1346ab ("block/null_blk: add queue_rqs() support")
> 
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION is enabled in the kernel config,
so null_queue_rq() will return BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE
for some requests, which have been marked as IN_FLIGHT status.

Then null_queue_rqs() put these requests in the rqlist and return back,
blk-mq will try to queue them individually once again, caused the warning
"WARN_ON_ONCE(blk_mq_rq_state(rq) != MQ_RQ_IDLE)" in blk_mq_start_request().

So handling of return value of null_queue_rq() in null_queue_rqs() is wrong,
maybe we should __blk_mq_requeue_request() for these requests, before
adding them in the rqlist?

Thanks!

Re: [syzbot] [block?] WARNING in blk_mq_start_request

[Bart Van Assche]

On 11/8/23 17:27, Chengming Zhou wrote:
> CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION is enabled in the kernel config,
> so null_queue_rq() will return BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE
> for some requests, which have been marked as IN_FLIGHT status.
> 
> Then null_queue_rqs() put these requests in the rqlist and return back,
> blk-mq will try to queue them individually once again, caused the warning
> "WARN_ON_ONCE(blk_mq_rq_state(rq) != MQ_RQ_IDLE)" in blk_mq_start_request().
> 
> So handling of return value of null_queue_rq() in null_queue_rqs() is wrong,
> maybe we should __blk_mq_requeue_request() for these requests, before
> adding them in the rqlist?

Please follow the example of virtio_queue_rqs() and send any requests
that need to be requeued back to the block layer core instead of
handling these directly in null_queue_rqs().

Thanks,

Bart.

Re: [External] Re: [syzbot] [block?] WARNING in blk_mq_start_request

[Chengming Zhou]

On 2023/11/10 02:13, Bart Van Assche wrote:
> On 11/8/23 17:27, Chengming Zhou wrote:
>> CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION is enabled in the kernel config,
>> so null_queue_rq() will return BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE
>> for some requests, which have been marked as IN_FLIGHT status.
>>
>> Then null_queue_rqs() put these requests in the rqlist and return back,
>> blk-mq will try to queue them individually once again, caused the warning
>> "WARN_ON_ONCE(blk_mq_rq_state(rq) != MQ_RQ_IDLE)" in blk_mq_start_request().
>>
>> So handling of return value of null_queue_rq() in null_queue_rqs() is wrong,
>> maybe we should __blk_mq_requeue_request() for these requests, before
>> adding them in the rqlist?
> 
> Please follow the example of virtio_queue_rqs() and send any requests
> that need to be requeued back to the block layer core instead of
> handling these directly in null_queue_rqs().
> 

Ok, I reviewed the code of virtio_queue_rqs(), found the main difference
is that request won't fail after blk_mq_start_request().

But in null_blk case, the request will fail after blk_mq_start_request(),
return BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE. If we return these rqs
back to the block layer core, they will be queued individually once again.
So caused the warning.

Thanks!

Re: [External] Re: [syzbot] [block?] WARNING in blk_mq_start_request

[Bart Van Assche]

On 11/13/23 07:05, Chengming Zhou wrote:
> Ok, I reviewed the code of virtio_queue_rqs(), found the main difference
> is that request won't fail after blk_mq_start_request().
> 
> But in null_blk case, the request will fail after blk_mq_start_request(),
> return BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE. If we return these rqs
> back to the block layer core, they will be queued individually once again.
> So caused the warning.

I think it is safe to move the blk_mq_start_request() call under the if-block
that decides whether or not to requeue a request in null_queue_rq()

Thanks,

Bart.

Re: [External] Re: [syzbot] [block?] WARNING in blk_mq_start_request

[Chengming Zhou]

On 2023/11/14 07:57, Bart Van Assche wrote:
> On 11/13/23 07:05, Chengming Zhou wrote:
>> Ok, I reviewed the code of virtio_queue_rqs(), found the main difference
>> is that request won't fail after blk_mq_start_request().
>>
>> But in null_blk case, the request will fail after blk_mq_start_request(),
>> return BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE. If we return these rqs
>> back to the block layer core, they will be queued individually once again.
>> So caused the warning.
> 
> I think it is safe to move the blk_mq_start_request() call under the if-block
> that decides whether or not to requeue a request in null_queue_rq()
> 

Right! And null_handle_throttled() in null_handle_cmd() may also return the
BLK_STS_DEV_RESOURCE, it's also needed to put in null_queue_rq() and before
the blk_mq_start_request().

Then request must return BLK_STS_OK after blk_mq_start_request().

Thanks!