From: Sven Schnelle <svens@linux.ibm.com>
To: Christoph Hellwig <hch@lst.de>
Cc: "Jens Axboe" <axboe@kernel.dk>,
"Stefan Haberland" <sth@linux.ibm.com>,
"Jan Hoeppner" <hoeppner@linux.ibm.com>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
"Doug Gilbert" <dgilbert@interlog.com>,
"Kai Mäkisara" <Kai.Makisara@kolumbus.fi>,
"Luis Chamberlain" <mcgrof@kernel.org>,
linux-block@vger.kernel.org, linux-nvme@lists.infradead.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org
Subject: Re: [PATCH 9/9] block: add an explicit ->disk backpointer to the request_queue
Date: Thu, 19 Aug 2021 16:39:08 +0200 [thread overview]
Message-ID: <yt9deeaph52r.fsf@linux.ibm.com> (raw)
In-Reply-To: <20210816134624.GA24234@lst.de> (Christoph Hellwig's message of "Mon, 16 Aug 2021 15:46:24 +0200")
Christoph Hellwig <hch@lst.de> writes:
> Replace the magic lookup through the kobject tree with an explicit
> backpointer, given that the device model links are set up and torn
> down at times when I/O is still possible, leading to potential
> NULL or invalid pointer dereferences.
>
> Fixes: edb0872f44ec ("block: move the bdi from the request_queue to the gendisk")
> Reported-by: syzbot <syzbot+aa0801b6b32dca9dda82@syzkaller.appspotmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/bfq-iosched.c | 2 +-
> block/blk-cgroup.c | 4 ++--
> block/blk-mq.c | 2 +-
> block/blk-settings.c | 8 ++++----
> block/blk-sysfs.c | 13 ++++++-------
> block/blk-wbt.c | 10 +++++-----
> block/genhd.c | 2 ++
> include/linux/blkdev.h | 5 ++---
> include/trace/events/kyber.h | 6 +++---
> 9 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index e4a61eda2d0f..6ccab91a6336 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -5269,7 +5269,7 @@ bfq_set_next_ioprio_data(struct bfq_queue *bfqq, struct bfq_io_cq *bic)
> switch (ioprio_class) {
> default:
> pr_err("bdi %s: bfq: bad prio class %d\n",
> - bdi_dev_name(queue_to_disk(bfqq->bfqd->queue)->bdi),
> + bdi_dev_name(bfqq->bfqd->queue->disk->bdi),
> ioprio_class);
> fallthrough;
> case IOPRIO_CLASS_NONE:
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index db034e35ae20..a032536a298d 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -489,9 +489,9 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css,
>
> const char *blkg_dev_name(struct blkcg_gq *blkg)
> {
> - if (!queue_has_disk(blkg->q) || !queue_to_disk(blkg->q)->bdi->dev)
> + if (!blkg->q->disk || !blkg->q->disk->bdi->dev)
> return NULL;
> - return bdi_dev_name(queue_to_disk(blkg->q)->bdi);
> + return bdi_dev_name(blkg->q->disk->bdi);
> }
>
> /**
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 2ca7e7c94b18..0a33d16a7298 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -525,7 +525,7 @@ void blk_mq_free_request(struct request *rq)
> __blk_mq_dec_active_requests(hctx);
>
> if (unlikely(laptop_mode && !blk_rq_is_passthrough(rq)))
> - laptop_io_completion(queue_to_disk(q)->bdi);
> + laptop_io_completion(q->disk->bdi);
>
> rq_qos_done(q, rq);
>
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 3613d2cc0688..a7c857ad7d10 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -141,9 +141,9 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto
> limits->logical_block_size >> SECTOR_SHIFT);
> limits->max_sectors = max_sectors;
>
> - if (!queue_has_disk(q))
> + if (!q->disk)
> return;
> - queue_to_disk(q)->bdi->io_pages = max_sectors >> (PAGE_SHIFT - 9);
> + q->disk->bdi->io_pages = max_sectors >> (PAGE_SHIFT - 9);
> }
> EXPORT_SYMBOL(blk_queue_max_hw_sectors);
>
> @@ -475,9 +475,9 @@ EXPORT_SYMBOL(blk_limits_io_opt);
> void blk_queue_io_opt(struct request_queue *q, unsigned int opt)
> {
> blk_limits_io_opt(&q->limits, opt);
> - if (!queue_has_disk(q))
> + if (!q->disk)
> return;
> - queue_to_disk(q)->bdi->ra_pages =
> + q->disk->bdi->ra_pages =
> max(queue_io_opt(q) * 2 / PAGE_SIZE, VM_READAHEAD_PAGES);
> }
> EXPORT_SYMBOL(blk_queue_io_opt);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 586507a5b8c2..7fd99487300c 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -90,9 +90,9 @@ static ssize_t queue_ra_show(struct request_queue *q, char *page)
> {
> unsigned long ra_kb;
>
> - if (!queue_has_disk(q))
> + if (!q->disk)
> return -EINVAL;
> - ra_kb = queue_to_disk(q)->bdi->ra_pages << (PAGE_SHIFT - 10);
> + ra_kb = q->disk->bdi->ra_pages << (PAGE_SHIFT - 10);
> return queue_var_show(ra_kb, page);
> }
>
> @@ -102,12 +102,12 @@ queue_ra_store(struct request_queue *q, const char *page, size_t count)
> unsigned long ra_kb;
> ssize_t ret;
>
> - if (!queue_has_disk(q))
> + if (!q->disk)
> return -EINVAL;
> ret = queue_var_store(&ra_kb, page, count);
> if (ret < 0)
> return ret;
> - queue_to_disk(q)->bdi->ra_pages = ra_kb >> (PAGE_SHIFT - 10);
> + q->disk->bdi->ra_pages = ra_kb >> (PAGE_SHIFT - 10);
> return ret;
> }
>
> @@ -254,9 +254,8 @@ queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
>
> spin_lock_irq(&q->queue_lock);
> q->limits.max_sectors = max_sectors_kb << 1;
> - if (queue_has_disk(q))
> - queue_to_disk(q)->bdi->io_pages =
> - max_sectors_kb >> (PAGE_SHIFT - 10);
> + if (q->disk)
> + q->disk->bdi->io_pages = max_sectors_kb >> (PAGE_SHIFT - 10);
> spin_unlock_irq(&q->queue_lock);
>
> return ret;
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index 31086afaad9c..874c1c37bf0c 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -97,7 +97,7 @@ static void wb_timestamp(struct rq_wb *rwb, unsigned long *var)
> */
> static bool wb_recent_wait(struct rq_wb *rwb)
> {
> - struct bdi_writeback *wb = &queue_to_disk(rwb->rqos.q)->bdi->wb;
> + struct bdi_writeback *wb = &rwb->rqos.q->disk->bdi->wb;
>
> return time_before(jiffies, wb->dirty_sleep + HZ);
> }
> @@ -234,7 +234,7 @@ enum {
>
> static int latency_exceeded(struct rq_wb *rwb, struct blk_rq_stat *stat)
> {
> - struct backing_dev_info *bdi = queue_to_disk(rwb->rqos.q)->bdi;
> + struct backing_dev_info *bdi = rwb->rqos.q->disk->bdi;
> struct rq_depth *rqd = &rwb->rq_depth;
> u64 thislat;
>
> @@ -287,7 +287,7 @@ static int latency_exceeded(struct rq_wb *rwb, struct blk_rq_stat *stat)
>
> static void rwb_trace_step(struct rq_wb *rwb, const char *msg)
> {
> - struct backing_dev_info *bdi = queue_to_disk(rwb->rqos.q)->bdi;
> + struct backing_dev_info *bdi = rwb->rqos.q->disk->bdi;
> struct rq_depth *rqd = &rwb->rq_depth;
>
> trace_wbt_step(bdi, msg, rqd->scale_step, rwb->cur_win_nsec,
> @@ -359,8 +359,8 @@ static void wb_timer_fn(struct blk_stat_callback *cb)
>
> status = latency_exceeded(rwb, cb->stat);
>
> - trace_wbt_timer(queue_to_disk(rwb->rqos.q)->bdi, status,
> - rqd->scale_step, inflight);
> + trace_wbt_timer(rwb->rqos.q->disk->bdi, status, rqd->scale_step,
> + inflight);
>
> /*
> * If we exceeded the latency target, step down. If we did not,
> diff --git a/block/genhd.c b/block/genhd.c
> index 6294517cebe6..02cd9ec93e52 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1078,6 +1078,7 @@ static void disk_release(struct device *dev)
> disk_release_events(disk);
> kfree(disk->random);
> xa_destroy(&disk->part_tbl);
> + disk->queue->disk = NULL;
> blk_put_queue(disk->queue);
> iput(disk->part0->bd_inode); /* frees the disk */
> }
> @@ -1276,6 +1277,7 @@ struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
> device_initialize(disk_to_dev(disk));
> inc_diskseq(disk);
> disk->queue = q;
> + q->disk = disk;
> lockdep_init_map(&disk->lockdep_map, "(bio completion)", lkclass, 0);
> #ifdef CONFIG_BLOCK_HOLDER_DEPRECATED
> INIT_LIST_HEAD(&disk->slave_bdevs);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index df404c1fb087..22b5b8502d2a 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -421,6 +421,8 @@ struct request_queue {
>
> spinlock_t queue_lock;
>
> + struct gendisk *disk;
> +
> /*
> * queue kobject
> */
> @@ -661,9 +663,6 @@ extern void blk_clear_pm_only(struct request_queue *q);
> dma_map_page_attrs(dev, (bv)->bv_page, (bv)->bv_offset, (bv)->bv_len, \
> (dir), (attrs))
>
> -#define queue_has_disk(q) ((q)->kobj.parent != NULL)
> -#define queue_to_disk(q) (dev_to_disk(kobj_to_dev((q)->kobj.parent)))
> -
> static inline bool queue_is_mq(struct request_queue *q)
> {
> return q->mq_ops;
> diff --git a/include/trace/events/kyber.h b/include/trace/events/kyber.h
> index f9802562edf6..491098a0d8ed 100644
> --- a/include/trace/events/kyber.h
> +++ b/include/trace/events/kyber.h
> @@ -30,7 +30,7 @@ TRACE_EVENT(kyber_latency,
> ),
>
> TP_fast_assign(
> - __entry->dev = disk_devt(queue_to_disk(q));
> + __entry->dev = disk_devt(q->disk);
> strlcpy(__entry->domain, domain, sizeof(__entry->domain));
> strlcpy(__entry->type, type, sizeof(__entry->type));
> __entry->percentile = percentile;
> @@ -59,7 +59,7 @@ TRACE_EVENT(kyber_adjust,
> ),
>
> TP_fast_assign(
> - __entry->dev = disk_devt(queue_to_disk(q));
> + __entry->dev = disk_devt(q->disk);
> strlcpy(__entry->domain, domain, sizeof(__entry->domain));
> __entry->depth = depth;
> ),
> @@ -81,7 +81,7 @@ TRACE_EVENT(kyber_throttled,
> ),
>
> TP_fast_assign(
> - __entry->dev = disk_devt(queue_to_disk(q));
> + __entry->dev = disk_devt(q->disk);
> strlcpy(__entry->domain, domain, sizeof(__entry->domain));
> ),
Tested-by: Sven Schnelle <svens@linux.ibm.com>
next prev parent reply other threads:[~2021-08-19 14:39 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-16 13:19 ensure each gendisk always has a request_queue reference v2 Christoph Hellwig
2021-08-16 13:19 ` [PATCH 1/9] nvme: use blk_mq_alloc_disk Christoph Hellwig
2021-08-19 14:54 ` Keith Busch
2021-08-19 14:58 ` Keith Busch
2021-08-19 22:57 ` Luis Chamberlain
2021-08-20 4:06 ` Christoph Hellwig
2021-08-23 17:22 ` Sagi Grimberg
2021-08-16 13:19 ` [PATCH 2/9] st: do not allocate a gendisk Christoph Hellwig
2021-08-16 13:19 ` [PATCH 3/9] sg: " Christoph Hellwig
2021-08-19 21:33 ` Luis Chamberlain
2021-08-19 21:36 ` Luis Chamberlain
2021-08-20 4:05 ` Christoph Hellwig
2021-08-16 13:19 ` [PATCH 4/9] block: cleanup the lockdep handling in *alloc_disk Christoph Hellwig
2021-08-16 13:19 ` [PATCH 5/9] block: remove alloc_disk and alloc_disk_node Christoph Hellwig
2021-08-16 13:19 ` [PATCH 6/9] block: remove the minors argument to __alloc_disk_node Christoph Hellwig
2021-08-16 13:19 ` [PATCH 7/9] block: pass a request_queue to __blk_alloc_disk Christoph Hellwig
2021-08-16 13:19 ` [PATCH 8/9] block: hold a request_queue reference for the lifetime of struct gendisk Christoph Hellwig
2021-08-20 13:42 ` Ming Lei
2021-08-16 13:46 ` [PATCH 9/9] block: add an explicit ->disk backpointer to the request_queue Christoph Hellwig
2021-08-19 14:39 ` Sven Schnelle [this message]
2021-08-23 18:55 ` ensure each gendisk always has a request_queue reference v2 Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=yt9deeaph52r.fsf@linux.ibm.com \
--to=svens@linux.ibm.com \
--cc=Kai.Makisara@kolumbus.fi \
--cc=axboe@kernel.dk \
--cc=dgilbert@interlog.com \
--cc=hch@lst.de \
--cc=hoeppner@linux.ibm.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=mcgrof@kernel.org \
--cc=sth@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).