[PATCH] blk-mq: move lockdep_assert_held() into elevator_exit

[PATCH] blk-mq: move lockdep_assert_held() into elevator_exit

[Ming Lei]

Commit c48dac137a62 ("block: don't hold q->sysfs_lock in elevator_init_mq")
removes q->sysfs_lock from elevator_init_mq(), but forgot to deal with
lockdep_assert_held() called in blk_mq_sched_free_requests() which is
run in failure path of elevator_init_mq().

blk_mq_sched_free_requests() is called in the following 3 functions:

	elevator_init_mq()
	elevator_exit()
	blk_cleanup_queue()

In blk_cleanup_queue(), blk_mq_sched_free_requests() is followed exactly
by 'mutex_lock(&q->sysfs_lock)'.

So moving the lockdep_assert_held() from blk_mq_sched_free_requests()
into elevator_exit() for fixing the report by syzbot.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <Damien.LeMoal@wdc.com>
Reported-by: syzbot+da3b7677bb913dc1b737@syzkaller.appspotmail.com
Fixed: c48dac137a62 ("block: don't hold q->sysfs_lock in elevator_init_mq")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-sched.c | 2 --
 block/blk.h          | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index c9d183d6c499..ca22afd47b3d 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -555,8 +555,6 @@ void blk_mq_sched_free_requests(struct request_queue *q)
 	struct blk_mq_hw_ctx *hctx;
 	int i;
 
-	lockdep_assert_held(&q->sysfs_lock);
-
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (hctx->sched_tags)
 			blk_mq_free_rqs(q->tag_set, hctx->sched_tags, i);
diff --git a/block/blk.h b/block/blk.h
index ed347f7a97b1..25773d668ec0 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -194,6 +194,8 @@ void elv_unregister_queue(struct request_queue *q);
 static inline void elevator_exit(struct request_queue *q,
 		struct elevator_queue *e)
 {
+	lockdep_assert_held(&q->sysfs_lock);
+
 	blk_mq_sched_free_requests(q);
 	__elevator_exit(q, e);
 }
-- 
2.20.1

Re: [PATCH] blk-mq: move lockdep_assert_held() into elevator_exit

[Bart Van Assche]

On 9/25/19 3:23 PM, Ming Lei wrote:
> Commit c48dac137a62 ("block: don't hold q->sysfs_lock in elevator_init_mq")
> removes q->sysfs_lock from elevator_init_mq(), but forgot to deal with
> lockdep_assert_held() called in blk_mq_sched_free_requests() which is
> run in failure path of elevator_init_mq().
> 
> blk_mq_sched_free_requests() is called in the following 3 functions:
> 
> 	elevator_init_mq()
> 	elevator_exit()
> 	blk_cleanup_queue()
> 
> In blk_cleanup_queue(), blk_mq_sched_free_requests() is followed exactly
> by 'mutex_lock(&q->sysfs_lock)'.
> 
> So moving the lockdep_assert_held() from blk_mq_sched_free_requests()
> into elevator_exit() for fixing the report by syzbot.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] blk-mq: move lockdep_assert_held() into elevator_exit

[Damien Le Moal]

On 2019/09/25 15:24, Ming Lei wrote:
> Commit c48dac137a62 ("block: don't hold q->sysfs_lock in elevator_init_mq")
> removes q->sysfs_lock from elevator_init_mq(), but forgot to deal with
> lockdep_assert_held() called in blk_mq_sched_free_requests() which is
> run in failure path of elevator_init_mq().
> 
> blk_mq_sched_free_requests() is called in the following 3 functions:
> 
> 	elevator_init_mq()
> 	elevator_exit()
> 	blk_cleanup_queue()
> 
> In blk_cleanup_queue(), blk_mq_sched_free_requests() is followed exactly
> by 'mutex_lock(&q->sysfs_lock)'.
> 
> So moving the lockdep_assert_held() from blk_mq_sched_free_requests()
> into elevator_exit() for fixing the report by syzbot.
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>
> Reported-by: syzbot+da3b7677bb913dc1b737@syzkaller.appspotmail.com
> Fixed: c48dac137a62 ("block: don't hold q->sysfs_lock in elevator_init_mq")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-sched.c | 2 --
>  block/blk.h          | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index c9d183d6c499..ca22afd47b3d 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -555,8 +555,6 @@ void blk_mq_sched_free_requests(struct request_queue *q)
>  	struct blk_mq_hw_ctx *hctx;
>  	int i;
>  
> -	lockdep_assert_held(&q->sysfs_lock);
> -
>  	queue_for_each_hw_ctx(q, hctx, i) {
>  		if (hctx->sched_tags)
>  			blk_mq_free_rqs(q->tag_set, hctx->sched_tags, i);
> diff --git a/block/blk.h b/block/blk.h
> index ed347f7a97b1..25773d668ec0 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -194,6 +194,8 @@ void elv_unregister_queue(struct request_queue *q);
>  static inline void elevator_exit(struct request_queue *q,
>  		struct elevator_queue *e)
>  {
> +	lockdep_assert_held(&q->sysfs_lock);
> +
>  	blk_mq_sched_free_requests(q);
>  	__elevator_exit(q, e);
>  }
> 

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] blk-mq: move lockdep_assert_held() into elevator_exit

[Jens Axboe]

On 9/26/19 12:23 AM, Ming Lei wrote:
> Commit c48dac137a62 ("block: don't hold q->sysfs_lock in elevator_init_mq")
> removes q->sysfs_lock from elevator_init_mq(), but forgot to deal with
> lockdep_assert_held() called in blk_mq_sched_free_requests() which is
> run in failure path of elevator_init_mq().
> 
> blk_mq_sched_free_requests() is called in the following 3 functions:
> 
> 	elevator_init_mq()
> 	elevator_exit()
> 	blk_cleanup_queue()
> 
> In blk_cleanup_queue(), blk_mq_sched_free_requests() is followed exactly
> by 'mutex_lock(&q->sysfs_lock)'.
> 
> So moving the lockdep_assert_held() from blk_mq_sched_free_requests()
> into elevator_exit() for fixing the report by syzbot.

Applied, thanks Ming.

-- 
Jens Axboe