Re: [PATCH 2/2] ext4: add inode table check in __ext4_get_inode_loc to aovid possible infinite loop
From: Baokun Li
Date: Wed Aug 17 2022 - 21:54:55 EST
On 8/17/2022 10:31 PM, Jan Kara wrote:
On Wed 17-08-22 21:27:01, Baokun Li wrote:
In do_writepages, if the value returned by ext4_writepages is "-ENOMEM"
and "wbc->sync_mode == WB_SYNC_ALL", retry until the condition is not met.
In __ext4_get_inode_loc, if the bh returned by sb_getblk is NULL,
the function returns -ENOMEM.
In __getblk_slow, if the return value of grow_buffers is less than 0,
the function returns NULL.
When the three processes are connected in series like the following stack,
an infinite loop may occur:
do_writepages <--- keep retrying
ext4_writepages
mpage_map_and_submit_extent
mpage_map_one_extent
ext4_map_blocks
ext4_ext_map_blocks
ext4_ext_handle_unwritten_extents
ext4_ext_convert_to_initialized
ext4_split_extent
ext4_split_extent_at
__ext4_ext_dirty
__ext4_mark_inode_dirty
ext4_reserve_inode_write
ext4_get_inode_loc
__ext4_get_inode_loc <--- return -ENOMEM
sb_getblk
__getblk_gfp
__getblk_slow <--- return NULL
grow_buffers
grow_dev_page <--- return -ENXIO
ret = (block < end_block) ? 1 : -ENXIO;
In this issue, bg_inode_table_hi is overwritten as an incorrect value.
As a result, `block < end_block` cannot be met in grow_dev_page.
Therefore, __ext4_get_inode_loc always returns '-ENOMEM' and do_writepages
keeps retrying. As a result, the writeback process is in the D state due
to an infinite loop.
Add a check on inode table block in the __ext4_get_inode_loc function by
referring to ext4_read_inode_bitmap to avoid this infinite loop.
Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
Thanks for the fixes. Normally, we check that inode table is fine in
ext4_check_descriptors() (and those checks are much stricter) so it seems
Yes, when I first found this problem, I thought that the inode table was
an abnormal value
when the disk was mounted. However, in ext4_ check_ Descriptors see the
inspection of inode table.
ext4_ check_ Descriptors is more strict, and it also checks blocks_
Bitmap and inode_ bitmap。
However, in addition to mounting, I can't find any changes to
bg_inode_table_hi in other parts of
the kernel, which makes me very confused. It wasn't until I tracked the
address of the variable that
I found it was modified by memcpy.
This check is added here to facilitate us to find problems and avoid
dead loops. Otherwise,
we may not find problems until the write back process is stuck in the D
state for several hours.
At this time, the file system may be in a mess.
unnecessary to check it again here. I understand that in your case it was
resize that corrupted the group descriptor after the filesystem was mounted
which is nasty but there's much more metadata that can be corrupted like
this and it's infeasible to check each metadata block before we use it.
Indeed, But is it true that checking for inode_bitmap in
ext4_read_inode_bitmap and
checking for block_bitmap in ext4_read_block_bitmap_nowait also
unnecessary?
After all, inode_bitmap and block_bitmap may be faulty only when the
metadata is corrupted.
I think it seems unreasonable that both block_bitmap and inode_bitmap
have checks and inode_table does not.
IMHO a proper fix to this class of issues would be for sb_getblk() to
return proper error so that we can distinguish ENOMEM from other errors.
Totally agree! There is a FIXME in the comments of __getblk_gfp:
` ` `
__getblk_gfp() will lock up the machine if grow_dev_page's
try_to_free_buffers() attempt is failing. FIXME, perhaps?
` ` `
This is the same as this issue because the return value of the
grow_buffers function is not propagated correctly.
But that will be a larger undertaking...
Honza
Yes, this function can be called in many places, and external interface
changes are involved.
---
fs/ext4/inode.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Thanks!
--
With Best Regards,
Baokun Li