Message-ID: <d07a4397-1648-4264-8a30-74a2ea3da165@kernel.org>
Date: Wed, 20 Aug 2025 16:02:31 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Keith Busch <kbusch@...a.com>, linux-block@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: snitzer@...nel.org, axboe@...nel.dk, dw@...idwei.uk, brauner@...nel.org,
 hch@....de, martin.petersen@...cle.com, djwong@...nel.org,
 linux-xfs@...r.kernel.org, viro@...iv.linux.org.uk,
 Keith Busch <kbusch@...nel.org>
Subject: Re: [PATCHv3 1/8] block: check for valid bio while splitting

On 8/20/25 1:49 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@...nel.org>
> 
> We're already iterating every segment, so check these for a valid IO at
> the same time.
> 
> Signed-off-by: Keith Busch <kbusch@...nel.org>
> ---
>  block/blk-map.c        |  2 +-
>  block/blk-merge.c      | 20 ++++++++++++++++----
>  include/linux/bio.h    |  4 ++--
>  include/linux/blkdev.h | 13 +++++++++++++
>  4 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/block/blk-map.c b/block/blk-map.c
> index 23e5d5ebe59ec..c5da9d37deee9 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -443,7 +443,7 @@ int blk_rq_append_bio(struct request *rq, struct bio *bio)
>  	int ret;
>  
>  	/* check that the data layout matches the hardware restrictions */
> -	ret = bio_split_rw_at(bio, lim, &nr_segs, max_bytes);
> +	ret = bio_split_drv_at(bio, lim, &nr_segs, max_bytes);
>  	if (ret) {
>  		/* if we would have to split the bio, copy instead */
>  		if (ret > 0)
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 70d704615be52..a0d8364983000 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -279,25 +279,29 @@ static unsigned int bio_split_alignment(struct bio *bio,
>  }
>  
>  /**
> - * bio_split_rw_at - check if and where to split a read/write bio
> + * bio_split_io_at - check if and where to split a read/write bio
>   * @bio:  [in] bio to be split
>   * @lim:  [in] queue limits to split based on
>   * @segs: [out] number of segments in the bio with the first half of the sectors
>   * @max_bytes: [in] maximum number of bytes per bio
> + * @len_align: [in] length alignment for each vector
>   *
>   * Find out if @bio needs to be split to fit the queue limits in @lim and a
>   * maximum size of @max_bytes.  Returns a negative error number if @bio can't be
>   * split, 0 if the bio doesn't have to be split, or a positive sector offset if
>   * @bio needs to be split.
>   */
> -int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
> -		unsigned *segs, unsigned max_bytes)
> +int bio_split_io_at(struct bio *bio, const struct queue_limits *lim,
> +		unsigned *segs, unsigned max_bytes, unsigned len_align)
>  {
>  	struct bio_vec bv, bvprv, *bvprvp = NULL;
>  	struct bvec_iter iter;
>  	unsigned nsegs = 0, bytes = 0;
>  
>  	bio_for_each_bvec(bv, bio, iter) {
> +		if (bv.bv_offset & lim->dma_alignment || bv.bv_len & len_align)

Shouldn't this be:

		if (bv.bv_offset & len_align || bv.bv_len & len_align)

?

> +			return -EINVAL;
> +
>  		/*
>  		 * If the queue doesn't support SG gaps and adding this
>  		 * offset would create a gap, disallow it.
> @@ -339,8 +343,16 @@ int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
>  	 * Individual bvecs might not be logical block aligned. Round down the
>  	 * split size so that each bio is properly block size aligned, even if
>  	 * we do not use the full hardware limits.
> +	 *
> +	 * Misuse may submit a bio that can't be split into a valid io. There
> +	 * may either be too many discontiguous vectors for the max segments
> +	 * limit, or contain virtual boundary gaps without having a valid block
> +	 * sized split. Catch that condition by checking for a zero byte
> +	 * result.
>  	 */
>  	bytes = ALIGN_DOWN(bytes, bio_split_alignment(bio, lim));
> +	if (!bytes)
> +		return -EINVAL;
>  
>  	/*
>  	 * Bio splitting may cause subtle trouble such as hang when doing sync
> @@ -350,7 +362,7 @@ int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
>  	bio_clear_polled(bio);
>  	return bytes >> SECTOR_SHIFT;
>  }
> -EXPORT_SYMBOL_GPL(bio_split_rw_at);
> +EXPORT_SYMBOL_GPL(bio_split_io_at);
>  
>  struct bio *bio_split_rw(struct bio *bio, const struct queue_limits *lim,
>  		unsigned *nr_segs)
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 46ffac5caab78..519a1d59805f8 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -322,8 +322,8 @@ static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio)
>  void bio_trim(struct bio *bio, sector_t offset, sector_t size);
>  extern struct bio *bio_split(struct bio *bio, int sectors,
>  			     gfp_t gfp, struct bio_set *bs);
> -int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
> -		unsigned *segs, unsigned max_bytes);
> +int bio_split_io_at(struct bio *bio, const struct queue_limits *lim,
> +		unsigned *segs, unsigned max_bytes, unsigned len_align);
>  
>  /**
>   * bio_next_split - get next @sectors from a bio, splitting if necessary
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 95886b404b16b..7f83ad2df5425 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1869,6 +1869,19 @@ 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);
> +}
> +
> +static inline int bio_split_drv_at(struct bio *bio,
> +		const struct queue_limits *lim,
> +		unsigned *segs, unsigned max_bytes)
> +{
> +	return bio_split_io_at(bio, lim, segs, max_bytes, 0);
> +}
>  #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
>  
>  #endif /* _LINUX_BLKDEV_H */


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists