| Message ID | 20230905124120.325518-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iomap: handle error conditions more gracefully in iomap_to_bh | expand |
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 >
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.
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>
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
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?
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
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
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);
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(-)