* Re: Fix potential data loss and corruption due to Incorrect BIO Chain Handling
[not found] ` <CAHc6FU7+riVQBX7L2uk64A355rF+DfQ6xhP425ruQ76d_SDPGA@mail.gmail.com>
@ 2025-11-23 3:14 ` Stephen Zhang
2025-11-23 13:48 ` Ming Lei
1 sibling, 0 replies; 4+ messages in thread
From: Stephen Zhang @ 2025-11-23 3:14 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Ming Lei, linux-kernel, linux-block, nvdimm, virtualization,
linux-nvme, gfs2, ntfs3, linux-xfs, zhangshida, linux-bcache
Andreas Gruenbacher <agruenba@redhat.com> 于2025年11月22日周六 22:57写道：
>
> On Sat, Nov 22, 2025 at 1:07 PM Ming Lei <ming.lei@redhat.com> wrote:
> > > static void bio_chain_endio(struct bio *bio)
> > > {
> > > bio_endio(__bio_chain_endio(bio));
> > > }
> >
> > bio_chain_endio() never gets called really, which can be thought as `flag`,
>
> That's probably where this stops being relevant for the problem
> reported by Stephen Zhang.
>
> > and it should have been defined as `WARN_ON_ONCE(1);` for not confusing people.
>
> But shouldn't bio_chain_endio() still be fixed to do the right thing
> if called directly, or alternatively, just BUG()? Warning and still
> doing the wrong thing seems a bit bizarre.
>
> I also see direct bi_end_io calls in erofs_fileio_ki_complete(),
> erofs_fscache_bio_endio(), and erofs_fscache_submit_bio(), so those
> are at least confusing.
>
> Thanks,
> Andreas
>
Thank you, Ming and Andreas, for the detailed explanation. I will
remember to add the `WARN()`/`BUG()` macros in `bio_chain_endio()`.
Following that discussion, I have identified a similar and suspicious
call in the
bcache driver.
Location:`drivers/md/bcache/request.c`
```c
static void detached_dev_do_request(struct bcache_device *d, struct bio *bio,
struct block_device *orig_bdev,
unsigned long start_time)
{
struct detached_dev_io_private *ddip;
struct cached_dev *dc = container_of(d, struct cached_dev, disk);
/*
* 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->bi_end_io(bio); // <-- POTENTIAL ISSUE
return;
}
// ...
}
```
Scenario Description:
1. A chained bio is created in the block layer.
2. This bio is intercepted by the bcache layer to be routed to the appropriate
backing disk.
3. The code path determines that the backing device is in a detached state,
leading to a call to `detached_dev_do_request()` to handle the I/O.
4. The memory allocation for the `ddip` structure fails.
5. In the error path, the function directly invokes `bio->bi_end_io(bio)`.
The Problem:
For a bio that is part of a chain, the `bi_end_io` function is likely set to
`bio_chain_endio`. Directly calling it in this context would corrupt the
`bi_remaining` reference count, exactly as described in our previous
discussion.
Is it a valid theoretical scenario?
And there is another call:
```
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;
/* Count on the bcache device */
bio_end_io_acct_remapped(bio, ddip->start_time, ddip->orig_bdev);
if (bio->bi_status) {
struct cached_dev *dc = container_of(ddip->d,
struct cached_dev, disk);
/* should count I/O error for backing device here */
bch_count_backing_io_errors(dc, bio);
}
kfree(ddip);
bio->bi_end_io(bio);
}
```
Would you mind me adding the bcache to the talk?
[Adding the bcache list to the CC]
Thanks,
Shida
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Fix potential data loss and corruption due to Incorrect BIO Chain Handling
[not found] ` <CAHc6FU7+riVQBX7L2uk64A355rF+DfQ6xhP425ruQ76d_SDPGA@mail.gmail.com>
2025-11-23 3:14 ` Fix potential data loss and corruption due to Incorrect BIO Chain Handling Stephen Zhang
@ 2025-11-23 13:48 ` Ming Lei
2025-11-24 1:28 ` Stephen Zhang
1 sibling, 1 reply; 4+ messages in thread
From: Ming Lei @ 2025-11-23 13:48 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Stephen Zhang, linux-kernel, linux-block, nvdimm, virtualization,
linux-nvme, gfs2, ntfs3, linux-xfs, zhangshida, Coly Li,
linux-bcache
On Sat, Nov 22, 2025 at 03:56:58PM +0100, Andreas Gruenbacher wrote:
> On Sat, Nov 22, 2025 at 1:07 PM Ming Lei <ming.lei@redhat.com> wrote:
> > > static void bio_chain_endio(struct bio *bio)
> > > {
> > > bio_endio(__bio_chain_endio(bio));
> > > }
> >
> > bio_chain_endio() never gets called really, which can be thought as `flag`,
>
> That's probably where this stops being relevant for the problem
> reported by Stephen Zhang.
>
> > and it should have been defined as `WARN_ON_ONCE(1);` for not confusing people.
>
> But shouldn't bio_chain_endio() still be fixed to do the right thing
> if called directly, or alternatively, just BUG()? Warning and still
> doing the wrong thing seems a bit bizarre.
IMO calling ->bi_end_io() directly shouldn't be encouraged.
The only in-tree direct call user could be bcache, so is this reported
issue triggered on bcache?
If bcache can't call bio_endio(), I think it is fine to fix
bio_chain_endio().
>
> I also see direct bi_end_io calls in erofs_fileio_ki_complete(),
> erofs_fscache_bio_endio(), and erofs_fscache_submit_bio(), so those
> are at least confusing.
All looks FS bio(non-chained), so bio_chain_endio() shouldn't be involved
in erofs code base.
Thanks,
Ming
^ permalink raw reply [flat|nested] 4+ messages in thread