Hello!
This series changes ext4 to properly check extent tree blocks of journal inode.
Omitting these (which is a limitation of block validity checks) leads to crash
in ext4_cache_extents() in case the extent tree of the journal inode is
suitably corrupted.
Changes since v1:
* Added Reviewed-by tags from Lukas
* Fixed two more unrelated minor bugs in block validity testing spotted by
Lukas
Honza
There's one place that fails to handle error from add_system_zone() call
and thus we can fail to protect superblock and group-descriptor blocks
properly in case of ENOMEM. Fix it.
Reported-by: Lukas Czerner <[email protected]>
Signed-off-by: Jan Kara <[email protected]>
---
fs/ext4/block_validity.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 9c40214f31f9..2d008c1b58f2 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -235,10 +235,13 @@ int ext4_setup_system_zone(struct super_block *sb)
for (i=0; i < ngroups; i++) {
cond_resched();
if (ext4_bg_has_super(sb, i) &&
- ((i < 5) || ((i % flex_size) == 0)))
- add_system_zone(system_blks,
+ ((i < 5) || ((i % flex_size) == 0))) {
+ ret = add_system_zone(system_blks,
ext4_group_first_block_no(sb, i),
ext4_bg_num_gdb(sb, i) + 1, 0);
+ if (ret)
+ goto err;
+ }
gdp = ext4_get_group_desc(sb, i, NULL);
ret = add_system_zone(system_blks,
ext4_block_bitmap(sb, gdp), 1, 0);
--
2.16.4
Currently, add_system_zone() just silently merges two added system zones
that overlap. However the overlap should not happen and it generally
suggests that some unrelated metadata overlap which indicates the fs is
corrupted. We should have caught such problems earlier (e.g. in
ext4_check_descriptors()) but add this check as another line of defense.
In later patch we also use this for stricter checking of journal inode
extent tree.
Reviewed-by: Lukas Czerner <[email protected]>
Signed-off-by: Jan Kara <[email protected]>
---
fs/ext4/block_validity.c | 36 +++++++++++++-----------------------
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 16e9b2fda03a..b394a50ebbe3 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -68,7 +68,7 @@ static int add_system_zone(struct ext4_system_blocks *system_blks,
ext4_fsblk_t start_blk,
unsigned int count)
{
- struct ext4_system_zone *new_entry = NULL, *entry;
+ struct ext4_system_zone *new_entry, *entry;
struct rb_node **n = &system_blks->root.rb_node, *node;
struct rb_node *parent = NULL, *new_node = NULL;
@@ -79,30 +79,20 @@ static int add_system_zone(struct ext4_system_blocks *system_blks,
n = &(*n)->rb_left;
else if (start_blk >= (entry->start_blk + entry->count))
n = &(*n)->rb_right;
- else {
- if (start_blk + count > (entry->start_blk +
- entry->count))
- entry->count = (start_blk + count -
- entry->start_blk);
- new_node = *n;
- new_entry = rb_entry(new_node, struct ext4_system_zone,
- node);
- break;
- }
+ else /* Unexpected overlap of system zones. */
+ return -EFSCORRUPTED;
}
- if (!new_entry) {
- new_entry = kmem_cache_alloc(ext4_system_zone_cachep,
- GFP_KERNEL);
- if (!new_entry)
- return -ENOMEM;
- new_entry->start_blk = start_blk;
- new_entry->count = count;
- new_node = &new_entry->node;
-
- rb_link_node(new_node, parent, n);
- rb_insert_color(new_node, &system_blks->root);
- }
+ new_entry = kmem_cache_alloc(ext4_system_zone_cachep,
+ GFP_KERNEL);
+ if (!new_entry)
+ return -ENOMEM;
+ new_entry->start_blk = start_blk;
+ new_entry->count = count;
+ new_node = &new_entry->node;
+
+ rb_link_node(new_node, parent, n);
+ rb_insert_color(new_node, &system_blks->root);
/* Can we merge to the left? */
node = rb_prev(new_node);
--
2.16.4
On Mon, Jul 27, 2020 at 01:44:28PM +0200, Jan Kara wrote:
> There's one place that fails to handle error from add_system_zone() call
> and thus we can fail to protect superblock and group-descriptor blocks
> properly in case of ENOMEM. Fix it.
Looks good, thanks.
Reviewed-by: Lukas Czerner <[email protected]>
>
> Reported-by: Lukas Czerner <[email protected]>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> fs/ext4/block_validity.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> index 9c40214f31f9..2d008c1b58f2 100644
> --- a/fs/ext4/block_validity.c
> +++ b/fs/ext4/block_validity.c
> @@ -235,10 +235,13 @@ int ext4_setup_system_zone(struct super_block *sb)
> for (i=0; i < ngroups; i++) {
> cond_resched();
> if (ext4_bg_has_super(sb, i) &&
> - ((i < 5) || ((i % flex_size) == 0)))
> - add_system_zone(system_blks,
> + ((i < 5) || ((i % flex_size) == 0))) {
> + ret = add_system_zone(system_blks,
> ext4_group_first_block_no(sb, i),
> ext4_bg_num_gdb(sb, i) + 1, 0);
> + if (ret)
> + goto err;
> + }
> gdp = ext4_get_group_desc(sb, i, NULL);
> ret = add_system_zone(system_blks,
> ext4_block_bitmap(sb, gdp), 1, 0);
> --
> 2.16.4
>