Re: [PATCH] iomap: handle error conditions more gracefully in iomap_to_bh

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Sep 05, 2023 at 02:41:20PM +0200, Christoph Hellwig wrote:
> iomap_to_bh currently BUG()s when the passed in block number is not
> in the iomap.  For file systems that have proper synchronization this
> should never happen and so far hasn't in mainline, but for block devices
> size changes aren't fully synchronized against ongoing I/O.  Instead
> of BUG()ing in this case, return -EIO to the caller, which already has
> proper error handling.  While we're at it, also return -EIO for an
> unknown iomap state instead of returning garbage.
> 
> Fixes: 487c607df790 ("block: use iomap for writes to block devices")
> Reported-by: syzbot+4a08ffdf3667b36650a1@syzkaller.appspotmail.com
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks like a good improvement.  Who should this go through, me (iomap)
or viro/brauner (vfs*) ?

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

(lol is email down again?)

--D

>  fs/buffer.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 2379564e5aeadf..a6785cd07081cb 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2011,7 +2011,7 @@ void folio_zero_new_buffers(struct folio *folio, size_t from, size_t to)
>  }
>  EXPORT_SYMBOL(folio_zero_new_buffers);
>  
> -static void
> +static int
>  iomap_to_bh(struct inode *inode, sector_t block, struct buffer_head *bh,
>  		const struct iomap *iomap)
>  {
> @@ -2025,7 +2025,8 @@ iomap_to_bh(struct inode *inode, sector_t block, struct buffer_head *bh,
>  	 * current block, then do not map the buffer and let the caller
>  	 * handle it.
>  	 */
> -	BUG_ON(offset >= iomap->offset + iomap->length);
> +	if (offset >= iomap->offset + iomap->length)
> +		return -EIO;
>  
>  	switch (iomap->type) {
>  	case IOMAP_HOLE:
> @@ -2037,7 +2038,7 @@ iomap_to_bh(struct inode *inode, sector_t block, struct buffer_head *bh,
>  		if (!buffer_uptodate(bh) ||
>  		    (offset >= i_size_read(inode)))
>  			set_buffer_new(bh);
> -		break;
> +		return 0;
>  	case IOMAP_DELALLOC:
>  		if (!buffer_uptodate(bh) ||
>  		    (offset >= i_size_read(inode)))
> @@ -2045,7 +2046,7 @@ iomap_to_bh(struct inode *inode, sector_t block, struct buffer_head *bh,
>  		set_buffer_uptodate(bh);
>  		set_buffer_mapped(bh);
>  		set_buffer_delay(bh);
> -		break;
> +		return 0;
>  	case IOMAP_UNWRITTEN:
>  		/*
>  		 * For unwritten regions, we always need to ensure that regions
> @@ -2062,7 +2063,10 @@ iomap_to_bh(struct inode *inode, sector_t block, struct buffer_head *bh,
>  		bh->b_blocknr = (iomap->addr + offset - iomap->offset) >>
>  				inode->i_blkbits;
>  		set_buffer_mapped(bh);
> -		break;
> +		return 0;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return -EIO;
>  	}
>  }
>  
> @@ -2103,13 +2107,12 @@ int __block_write_begin_int(struct folio *folio, loff_t pos, unsigned len,
>  			clear_buffer_new(bh);
>  		if (!buffer_mapped(bh)) {
>  			WARN_ON(bh->b_size != blocksize);
> -			if (get_block) {
> +			if (get_block)
>  				err = get_block(inode, block, bh, 1);
> -				if (err)
> -					break;
> -			} else {
> -				iomap_to_bh(inode, block, bh, iomap);
> -			}
> +			else
> +				err = iomap_to_bh(inode, block, bh, iomap);
> +			if (err)
> +				break;
>  
>  			if (buffer_new(bh)) {
>  				clean_bdev_bh_alias(bh);
> -- 
> 2.39.2
>