2021-09-04 04:42:42

by yangerkun

Subject: [PATCH 0/2] bugfix for read_extent_tree_block

Our stress testcase with io error can trigger later OOB:

[59898.282466] BUG: KASAN: slab-out-of-bounds in ext4_find_extent+0x2e4/0x480
...
[59898.287162] Call Trace:
[59898.287575] dump_stack+0x8b/0xb9
[59898.288070] print_address_description+0x73/0x280
[59898.289903] ext4_find_extent+0x2e4/0x480
[59898.290553] ext4_ext_map_blocks+0x125/0x1470
[59898.295481] ext4_map_blocks+0x5ee/0x940
[59898.315984] ext4_mpage_readpages+0x63c/0xdb0
[59898.320231] read_pages+0xe6/0x370
[59898.321589] __do_page_cache_readahead+0x233/0x2a0
[59898.321594] ondemand_readahead+0x157/0x450
[59898.321598] generic_file_read_iter+0xcb2/0x1550
[59898.328828] __vfs_read+0x233/0x360
[59898.328840] vfs_read+0xa5/0x190
[59898.330126] ksys_read+0xa5/0x150
[59898.331405] do_syscall_64+0x6d/0x1f0
[59898.331418] entry_SYSCALL_64_after_hwframe+0x44/0xa9

It seem that a xattr block with verified confuse read_extent_tree_block.

The detail of the patch show us how to fix it.

yangerkun (2):
ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE
ext4: check magic even the extent block bh is verified

fs/ext4/extents.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)

--
2.31.1

2021-09-04 04:43:51

by yangerkun

Subject: [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE

Buffer with verified means that it has been checked before. No need
verify and call set_buffer_verified again.

Signed-off-by: yangerkun <[email protected]>
---
fs/ext4/extents.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index cbf37b2cf871..8559e288472f 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -505,13 +505,16 @@ __read_extent_tree_block(const char *function, unsigned int line,
if (err < 0)
goto errout;
}
- if (buffer_verified(bh) && !(flags & EXT4_EX_FORCE_CACHE))
- return bh;
- err = __ext4_ext_check(function, line, inode,
- ext_block_hdr(bh), depth, pblk);
- if (err)
- goto errout;
- set_buffer_verified(bh);
+ if (buffer_verified(bh)) {
+ if (!(flags & EXT4_EX_FORCE_CACHE))
+ return bh;
+ } else {
+ err = __ext4_ext_check(function, line, inode,
+ ext_block_hdr(bh), depth, pblk);
+ if (err)
+ goto errout;
+ set_buffer_verified(bh);
+ }
/*
* If this is a leaf block, cache all of its entries
*/
--
2.31.1

2021-09-24 09:33:06

by yangerkun

Subject: Re: [PATCH 0/2] bugfix for read_extent_tree_block

gently ping...

在 2021/9/4 12:49, yangerkun 写道:
> Our stress testcase with io error can trigger later OOB:
>
> [59898.282466] BUG: KASAN: slab-out-of-bounds in ext4_find_extent+0x2e4/0x480
> ...
> [59898.287162] Call Trace:
> [59898.287575] dump_stack+0x8b/0xb9
> [59898.288070] print_address_description+0x73/0x280
> [59898.289903] ext4_find_extent+0x2e4/0x480
> [59898.290553] ext4_ext_map_blocks+0x125/0x1470
> [59898.295481] ext4_map_blocks+0x5ee/0x940
> [59898.315984] ext4_mpage_readpages+0x63c/0xdb0
> [59898.320231] read_pages+0xe6/0x370
> [59898.321589] __do_page_cache_readahead+0x233/0x2a0
> [59898.321594] ondemand_readahead+0x157/0x450
> [59898.321598] generic_file_read_iter+0xcb2/0x1550
> [59898.328828] __vfs_read+0x233/0x360
> [59898.328840] vfs_read+0xa5/0x190
> [59898.330126] ksys_read+0xa5/0x150
> [59898.331405] do_syscall_64+0x6d/0x1f0
> [59898.331418] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> It seem that a xattr block with verified confuse read_extent_tree_block.
>
> The detail of the patch show us how to fix it.
>
> yangerkun (2):
> ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE
> ext4: check magic even the extent block bh is verified
>
> fs/ext4/extents.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>

2021-10-01 09:06:33

by Jan Kara

Subject: Re: [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE

On Sat 04-09-21 12:49:45, yangerkun wrote:
> Buffer with verified means that it has been checked before. No need
> verify and call set_buffer_verified again.
>
> Signed-off-by: yangerkun <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/ext4/extents.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index cbf37b2cf871..8559e288472f 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -505,13 +505,16 @@ __read_extent_tree_block(const char *function, unsigned int line,
> if (err < 0)
> goto errout;
> }
> - if (buffer_verified(bh) && !(flags & EXT4_EX_FORCE_CACHE))
> - return bh;
> - err = __ext4_ext_check(function, line, inode,
> - ext_block_hdr(bh), depth, pblk);
> - if (err)
> - goto errout;
> - set_buffer_verified(bh);
> + if (buffer_verified(bh)) {
> + if (!(flags & EXT4_EX_FORCE_CACHE))
> + return bh;
> + } else {
> + err = __ext4_ext_check(function, line, inode,
> + ext_block_hdr(bh), depth, pblk);
> + if (err)
> + goto errout;
> + set_buffer_verified(bh);
> + }
> /*
> * If this is a leaf block, cache all of its entries
> */
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR