[PATCH 1/2] xfs: handle bio split errors during gc

[PATCH 1/2] xfs: handle bio split errors during gc

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

bio_split_rw_at may return a negative error value if the bio can not be
split into anything that adheres to the block device's queue limits.
Check for that condition and fail the bio accordingly.

Reported-by: Chris Mason <clm@fb.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 fs/xfs/xfs_zone_gc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 064cd1a857a02..24799715efdc4 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -765,6 +765,8 @@ xfs_zone_gc_split_write(
 			lim->max_zone_append_sectors << SECTOR_SHIFT);
 	if (!split_sectors)
 		return NULL;
+	if (split_sectors < 0)
+		return ERR_PTR(split_sectors);
 
 	/* ensure the split chunk is still block size aligned */
 	split_sectors = ALIGN_DOWN(split_sectors << SECTOR_SHIFT,
@@ -819,8 +821,16 @@ xfs_zone_gc_write_chunk(
 	bio_add_folio_nofail(&chunk->bio, chunk->scratch->folio, chunk->len,
 			offset_in_folio(chunk->scratch->folio, bvec_paddr));
 
-	while ((split_chunk = xfs_zone_gc_split_write(data, chunk)))
+	while (!IS_ERR_OR_NULL(split_chunk = xfs_zone_gc_split_write(data, chunk)))
 		xfs_zone_gc_submit_write(data, split_chunk);
+
+	if (IS_ERR(split_chunk)) {
+		chunk->bio.bi_status = errno_to_blk_status(PTR_ERR(
+								split_chunk));
+		bio_endio(&chunk->bio);
+		return;
+	}
+
 	xfs_zone_gc_submit_write(data, chunk);
 }
 
-- 
2.47.3

[PATCH 2/2] btrfs: handle bio split errors for append

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

bio_split_rw_at may return a negative error value if the bio can not be
split into anything that adheres to the block device's queue limits.
Check for that condition and fail the bio accordingly.

Reported-by: Chris Mason <clm@fb.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 fs/btrfs/bio.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 21df48e6c4fa2..0ca86526a8bd8 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -646,6 +646,8 @@ static u64 btrfs_append_map_length(struct btrfs_bio *bbio, u64 map_length)
 	sector_offset = bio_split_rw_at(&bbio->bio, &bbio->fs_info->limits,
 					&nr_segs, map_length);
 	if (sector_offset) {
+		if (unlikely(sector_offset < 0))
+			return sector_offset;
 		/*
 		 * bio_split_rw_at() could split at a size smaller than our
 		 * sectorsize and thus cause unaligned I/Os.  Fix that by
@@ -685,8 +687,14 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
 	}
 
 	map_length = min(map_length, length);
-	if (use_append)
+	if (use_append) {
 		map_length = btrfs_append_map_length(bbio, map_length);
+		if (IS_ERR_VALUE(map_length)) {
+			status = errno_to_blk_status(map_length);
+			btrfs_bio_counter_dec(fs_info);
+			goto end_bbio;
+		}
+	}
 
 	if (map_length < length) {
 		struct btrfs_bio *split;
-- 
2.47.3

Re: [PATCH 1/2] xfs: handle bio split errors during gc

[Christoph Hellwig]

On Mon, Oct 20, 2025 at 07:43:55AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> bio_split_rw_at may return a negative error value if the bio can not be
> split into anything that adheres to the block device's queue limits.
> Check for that condition and fail the bio accordingly.

Ugg, how?  If that actually happens we're toast, so we should find a
way to ensure it does not happen.

Re: [PATCH 1/2] xfs: handle bio split errors during gc

[Keith Busch]

On Mon, Oct 20, 2025 at 04:45:22PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 20, 2025 at 07:43:55AM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > bio_split_rw_at may return a negative error value if the bio can not be
> > split into anything that adheres to the block device's queue limits.
> > Check for that condition and fail the bio accordingly.
> 
> Ugg, how?  If that actually happens we're toast, so we should find a
> way to ensure it does not happen.

You'd have to attempt sending an invalid bvec, like something that can't
DMA map because you have a byte aligned offset, or the total size is
smaller than the block device's.

Not that you're doing anything like that here. This condition should
never occur in this path because the bio vectors are all nicely aligned.
It's just for completeness to ensure it doesn't go uncaught for every
bio split caller.

Re: [PATCH 1/2] xfs: handle bio split errors during gc

[Christoph Hellwig]

On Mon, Oct 20, 2025 at 08:54:46AM -0600, Keith Busch wrote:
> > Ugg, how?  If that actually happens we're toast, so we should find a
> > way to ensure it does not happen.
> 
> You'd have to attempt sending an invalid bvec, like something that can't
> DMA map because you have a byte aligned offset, or the total size is
> smaller than the block device's.
> 
> Not that you're doing anything like that here. This condition should
> never occur in this path because the bio vectors are all nicely aligned.
> It's just for completeness to ensure it doesn't go uncaught for every
> bio split caller.

So this is just from code inspection and you did not actually hit
such a case?

I'll see if I can add some sanity checking for the buffer and
preferably handle this outside the I/O path where error handling
for this doesn't really make sense.

Re: [PATCH 1/2] xfs: handle bio split errors during gc

[Keith Busch]

On Mon, Oct 20, 2025 at 04:57:07PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 20, 2025 at 08:54:46AM -0600, Keith Busch wrote:
> > > Ugg, how?  If that actually happens we're toast, so we should find a
> > > way to ensure it does not happen.
> > 
> > You'd have to attempt sending an invalid bvec, like something that can't
> > DMA map because you have a byte aligned offset, or the total size is
> > smaller than the block device's.
> > 
> > Not that you're doing anything like that here. This condition should
> > never occur in this path because the bio vectors are all nicely aligned.
> > It's just for completeness to ensure it doesn't go uncaught for every
> > bio split caller.
> 
> So this is just from code inspection and you did not actually hit
> such a case?

Correct, no one actually hit such errors. Same is true for the btrfs
patch.

Re: [PATCH 1/2] xfs: handle bio split errors during gc

[Chris Mason]

On 10/20/25 10:57 AM, Christoph Hellwig wrote:
> On Mon, Oct 20, 2025 at 08:54:46AM -0600, Keith Busch wrote:
>>> Ugg, how?  If that actually happens we're toast, so we should find a
>>> way to ensure it does not happen.
>>
>> You'd have to attempt sending an invalid bvec, like something that can't
>> DMA map because you have a byte aligned offset, or the total size is
>> smaller than the block device's.
>>
>> Not that you're doing anything like that here. This condition should
>> never occur in this path because the bio vectors are all nicely aligned.
>> It's just for completeness to ensure it doesn't go uncaught for every
>> bio split caller.
> 
> So this is just from code inspection and you did not actually hit
> such a case?

This is from me testing out AI reviews on linux-next, and they do love
suggesting defensive programming a little too much.  I'd be happier
with EIOs in this case but agree it's probably never going to happen.

Just for completeness, the actual AI output was:

> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 7709d55adc23..9efacabaa2f7 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1870,6 +1870,13 @@ bdev_atomic_write_unit_max_bytes(struct block_device *bdev)
>  	return queue_atomic_write_unit_max_bytes(bdev_get_queue(bdev));
>  }
>
> +static inline int bio_split_rw_at(struct bio *bio,
> +		const struct queue_limits *lim,
> +		unsigned *segs, unsigned max_bytes)
> +{
> +	return bio_split_io_at(bio, lim, segs, max_bytes, lim->dma_alignment);
> +}
> +
>  #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
>
>  #endif /* _LINUX_BLKDEV_H */

bio_split_rw_at() now returns negative errors in three new cases:
  1. bv.bv_offset alignment violation
  2. bv.bv_len alignment violation
  3. Zero bytes after ALIGN_DOWN

However, two callers don't handle these new negative returns correctly:

In fs/btrfs/bio.c, btrfs_append_map_length() does this:

    sector_offset = bio_split_rw_at(&bbio->bio, &bbio->fs_info->limits,
                                    &nr_segs, map_length);
    if (sector_offset) {
        return ALIGN_DOWN(sector_offset << SECTOR_SHIFT,
                          bbio->fs_info->sectorsize);
    }

Can this mishandle the negative returns? When bio_split_rw_at() returns
-EINVAL (-22), the condition 'if (sector_offset)' is true. Then
'sector_offset << SECTOR_SHIFT' becomes -11264, and ALIGN_DOWN operates
on a negative value. The function returns this as if it's a valid
length (u64). The caller btrfs_submit_chunk() then uses this value in
'if (map_length < length)' and may attempt to split the bio at an
invalid position.

Similarly, in fs/xfs/xfs_zone_gc.c, xfs_zone_gc_split_write() does:

    split_sectors = bio_split_rw_at(&chunk->bio, lim, &nsegs,
                    lim->max_zone_append_sectors << SECTOR_SHIFT);
    if (!split_sectors)
        return NULL;

    split_sectors = ALIGN_DOWN(split_sectors << SECTOR_SHIFT,
                    data->mp->m_sb.sb_blocksize) >> SECTOR_SHIFT;
    split = bio_split(&chunk->bio, split_sectors, GFP_NOFS, &data->bio_set);

Can this pass negative values to bio_split()? The check only handles
zero, so when bio_split_rw_at() returns -EINVAL, the negative value
continues through ALIGN_DOWN operations and eventually reaches
bio_split(). While bio_split() has a WARN_ON_ONCE(sectors <= 0) check
and returns ERR_PTR(-EINVAL), xfs_zone_gc_split_write() doesn't check
for ERR_PTR. It does container_of() and returns the error pointer to
the caller xfs_zone_gc_write_chunk(), which treats it as a valid
pointer in the while loop.

Re: [PATCH 1/2] xfs: handle bio split errors during gc

[Christoph Hellwig]

On Mon, Oct 20, 2025 at 11:16:57AM -0400, Chris Mason wrote:
> On 10/20/25 10:57 AM, Christoph Hellwig wrote:
> > On Mon, Oct 20, 2025 at 08:54:46AM -0600, Keith Busch wrote:
> >>> Ugg, how?  If that actually happens we're toast, so we should find a
> >>> way to ensure it does not happen.
> >>
> >> You'd have to attempt sending an invalid bvec, like something that can't
> >> DMA map because you have a byte aligned offset, or the total size is
> >> smaller than the block device's.
> >>
> >> Not that you're doing anything like that here. This condition should
> >> never occur in this path because the bio vectors are all nicely aligned.
> >> It's just for completeness to ensure it doesn't go uncaught for every
> >> bio split caller.
> > 
> > So this is just from code inspection and you did not actually hit
> > such a case?
> 
> This is from me testing out AI reviews on linux-next, and they do love
> suggesting defensive programming a little too much.  I'd be happier
> with EIOs in this case but agree it's probably never going to happen.

It would be relaly useful to note in the patches if something was
found by code analys or testing and what tools are used.

Re: [PATCH 2/2] btrfs: handle bio split errors for append

[kernel test robot]

Hi Keith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on kdave/for-next linus/master v6.18-rc2 next-20251021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Keith-Busch/btrfs-handle-bio-split-errors-for-append/20251020-224536
base:   https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
patch link:    https://lore.kernel.org/r/20251020144356.693288-2-kbusch%40meta.com
patch subject: [PATCH 2/2] btrfs: handle bio split errors for append
config: i386-buildonly-randconfig-005-20251021 (https://download.01.org/0day-ci/archive/20251022/202510220421.KBMIIY8p-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251022/202510220421.KBMIIY8p-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510220421.KBMIIY8p-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/build_bug.h:5,
                    from arch/x86/include/asm/current.h:5,
                    from include/linux/sched.h:12,
                    from include/linux/mempool.h:8,
                    from include/linux/bio.h:8,
                    from fs/btrfs/bio.c:7:
   fs/btrfs/bio.c: In function 'btrfs_submit_chunk':
>> include/linux/err.h:28:49: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
      28 | #define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
         |                                                 ^
   include/linux/compiler.h:32:55: note: in definition of macro '__branch_check__'
      32 |                         ______r = __builtin_expect(!!(x), expect);      \
         |                                                       ^
   include/linux/err.h:28:25: note: in expansion of macro 'unlikely'
      28 | #define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
         |                         ^~~~~~~~
   fs/btrfs/bio.c:692:21: note: in expansion of macro 'IS_ERR_VALUE'
     692 |                 if (IS_ERR_VALUE(map_length)) {
         |                     ^~~~~~~~~~~~
>> include/linux/err.h:28:49: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
      28 | #define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
         |                                                 ^
   include/linux/compiler.h:34:54: note: in definition of macro '__branch_check__'
      34 |                                              expect, is_constant);      \
         |                                                      ^~~~~~~~~~~
   include/linux/err.h:28:25: note: in expansion of macro 'unlikely'
      28 | #define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
         |                         ^~~~~~~~
   fs/btrfs/bio.c:692:21: note: in expansion of macro 'IS_ERR_VALUE'
     692 |                 if (IS_ERR_VALUE(map_length)) {
         |                     ^~~~~~~~~~~~


vim +28 include/linux/err.h

ebba5f9fcb88230 Randy Dunlap   2006-09-27  21  
4d744ce9d5d7cf0 James Seo      2023-05-09  22  /**
4d744ce9d5d7cf0 James Seo      2023-05-09  23   * IS_ERR_VALUE - Detect an error pointer.
4d744ce9d5d7cf0 James Seo      2023-05-09  24   * @x: The pointer to check.
4d744ce9d5d7cf0 James Seo      2023-05-09  25   *
4d744ce9d5d7cf0 James Seo      2023-05-09  26   * Like IS_ERR(), but does not generate a compiler warning if result is unused.
4d744ce9d5d7cf0 James Seo      2023-05-09  27   */
aa00edc1287a693 Linus Torvalds 2016-05-27 @28  #define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
07ab67c8d0d7c10 Linus Torvalds 2005-05-19  29  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki