On Tue, May 31, 2022 at 06:05:35PM +0200, Christoph Hellwig wrote:
> The elevator is only used for file system requests, which are stopped in
> del_gendisk. Move disabling the elevator and freeing the scheduler tags
> to the end of del_gendisk instead of doing that work in disk_release and
> blk_cleanup_queue to avoid a use after free on q->tag_set from
> disk_release as the tag_set might not be alive at that point.
>
> Move the blk_qos_exit call as well, as it just depends on the elevator
> exit and would be the only reason to keep the not exactly cheap queue
> freeze in disk_release.
>
> Fixes: e155b0c238b2 ("blk-mq: Use shared tags for shared sbitmap support")
> Reported-by: syzbot+3e3f419f4a7816471838@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Tested-by: syzbot+3e3f419f4a7816471838@xxxxxxxxxxxxxxxxxxxxxxxxx
> ---
> block/blk-core.c | 13 -------------
> block/genhd.c | 38 ++++++++++----------------------------
> 2 files changed, 10 insertions(+), 41 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 80fa73c419a99..19cfa71e33728 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -322,19 +322,6 @@ void blk_cleanup_queue(struct request_queue *q)
> blk_mq_exit_queue(q);
> }
>
> - /*
> - * In theory, request pool of sched_tags belongs to request queue.
> - * However, the current implementation requires tag_set for freeing
> - * requests, so free the pool now.
> - *
> - * Queue has become frozen, there can't be any in-queue requests, so
> - * it is safe to free requests now.
> - */
> - mutex_lock(&q->sysfs_lock);
> - if (q->elevator)
> - blk_mq_sched_free_rqs(q);
> - mutex_unlock(&q->sysfs_lock);
> -
> /* @q is and will stay empty, shutdown and put */
> blk_put_queue(q);
> }
> diff --git a/block/genhd.c b/block/genhd.c
> index 8ff5b187791af..9914d0f24fecd 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -649,6 +649,16 @@ void del_gendisk(struct gendisk *disk)
>
> blk_sync_queue(q);
> blk_flush_integrity();
> + blk_mq_cancel_work_sync(q);
> +
> + if (q->elevator) {
> + mutex_lock(&q->sysfs_lock);
> + elevator_exit(q);
> + mutex_unlock(&q->sysfs_lock);
> + }
This way can't be safe, who can guarantee that all sync submission
activities are gone after queue is frozen? We had lots of reports on
blk_mq_sched_has_work() which triggers UAF.
Thanks,
Ming
