Re: [PATCH 10/13] block: don't require data unit aligned bio_vecs for encrypted bios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] btrfs: infinite loop and double submission in btrfs_submit_bbio() due to unhandled negative error from bio_split_rw_at()
--

commit 7790faffa64672f5a012e4c7a5a36eb8b60cf57a
Author: Baokun Li <libaokun@linux.alibaba.com>

block: don't require data unit aligned bio_vecs for encrypted bios

This patch drops the requirement that every bio_vec in an encrypted bio
must be data unit aligned and a multiple of the data unit size. The
alignment is now handled by bio_split_alignment() rounding the split size
down to the data unit size.

> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 9bcc9c52eea17..dd9be4c04041a 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -346,20 +346,18 @@ static inline unsigned int bvec_seg_gap(struct bio_vec *bvprv,
>  int bio_split_io_at(struct bio *bio, const struct queue_limits *lim,
>  		unsigned *segs, unsigned max_bytes, unsigned len_align_mask)
>  {
> -	struct bio_crypt_ctx *bc = bio_crypt_ctx(bio);
>  	struct bio_vec bv, bvprv, *bvprvp = NULL;
>  	unsigned nsegs = 0, bytes = 0, gaps = 0;
>  	unsigned int align, split_bytes;
>  	struct bvec_iter iter;
> -	unsigned start_align_mask = lim->dma_alignment;
> -
> -	if (bc) {
> -		start_align_mask |= (bc->bc_key->crypto_cfg.data_unit_size - 1);
> -		len_align_mask |= (bc->bc_key->crypto_cfg.data_unit_size - 1);
> -	}
>  
>  	bio_for_each_bvec(bv, bio, iter) {
> -		if (bv.bv_offset & start_align_mask ||
> +		/*
> +		 * The bio_vecs of a bio with a crypto context don't have to
> +		 * be data unit aligned, only the bio as a whole does; see
> +		 * bio_crypt_set_ctx() and bio_split_alignment().
> +		 */
> +		if (bv.bv_offset & lim->dma_alignment ||
>  		    bv.bv_len & len_align_mask)
>  			return -EINVAL;

[Severity: Critical]
Since bio_split_io_at() (and functions wrapping it like bio_split_rw_at())
returns negative error codes such as -EINVAL, can this lead to an infinite
loop in btrfs callers?

In fs/btrfs/bio.c:btrfs_append_map_length(), the return value is not
checked for negative error codes:

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

If bio_split_rw_at() returns a negative error code (such as -EINVAL or
-EAGAIN), ALIGN_DOWN() will operate on a negative value and return a massive
u64.

This huge value is then returned as map_length to btrfs_submit_chunk(),
where it causes the bio split to be skipped since map_length < length is
false.

Because btrfs_submit_chunk() returns false, it causes its caller
btrfs_submit_bbio() to enter an infinite loop:

	while (!btrfs_submit_chunk(bbio, mirror_num))
		;

Do the btrfs callers need to be updated to handle these negative return
values, or does this patch change the conditions under which
bio_split_io_at() returns an error for encrypted bios?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011170958.1759230-1-libaokun@linux.alibaba.com?part=10