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

[Chris Mason <clm@meta.com>]

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.