Re: [PATCH 2/4] ext4: Don't allow overlapping system zones

[Lukas Czerner <lczerner@redhat.com>]

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@redhat.com>


> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  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
>