On Wed, Jul 15, 2020 at 03:18:10PM +0200, Jan Kara wrote:
> 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.
Looks good, thanks!
Reviewed-by: Lukas Czerner <lczerner@xxxxxxxxxx>
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
> 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
>
