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

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

[Christoph Hellwig]

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>
---
 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

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

[Darrick J. Wong]

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
> 

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

[Christoph Hellwig]

On Tue, Sep 05, 2023 at 08:39:53AM -0700, Darrick J. Wong wrote:
> Looks like a good improvement.  Who should this go through, me (iomap)
> or viro/brauner (vfs*) ?

Either way is fine with me - this is pretty iomap specific, but sits
in buffer.c, so things are a bit complicated.

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

[Christian Brauner]

On Tue, Sep 05, 2023 at 08:39:53AM -0700, Darrick J. Wong wrote:
> 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?)

I can grab it if you want.
What's up with email? Did you send this a long time ago?

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

[Darrick J. Wong]

On Wed, Sep 06, 2023 at 04:30:39PM +0200, Christian Brauner wrote:
> On Tue, Sep 05, 2023 at 08:39:53AM -0700, Darrick J. Wong wrote:
> > 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?)
> 
> I can grab it if you want.
> What's up with email? Did you send this a long time ago?

This reply I only sent yesterday, but last week Konstantin reported that
gmail was DDOSing vger again, and then I didn't get any emails for 4
days.

On the bright side I did have a good sprint for online fsck. :)

--D

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

[Damien Le Moal]

On 9/5/23 21:41, 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>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

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

[Hannes Reinecke]

On 9/5/23 14:41, 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>
> ---
>   fs/buffer.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Christian Brauner]

On Tue, 05 Sep 2023 14:41:20 +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.
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] iomap: handle error conditions more gracefully in iomap_to_bh
      https://git.kernel.org/vfs/vfs/c/6f71eda85ab8