| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <aW-DHlpo4dzKRB8K@studio.local>
Date: Tue, 20 Jan 2026 22:46:04 +0800
From: "Coly Li" <colyli@...as.com>
To: "Stephen Zhang" <starzhangzsd@...il.com>
Cc: "Kent Overstreet" <kent.overstreet@...ux.dev>, <axboe@...nel.dk>,
"Sasha Levin" <sashal@...nel.org>,
"Christoph Hellwig" <hch@...radead.org>, <linux-bcache@...r.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
"zhangshida" <zhangshida@...inos.cn>
Subject: Re: Fwd: [PATCH v2] bcache: use bio cloning for detached device requests
On Tue, Jan 20, 2026 at 10:39:36AM +0800, Stephen Zhang wrote:
> ---------- Forwarded message ---------
> 发件人： zhangshida <starzhangzsd@...il.com>
> Date: 2026年1月20日周二 10:35
> Subject: [PATCH v2] bcache: use bio cloning for detached device requests
> To: <colyli@...as.com>, <kent.overstreet@...ux.dev>,
> <axboe@...nel.dk>, <sashal@...nel.org>, <hch@...radead.org>
> Cc: <linux-bcache@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
> <zhangshida@...inos.cn>, <starzhangzsd@...il.com>, Christoph Hellwig
> <hch@....de>
>
>
> From: Shida Zhang <zhangshida@...inos.cn>
>
> Previously, bcache hijacked the bi_end_io and bi_private fields of
> the incoming bio when the backing device was in a detached state.
> This is fragile and breaks if the bio is needed to be processed by
> other layers.
>
> This patch transitions to using a cloned bio embedded within a private
> structure. This ensures the original bio's metadata remains untouched.
>
> Fixes: 53280e398471 ("bcache: fix improper use of bi_end_io")
> Co-developed-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Shida Zhang <zhangshida@...inos.cn>
> ---
>
> Changelog:
> v1:
> https://lore.kernel.org/all/20260115074811.230807-1-zhangshida@kylinos.cn/
>
> drivers/md/bcache/bcache.h | 9 +++++
> drivers/md/bcache/request.c | 79 ++++++++++++++++---------------------
> drivers/md/bcache/super.c | 12 +++++-
> 3 files changed, 54 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 8ccacba8554..54ff4e0238a 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -273,6 +273,8 @@ struct bcache_device {
>
> struct bio_set bio_split;
>
> + struct bio_set bio_detach;
^^^^^^^^^-> better to rename it to bio_detached
> unsigned int data_csum:1;
>
> int (*cache_miss)(struct btree *b, struct search *s,
> @@ -753,6 +755,13 @@ struct bbio {
> struct bio bio;
> };
>
> +struct detached_dev_io_private {
> + struct bcache_device *d;
> + unsigned long start_time;
> + struct bio *orig_bio;
> + struct bio bio;
> +};
> +
> #define BTREE_PRIO USHRT_MAX
> #define INITIAL_PRIO 32768U
>
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 82fdea7dea7..e0b12cb622b 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -1077,68 +1077,58 @@ static CLOSURE_CALLBACK(cached_dev_nodata)
> continue_at(cl, cached_dev_bio_complete, NULL);
> }
>
> -struct detached_dev_io_private {
> - struct bcache_device *d;
> - unsigned long start_time;
> - bio_end_io_t *bi_end_io;
> - void *bi_private;
> - struct block_device *orig_bdev;
> -};
> -
> static void detached_dev_end_io(struct bio *bio)
> {
> - struct detached_dev_io_private *ddip;
> -
> - ddip = bio->bi_private;
> - bio->bi_end_io = ddip->bi_end_io;
> - bio->bi_private = ddip->bi_private;
> + struct detached_dev_io_private *ddip =
> + container_of(bio, struct detached_dev_io_private, bio);
> + struct bio *orig_bio = ddip->orig_bio;
>
> /* Count on the bcache device */
> - bio_end_io_acct_remapped(bio, ddip->start_time, ddip->orig_bdev);
> + bio_end_io_acct(orig_bio, ddip->start_time);
>
> if (bio->bi_status) {
> - struct cached_dev *dc = container_of(ddip->d,
> - struct cached_dev, disk);
> + struct cached_dev *dc = bio->bi_private;
> +
> /* should count I/O error for backing device here */
> bch_count_backing_io_errors(dc, bio);
> + orig_bio->bi_status = bio->bi_status;
> }
>
> - kfree(ddip);
> - bio_endio(bio);
> + bio_put(bio);
> + bio_endio(orig_bio);
> }
>
> -static void detached_dev_do_request(struct bcache_device *d, struct bio *bio,
> - struct block_device *orig_bdev, unsigned long start_time)
> +static void detached_dev_do_request(struct bcache_device *d,
> + struct bio *orig_bio, unsigned long start_time)
> {
> struct detached_dev_io_private *ddip;
> struct cached_dev *dc = container_of(d, struct cached_dev, disk);
> + struct bio *clone_bio;
>
> - /*
> - * no need to call closure_get(&dc->disk.cl),
> - * because upper layer had already opened bcache device,
> - * which would call closure_get(&dc->disk.cl)
> - */
> - ddip = kzalloc(sizeof(struct detached_dev_io_private), GFP_NOIO);
> - if (!ddip) {
> - bio->bi_status = BLK_STS_RESOURCE;
> - bio_endio(bio);
> + if (bio_op(orig_bio) == REQ_OP_DISCARD &&
> + !bdev_max_discard_sectors(dc->bdev)) {
> + bio_endio(orig_bio);
> return;
> }
>
> - ddip->d = d;
> + clone_bio = bio_alloc_clone(dc->bdev, orig_bio, GFP_NOIO,
> + &d->bio_detach);
> + if (!clone_bio) {
> + orig_bio->bi_status = BLK_STS_RESOURCE;
> + bio_endio(orig_bio);
> + return;
> + }
> +
> + ddip = container_of(clone_bio, struct detached_dev_io_private, bio);
> /* Count on the bcache device */
> - ddip->orig_bdev = orig_bdev;
> + ddip->d = d;
> ddip->start_time = start_time;
> - ddip->bi_end_io = bio->bi_end_io;
> - ddip->bi_private = bio->bi_private;
> - bio->bi_end_io = detached_dev_end_io;
> - bio->bi_private = ddip;
> -
> - if ((bio_op(bio) == REQ_OP_DISCARD) &&
> - !bdev_max_discard_sectors(dc->bdev))
> - detached_dev_end_io(bio);
> - else
> - submit_bio_noacct(bio);
> + ddip->orig_bio = orig_bio;
> +
> + clone_bio->bi_end_io = detached_dev_end_io;
> + clone_bio->bi_private = dc;
> +
> + submit_bio_noacct(clone_bio);
> }
>
> static void quit_max_writeback_rate(struct cache_set *c,
> @@ -1214,10 +1204,10 @@ void cached_dev_submit_bio(struct bio *bio)
>
> start_time = bio_start_io_acct(bio);
>
> - bio_set_dev(bio, dc->bdev);
> bio->bi_iter.bi_sector += dc->sb.data_offset;
>
> if (cached_dev_get(dc)) {
> + bio_set_dev(bio, dc->bdev);
> s = search_alloc(bio, d, orig_bdev, start_time);
> trace_bcache_request_start(s->d, bio);
>
> @@ -1237,9 +1227,10 @@ void cached_dev_submit_bio(struct bio *bio)
> else
> cached_dev_read(dc, s);
> }
> - } else
> + } else {
> /* I/O request sent to backing device */
> - detached_dev_do_request(d, bio, orig_bdev, start_time);
> + detached_dev_do_request(d, bio, start_time);
> + }
> }
>
> static int cached_dev_ioctl(struct bcache_device *d, blk_mode_t mode,
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index c17d4517af2..d4b798668c8 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -887,6 +887,7 @@ static void bcache_device_free(struct bcache_device *d)
> }
>
> bioset_exit(&d->bio_split);
> + bioset_exit(&d->bio_detach);
> kvfree(d->full_dirty_stripes);
> kvfree(d->stripe_sectors_dirty);
>
> @@ -949,6 +950,11 @@ static int bcache_device_init(struct
> bcache_device *d, unsigned int block_size,
> BIOSET_NEED_BVECS|BIOSET_NEED_RESCUER))
> goto out_ida_remove;
>
> + if (bioset_init(&d->bio_detach, 4,
^^^^^-> I feel 4 might be a bit small
here. bio_detached set is for normal IO when backing device is not attached to
a cache device. I would suggest to set the pool size to 128 or 256.
> + offsetof(struct detached_dev_io_private, bio),
> + BIOSET_NEED_BVECS|BIOSET_NEED_RESCUER))
> + goto out_bioset_split_exit;
> +
> if (lim.logical_block_size > PAGE_SIZE && cached_bdev) {
> /*
> * This should only happen with BCACHE_SB_VERSION_BDEV.
> @@ -964,7 +970,7 @@ static int bcache_device_init(struct bcache_device
> *d, unsigned int block_size,
>
> d->disk = blk_alloc_disk(&lim, NUMA_NO_NODE);
> if (IS_ERR(d->disk))
> - goto out_bioset_exit;
> + goto out_bioset_detach_exit;
>
> set_capacity(d->disk, sectors);
> snprintf(d->disk->disk_name, DISK_NAME_LEN, "bcache%i", idx);
> @@ -976,7 +982,9 @@ static int bcache_device_init(struct bcache_device
> *d, unsigned int block_size,
> d->disk->private_data = d;
> return 0;
>
> -out_bioset_exit:
> +out_bioset_detach_exit:
> + bioset_exit(&d->bio_detach);
> +out_bioset_split_exit:
> bioset_exit(&d->bio_split);
> out_ida_remove:
> ida_free(&bcache_device_idx, idx);
> --
Rested part is good to me. Thanks for patching up.
Coly Li
Powered by blists - more mailing lists