Hello,

we are not quite careful enough about setting bio->bi_status in all
places (see BACKGROUND below).  This patch queue tries to fix this by
systematically eliminating the direct assignments to bi_status sprinkled
all throughout the code.  Please comment.


The first patch ("bio: rename bio_chain arguments") is an loosely
related cleanup.  The remaining changes are:

- Use bio_io_error() in more places.

- Add a bio_set_errno() helper for setting bi_status based on an errno.
  Use this helper throughout the code.

- Add a bio_set_status() helper for setting bi_status to a blk_status_t
  status code.  Use this helper in places in the code where it's
  necessary, or at least useful without adding any overhead.

And on top of that, we have two more cleanups:

- Add a bio_endio_errno() helper that combines bio_set_errno() and
  bio_endio().

- Add a bio_endio_status() helper that combines bio_set_status() and
  bio_endio().

The patches are currently based on v6.18.

GIT tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=bio-cleanups

With these changes, only a few direct assignments to bio->bi_status
remain, in BTRFS and in MD, and SOME OF THOSE MAY BE UNSAFE.  Could the
maintainers of those subsystems please have a look?

Once the remaining direct assignments to bi_status are gone, we may want
to think about "write protecting" bi_status to prevent unintended new
direct assignments from creeping back in.


BACKGROUND

'struct bio' objects start out with their bi_status field initialized to
BLK_STS_OK (0).  When the bio completes, that field needs to be left
unchanged in case of success, and set to a BLK_STS_* error code
otherwise.

This is important when bios are chained (bio_chain()) because then,
multiple execution contexts will race updating the same bi_status field.
When an execution context resets bi_status to BLK_STS_OK (0) during bio
completion, this could hide the error code of the adjacent bio in the
chain.

When more than a single bio fails in a chain, we know that the resulting
bi_status will not be BLK_STS_OK, but we don't know which of the status
error codes will win.


CRYPTO FALLBACK (SATYA TANGIRALA?)

Related to chained bios but unrelated to setting bio->bi_status,
blk_crypto_fallback_encrypt_bio() in block/blk-crypto-fallback.c swaps
out bi_private and bi_end_io and reuses the same bio for downcalls, then
restores those fields in blk_crypto_fallback_decrypt_endio() before
calling bio_endio() again on the same bio.  This will at the very least
break with chained bios because it will mess up __bi_remaining.


Thanks,
Andreas

Andreas Gruenbacher (12):
  bio: rename bio_chain arguments
  bio: use bio_io_error more often
  bio: add bio_set_errno
  bio: use bio_set_errno in more places
  bio: add bio_set_status
  bio: don't check target->bi_status on error
  bio: use bio_set_status for BLK_STS_* status codes
  bio: use bio_set_status in some more places
  bio: switch to bio_set_status in submit_bio_noacct
  bio: never set bi_status to BLK_STS_OK during completion
  bio: add bio_endio_errno
  bio: add bio_endio_status

 block/bio-integrity-auto.c       |  3 +--
 block/bio.c                      | 25 ++++++++++++-------------
 block/blk-core.c                 | 10 ++++------
 block/blk-crypto-fallback.c      | 22 +++++++++++-----------
 block/blk-crypto-internal.h      |  2 +-
 block/blk-crypto.c               |  4 ++--
 block/blk-merge.c                |  6 ++----
 block/blk-mq.c                   | 11 ++++-------
 block/fops.c                     |  6 ++----
 block/t10-pi.c                   |  2 +-
 drivers/block/aoe/aoecmd.c       |  8 ++++----
 drivers/block/aoe/aoedev.c       |  2 +-
 drivers/block/drbd/drbd_int.h    |  3 +--
 drivers/block/drbd/drbd_req.c    |  9 +++------
 drivers/block/ps3vram.c          |  3 +--
 drivers/block/zram/zram_drv.c    |  4 ++--
 drivers/md/bcache/bcache.h       |  3 +--
 drivers/md/bcache/request.c      |  8 +++-----
 drivers/md/dm-cache-target.c     |  9 +++++----
 drivers/md/dm-ebs-target.c       |  2 +-
 drivers/md/dm-flakey.c           |  2 +-
 drivers/md/dm-integrity.c        | 30 +++++++++++-------------------
 drivers/md/dm-mpath.c            |  6 ++----
 drivers/md/dm-pcache/dm_pcache.c |  3 +--
 drivers/md/dm-raid1.c            |  7 +++----
 drivers/md/dm-thin.c             |  5 ++---
 drivers/md/dm-vdo/data-vio.c     |  3 +--
 drivers/md/dm-verity-target.c    |  2 +-
 drivers/md/dm-writecache.c       |  7 +++----
 drivers/md/dm-zoned-target.c     |  2 +-
 drivers/md/dm.c                  |  4 +---
 drivers/md/md.c                  |  8 +++-----
 drivers/md/raid1-10.c            |  3 +--
 drivers/md/raid1.c               |  2 +-
 drivers/md/raid10.c              | 18 +++++++-----------
 drivers/md/raid5.c               |  4 ++--
 drivers/nvdimm/btt.c             |  4 ++--
 drivers/nvdimm/pmem.c            |  7 ++-----
 fs/btrfs/bio.c                   |  8 ++++----
 fs/btrfs/direct-io.c             |  2 +-
 fs/btrfs/raid56.c                |  6 ++----
 fs/crypto/bio.c                  |  2 +-
 fs/erofs/fileio.c                |  2 +-
 fs/erofs/fscache.c               |  4 ++--
 fs/f2fs/data.c                   |  6 +++---
 fs/f2fs/segment.c                |  3 +--
 fs/iomap/ioend.c                 |  3 +--
 fs/verity/verify.c               |  2 +-
 fs/xfs/xfs_aops.c                |  3 +--
 fs/xfs/xfs_zone_alloc.c          |  2 +-
 include/linux/bio.h              | 30 +++++++++++++++++++++++++++---
 51 files changed, 153 insertions(+), 179 deletions(-)

-- 
2.51.0

On Mon, Dec 08, 2025 at 12:10:07PM +0000, Andreas Gruenbacher wrote:
> Hello,
> 
> we are not quite careful enough about setting bio->bi_status in all
> places (see BACKGROUND below).  This patch queue tries to fix this by
> systematically eliminating the direct assignments to bi_status sprinkled
> all throughout the code.  Please comment.
> 
> 
> The first patch ("bio: rename bio_chain arguments") is an loosely
> related cleanup.  The remaining changes are:
> 
> - Use bio_io_error() in more places.
> 
> - Add a bio_set_errno() helper for setting bi_status based on an errno.
>   Use this helper throughout the code.
> 
> - Add a bio_set_status() helper for setting bi_status to a blk_status_t
>   status code.  Use this helper in places in the code where it's
>   necessary, or at least useful without adding any overhead.
> 
> And on top of that, we have two more cleanups:
> 
> - Add a bio_endio_errno() helper that combines bio_set_errno() and
>   bio_endio().
> 
> - Add a bio_endio_status() helper that combines bio_set_status() and
>   bio_endio().
> 
> The patches are currently based on v6.18.
> 
> GIT tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=bio-cleanups
> 
> With these changes, only a few direct assignments to bio->bi_status
> remain, in BTRFS and in MD, and SOME OF THOSE MAY BE UNSAFE.  Could the
> maintainers of those subsystems please have a look?

The btrfs bits look good to me, we expect the same semantics, ie. not
overwrite existing error with 0. If there are racing writes to the
status like in btrfs_bio_end_io() we use cmpxchg() so we don't overwrite
it.

> Once the remaining direct assignments to bi_status are gone, we may want
> to think about "write protecting" bi_status to prevent unintended new
> direct assignments from creeping back in.

This makes sense, though I'm not sure if this takes into account the
mentioned cmpxchg pattern:

	if (status != BLK_STS_OK)
		cmpxchg(&bbio->status, BLK_STS_OK, status);

On Mon, Dec 8, 2025 at 8:43 PM David Sterba <dsterba@suse.cz> wrote:
> On Mon, Dec 08, 2025 at 12:10:07PM +0000, Andreas Gruenbacher wrote:
> > With these changes, only a few direct assignments to bio->bi_status
> > remain, in BTRFS and in MD, and SOME OF THOSE MAY BE UNSAFE.  Could the
> > maintainers of those subsystems please have a look?
>
> The btrfs bits look good to me, we expect the same semantics, ie. not
> overwrite existing error with 0. If there are racing writes to the
> status like in btrfs_bio_end_io() we use cmpxchg() so we don't overwrite
> it.

Really? I'm not talking about the status field in struct btrfs_bio but
about the bi_status field in struct bio. The first mention of
bi_status I can find in the btrfs code is right at the beginning of
btrfs_bio_end_io():

  bbio->bio.bi_status = status;

If status is ever BLK_STS_OK (0) here and bbio->bio is a chained bio,
things are already not great.

I believe we should eliminate all direct assignments to bi_status and
use bio_set_status() instead. I'm not familiar enough with the btrfs
code to make that replacement for you, though.

A cursory look at struct btrfs_bio suggests that those objects cannot
be chained like plain old bios (bio_chain()). This means that
cmpxchg() may actually work for catching the first error that occurs.
For chained regular bios, cmpxchg() won't catch the first error, at
least not if the length of the chain is greater than two.

Thanks,
Andreas