* [PATCH 0/2] bugfix for read_extent_tree_block
@ 2021-09-04 4:49 yangerkun
2021-09-04 4:49 ` [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE yangerkun
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: yangerkun @ 2021-09-04 4:49 UTC (permalink / raw)
To: tytso, jack; +Cc: linux-ext4, yangerkun, yukuai3
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE 2021-09-04 4:49 [PATCH 0/2] bugfix for read_extent_tree_block yangerkun @ 2021-09-04 4:49 ` yangerkun 2021-10-01 9:04 ` Jan Kara 2021-09-04 4:49 ` [PATCH 2/2] ext4: check magic even the extent block bh is verified yangerkun 2021-09-24 9:32 ` [PATCH 0/2] bugfix for read_extent_tree_block yangerkun 2 siblings, 1 reply; 10+ messages in thread From: yangerkun @ 2021-09-04 4:49 UTC (permalink / raw) To: tytso, jack; +Cc: linux-ext4, yangerkun, yukuai3 Buffer with verified means that it has been checked before. No need verify and call set_buffer_verified again. Signed-off-by: yangerkun <yangerkun@huawei.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE 2021-09-04 4:49 ` [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE yangerkun @ 2021-10-01 9:04 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2021-10-01 9:04 UTC (permalink / raw) To: yangerkun; +Cc: tytso, jack, linux-ext4, yukuai3 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 <yangerkun@huawei.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> 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 <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] ext4: check magic even the extent block bh is verified 2021-09-04 4:49 [PATCH 0/2] bugfix for read_extent_tree_block yangerkun 2021-09-04 4:49 ` [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE yangerkun @ 2021-09-04 4:49 ` yangerkun 2021-10-01 9:18 ` Jan Kara 2021-09-24 9:32 ` [PATCH 0/2] bugfix for read_extent_tree_block yangerkun 2 siblings, 1 reply; 10+ messages in thread From: yangerkun @ 2021-09-04 4:49 UTC (permalink / raw) To: tytso, jack; +Cc: linux-ext4, yangerkun, yukuai3 Our stress testing with IO error can trigger follow OOB with a very low probability. [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 Digging deep and we found it's actually a xattr block which can happened with follow steps: 1. extent update for file1 and will remove a leaf extent block(block A) 2. we need update the idx extent block too 3. block A has been allocated as a xattr block and will set verified 3. io error happened for this idx block and will the buffer has been released late 4. extent find for file1 will read the idx block and see block A again 5. since the buffer of block A is already verified, we will use it directly, which can lead the upper OOB Same as __ext4_xattr_check_block, we can check magic even the buffer is verified to fix the problem. Signed-off-by: yangerkun <yangerkun@huawei.com> --- fs/ext4/extents.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 8559e288472f..d2e2ae90bc4a 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -506,6 +506,14 @@ __read_extent_tree_block(const char *function, unsigned int line, goto errout; } if (buffer_verified(bh)) { + if (unlikely(ext_block_hdr(bh)->eh_magic != EXT4_EXT_MAGIC)) { + err = -EFSCORRUPTED; + ext4_error_inode(inode, function, line, 0, + "invalid magic for verified extent block %llu", + (unsigned long long)bh->b_blocknr); + goto errout; + } + if (!(flags & EXT4_EX_FORCE_CACHE)) return bh; } else { -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: check magic even the extent block bh is verified 2021-09-04 4:49 ` [PATCH 2/2] ext4: check magic even the extent block bh is verified yangerkun @ 2021-10-01 9:18 ` Jan Kara 2021-10-01 14:09 ` Theodore Ts'o 2021-10-08 1:38 ` yangerkun 0 siblings, 2 replies; 10+ messages in thread From: Jan Kara @ 2021-10-01 9:18 UTC (permalink / raw) To: yangerkun; +Cc: tytso, jack, linux-ext4, yukuai3 On Sat 04-09-21 12:49:46, yangerkun wrote: > Our stress testing with IO error can trigger follow OOB with a very low > probability. > > [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 > > Digging deep and we found it's actually a xattr block which can happened > with follow steps: > > 1. extent update for file1 and will remove a leaf extent block(block A) > 2. we need update the idx extent block too > 3. block A has been allocated as a xattr block and will set verified > 3. io error happened for this idx block and will the buffer has been > released late > 4. extent find for file1 will read the idx block and see block A again > 5. since the buffer of block A is already verified, we will use it > directly, which can lead the upper OOB > > Same as __ext4_xattr_check_block, we can check magic even the buffer is > verified to fix the problem. > > Signed-off-by: yangerkun <yangerkun@huawei.com> Honestly, I'm not sure if this is worth it. What you suggest will work if the magic is overwritten but if we reallocate the block for something else but the magic happens to stay intact, we have a problem. The filesystem is corrupted at that point with metadata blocks being multiply claimed and that's very difficult to deal with. Maybe we should start ignoring buffer_verified() bit once the fs is known to have errors and recheck the buffer contents on each access? Sure it will be slow but I have little sympathy towards people running filesystems with errors... What do people think? Honza > --- > fs/ext4/extents.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 8559e288472f..d2e2ae90bc4a 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -506,6 +506,14 @@ __read_extent_tree_block(const char *function, unsigned int line, > goto errout; > } > if (buffer_verified(bh)) { > + if (unlikely(ext_block_hdr(bh)->eh_magic != EXT4_EXT_MAGIC)) { > + err = -EFSCORRUPTED; > + ext4_error_inode(inode, function, line, 0, > + "invalid magic for verified extent block %llu", > + (unsigned long long)bh->b_blocknr); > + goto errout; > + } > + > if (!(flags & EXT4_EX_FORCE_CACHE)) > return bh; > } else { > -- > 2.31.1 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: check magic even the extent block bh is verified 2021-10-01 9:18 ` Jan Kara @ 2021-10-01 14:09 ` Theodore Ts'o 2021-10-14 7:21 ` yangerkun 2021-10-08 1:38 ` yangerkun 1 sibling, 1 reply; 10+ messages in thread From: Theodore Ts'o @ 2021-10-01 14:09 UTC (permalink / raw) To: Jan Kara; +Cc: yangerkun, linux-ext4, yukuai3 On Fri, Oct 01, 2021 at 11:18:33AM +0200, Jan Kara wrote: > > > > Digging deep and we found it's actually a xattr block which can happened > > with follow steps: > > > > 1. extent update for file1 and will remove a leaf extent block(block A) > > 2. we need update the idx extent block too > > 3. block A has been allocated as a xattr block and will set verified > > 3. io error happened for this idx block and will the buffer has been > > released late > > 4. extent find for file1 will read the idx block and see block A again > > 5. since the buffer of block A is already verified, we will use it > > directly, which can lead the upper OOB > > > > Same as __ext4_xattr_check_block, we can check magic even the buffer is > > verified to fix the problem. > > > > Signed-off-by: yangerkun <yangerkun@huawei.com> > > Honestly, I'm not sure if this is worth it. What you suggest will work if > the magic is overwritten but if we reallocate the block for something else > but the magic happens to stay intact, we have a problem. The filesystem is > corrupted at that point with metadata blocks being multiply claimed and > that's very difficult to deal with. Maybe we should start ignoring > buffer_verified() bit once the fs is known to have errors and recheck the > buffer contents on each access? Sure it will be slow but I have little > sympathy towards people running filesystems with errors... What do people > think? At some point, if we transition away from using buffer_heads for the jbd2 layer, and use our own ext4_metadata_buf structure which incorporates the journal_head and buffer_head fields, this will allow us to control our own writeback, and allow us to have our own error callbacks so we can do things like declare an inode to be bad and not to be referenced again. This would allow us to have a metadata type field, so we could know that a buffer had been verified as an inode table block, or bitmap block, or an xattr block. However, I think the bigger issue is that even if we had a metadata type field in the buffer_head (or ext4_metadata_buf), we should be using the metadata validation, and buffer_verified bit, as a backup. It should not be the primary line of defense. So what I would suggest doing is preventing the out of bounds reference in ext4_find_extent() in the first place. I note we're not sanity checking the values of EXT4_{FIRST,LAST}_{EXTENT,INDEX} used in ext4_ext_binsearch() and ext4_ext_binsearch_idx(), and that's probably how we triggered the out of bounds read in the first place. The cost of making sure that pointers returned by EXT4_{FIRST,LAST}_{EXTENT,INDEX} don't exceed the bounds of the extent tree node would be minimal, and it would be an additional cross check which would protect us against the buffer getting corrupted while in memory (bit flips, or wild pointer dereferences). Cheers, - Ted ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: check magic even the extent block bh is verified 2021-10-01 14:09 ` Theodore Ts'o @ 2021-10-14 7:21 ` yangerkun 2021-10-25 1:10 ` yangerkun 0 siblings, 1 reply; 10+ messages in thread From: yangerkun @ 2021-10-14 7:21 UTC (permalink / raw) To: Theodore Ts'o, Jan Kara; +Cc: linux-ext4, yukuai3 在 2021/10/1 22:09, Theodore Ts'o 写道: > On Fri, Oct 01, 2021 at 11:18:33AM +0200, Jan Kara wrote: >>> >>> Digging deep and we found it's actually a xattr block which can happened >>> with follow steps: >>> >>> 1. extent update for file1 and will remove a leaf extent block(block A) >>> 2. we need update the idx extent block too >>> 3. block A has been allocated as a xattr block and will set verified >>> 3. io error happened for this idx block and will the buffer has been >>> released late >>> 4. extent find for file1 will read the idx block and see block A again >>> 5. since the buffer of block A is already verified, we will use it >>> directly, which can lead the upper OOB >>> >>> Same as __ext4_xattr_check_block, we can check magic even the buffer is >>> verified to fix the problem. >>> >>> Signed-off-by: yangerkun <yangerkun@huawei.com> >> >> Honestly, I'm not sure if this is worth it. What you suggest will work if >> the magic is overwritten but if we reallocate the block for something else >> but the magic happens to stay intact, we have a problem. The filesystem is >> corrupted at that point with metadata blocks being multiply claimed and >> that's very difficult to deal with. Maybe we should start ignoring >> buffer_verified() bit once the fs is known to have errors and recheck the >> buffer contents on each access? Sure it will be slow but I have little >> sympathy towards people running filesystems with errors... What do people >> think? > > At some point, if we transition away from using buffer_heads for the > jbd2 layer, and use our own ext4_metadata_buf structure which > incorporates the journal_head and buffer_head fields, this will allow > us to control our own writeback, and allow us to have our own error > callbacks so we can do things like declare an inode to be bad and not > to be referenced again. This would allow us to have a metadata type > field, so we could know that a buffer had been verified as an inode > table block, or bitmap block, or an xattr block. > > However, I think the bigger issue is that even if we had a metadata > type field in the buffer_head (or ext4_metadata_buf), we should be > using the metadata validation, and buffer_verified bit, as a backup. > It should not be the primary line of defense. > > So what I would suggest doing is preventing the out of bounds > reference in ext4_find_extent() in the first place. I note we're not > sanity checking the values of EXT4_{FIRST,LAST}_{EXTENT,INDEX} used in > ext4_ext_binsearch() and ext4_ext_binsearch_idx(), and that's probably > how we triggered the out of bounds read in the first place. The cost > of making sure that pointers returned by > EXT4_{FIRST,LAST}_{EXTENT,INDEX} don't exceed the bounds of the extent > tree node would be minimal, and it would be an additional cross check > which would protect us against the buffer getting corrupted while in > memory (bit flips, or wild pointer dereferences). Sorry for the latter replay. This can prevent a corrupt extent block buffer(maybe a xattr block for another file) with verified trigger the OOB. But once corrupt data in extent block buffer won't trigger OOB. We pass the check and will use a xattr block's data as a extent block. This may trigger other unpredictable result... The patch I send check the magic to ensure the block is really a extent block which prevent this case. But for the case a extent block been reallocated as another file's extent block. This seems useless and will lead to some problem too. But we may first stop the unpredictable result like the OOB or other error. > > Cheers, > > - Ted > . > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: check magic even the extent block bh is verified 2021-10-14 7:21 ` yangerkun @ 2021-10-25 1:10 ` yangerkun 0 siblings, 0 replies; 10+ messages in thread From: yangerkun @ 2021-10-25 1:10 UTC (permalink / raw) To: Theodore Ts'o, Jan Kara; +Cc: linux-ext4, yukuai3 在 2021/10/14 15:21, yangerkun 写道: > > 在 2021/10/1 22:09, Theodore Ts'o 写道: >> On Fri, Oct 01, 2021 at 11:18:33AM +0200, Jan Kara wrote: >>>> >>>> Digging deep and we found it's actually a xattr block which can >>>> happened >>>> with follow steps: >>>> >>>> 1. extent update for file1 and will remove a leaf extent block(block A) >>>> 2. we need update the idx extent block too >>>> 3. block A has been allocated as a xattr block and will set verified >>>> 3. io error happened for this idx block and will the buffer has been >>>> released late >>>> 4. extent find for file1 will read the idx block and see block A again >>>> 5. since the buffer of block A is already verified, we will use it >>>> directly, which can lead the upper OOB >>>> >>>> Same as __ext4_xattr_check_block, we can check magic even the buffer is >>>> verified to fix the problem. >>>> >>>> Signed-off-by: yangerkun <yangerkun@huawei.com> >>> >>> Honestly, I'm not sure if this is worth it. What you suggest will >>> work if >>> the magic is overwritten but if we reallocate the block for something >>> else >>> but the magic happens to stay intact, we have a problem. The >>> filesystem is >>> corrupted at that point with metadata blocks being multiply claimed and >>> that's very difficult to deal with. Maybe we should start ignoring >>> buffer_verified() bit once the fs is known to have errors and recheck >>> the >>> buffer contents on each access? Sure it will be slow but I have little >>> sympathy towards people running filesystems with errors... What do >>> people >>> think? >> >> At some point, if we transition away from using buffer_heads for the >> jbd2 layer, and use our own ext4_metadata_buf structure which >> incorporates the journal_head and buffer_head fields, this will allow >> us to control our own writeback, and allow us to have our own error >> callbacks so we can do things like declare an inode to be bad and not >> to be referenced again. This would allow us to have a metadata type >> field, so we could know that a buffer had been verified as an inode >> table block, or bitmap block, or an xattr block. >> >> However, I think the bigger issue is that even if we had a metadata >> type field in the buffer_head (or ext4_metadata_buf), we should be >> using the metadata validation, and buffer_verified bit, as a backup. >> It should not be the primary line of defense. >> >> So what I would suggest doing is preventing the out of bounds >> reference in ext4_find_extent() in the first place. I note we're not >> sanity checking the values of EXT4_{FIRST,LAST}_{EXTENT,INDEX} used in >> ext4_ext_binsearch() and ext4_ext_binsearch_idx(), and that's probably >> how we triggered the out of bounds read in the first place. The cost >> of making sure that pointers returned by >> EXT4_{FIRST,LAST}_{EXTENT,INDEX} don't exceed the bounds of the extent >> tree node would be minimal, and it would be an additional cross check >> which would protect us against the buffer getting corrupted while in >> memory (bit flips, or wild pointer dereferences). > > Sorry for the latter replay. > > This can prevent a corrupt extent block buffer(maybe a xattr block for > another file) with verified trigger the OOB. But once corrupt data in > extent block buffer won't trigger OOB. We pass the check and will use a > xattr block's data as a extent block. This may trigger other > unpredictable result... > > The patch I send check the magic to ensure the block is really a extent > block which prevent this case. But for the case a extent block been > reallocated as another file's extent block. This seems useless and will > lead to some problem too. But we may first stop the unpredictable result > like the OOB or other error. Hi, Does there some advise for this... Thanks, Kun. > >> >> Cheers, >> >> - Ted >> . >> > . ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: check magic even the extent block bh is verified 2021-10-01 9:18 ` Jan Kara 2021-10-01 14:09 ` Theodore Ts'o @ 2021-10-08 1:38 ` yangerkun 1 sibling, 0 replies; 10+ messages in thread From: yangerkun @ 2021-10-08 1:38 UTC (permalink / raw) To: Jan Kara; +Cc: tytso, linux-ext4, yukuai3 在 2021/10/1 17:18, Jan Kara 写道: > On Sat 04-09-21 12:49:46, yangerkun wrote: >> Our stress testing with IO error can trigger follow OOB with a very low >> probability. >> >> [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 >> >> Digging deep and we found it's actually a xattr block which can happened >> with follow steps: >> >> 1. extent update for file1 and will remove a leaf extent block(block A) >> 2. we need update the idx extent block too >> 3. block A has been allocated as a xattr block and will set verified >> 3. io error happened for this idx block and will the buffer has been >> released late >> 4. extent find for file1 will read the idx block and see block A again >> 5. since the buffer of block A is already verified, we will use it >> directly, which can lead the upper OOB >> >> Same as __ext4_xattr_check_block, we can check magic even the buffer is >> verified to fix the problem. >> >> Signed-off-by: yangerkun <yangerkun@huawei.com> > > Honestly, I'm not sure if this is worth it. What you suggest will work if > the magic is overwritten but if we reallocate the block for something else > but the magic happens to stay intact, we have a problem. The filesystem is > corrupted at that point with metadata blocks being multiply claimed and > that's very difficult to deal with. Maybe we should start ignoring > buffer_verified() bit once the fs is known to have errors and recheck the > buffer contents on each access? Sure it will be slow but I have little > sympathy towards people running filesystems with errors... What do people > think? What you means was that something like a extent block for inode A has been reallocate as a extent block for inode B? Ignoring buffer_verified seems useless for this case since extent check will pass. Maybe we should first try to prevent the OOB... > > Honza > >> --- >> fs/ext4/extents.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index 8559e288472f..d2e2ae90bc4a 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -506,6 +506,14 @@ __read_extent_tree_block(const char *function, unsigned int line, >> goto errout; >> } >> if (buffer_verified(bh)) { >> + if (unlikely(ext_block_hdr(bh)->eh_magic != EXT4_EXT_MAGIC)) { >> + err = -EFSCORRUPTED; >> + ext4_error_inode(inode, function, line, 0, >> + "invalid magic for verified extent block %llu", >> + (unsigned long long)bh->b_blocknr); >> + goto errout; >> + } >> + >> if (!(flags & EXT4_EX_FORCE_CACHE)) >> return bh; >> } else { >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] bugfix for read_extent_tree_block 2021-09-04 4:49 [PATCH 0/2] bugfix for read_extent_tree_block yangerkun 2021-09-04 4:49 ` [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE yangerkun 2021-09-04 4:49 ` [PATCH 2/2] ext4: check magic even the extent block bh is verified yangerkun @ 2021-09-24 9:32 ` yangerkun 2 siblings, 0 replies; 10+ messages in thread From: yangerkun @ 2021-09-24 9:32 UTC (permalink / raw) To: tytso, jack; +Cc: linux-ext4, yukuai3 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(-) > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-10-25 1:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-04 4:49 [PATCH 0/2] bugfix for read_extent_tree_block yangerkun 2021-09-04 4:49 ` [PATCH 1/2] ext4: avoid recheck extent for EXT4_EX_FORCE_CACHE yangerkun 2021-10-01 9:04 ` Jan Kara 2021-09-04 4:49 ` [PATCH 2/2] ext4: check magic even the extent block bh is verified yangerkun 2021-10-01 9:18 ` Jan Kara 2021-10-01 14:09 ` Theodore Ts'o 2021-10-14 7:21 ` yangerkun 2021-10-25 1:10 ` yangerkun 2021-10-08 1:38 ` yangerkun 2021-09-24 9:32 ` [PATCH 0/2] bugfix for read_extent_tree_block yangerkun
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.