Received-SPF: pass (google.com: domain of linux-ext4+bounces-1285-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Subject: Re: [PATCH v2] ext4: Verify s_clusters_per_group even without
 bigalloc
To: Jan Kara <jack@suse.cz>
CC: <linux-ext4@vger.kernel.org>, Ted Tso <tytso@mit.edu>
References: <20240219171033.22882-1-jack@suse.cz>
From: Zhang Yi <yi.zhang@huawei.com>
Message-ID: <a426f70f-1553-4850-3b0e-0584f8cbdb71@huawei.com>
Date: Tue, 20 Feb 2024 09:55:11 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240219171033.22882-1-jack@suse.cz>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 2024/2/20 1:10, Jan Kara wrote:
> Currently we ignore s_clusters_per_group field in the on-disk superblock
> if bigalloc feature is not enabled. However e2fsprogs don't even open
> the filesystem if s_clusters_per_group is invalid. This results in an
> odd state where kernel happily works with the filesystem while even
> e2fsck refuses to touch it. Verify that s_clusters_per_group is valid
> even if bigalloc feature is not enabled to make things consistent. Due
> to current e2fsprogs behavior it is unlikely there are filesystems out
> in the wild (except for intentionally fuzzed ones) with invalid
> s_clusters_per_group counts.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/ext4/super.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> Changes since v1:
> * share code checking s_clusters_per_group for !bigalloc & bigalloc configs
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 0f931d0c227d..0a34e0b23541 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4422,22 +4422,6 @@ static int ext4_handle_clustersize(struct super_block *sb)
>  		}
>  		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
>  			le32_to_cpu(es->s_log_block_size);
> -		sbi->s_clusters_per_group =
> -			le32_to_cpu(es->s_clusters_per_group);
> -		if (sbi->s_clusters_per_group > sb->s_blocksize * 8) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "#clusters per group too big: %lu",
> -				 sbi->s_clusters_per_group);
> -			return -EINVAL;
> -		}
> -		if (sbi->s_blocks_per_group !=
> -		    (sbi->s_clusters_per_group * (clustersize / sb->s_blocksize))) {
> -			ext4_msg(sb, KERN_ERR, "blocks per group (%lu) and "
> -				 "clusters per group (%lu) inconsistent",
> -				 sbi->s_blocks_per_group,
> -				 sbi->s_clusters_per_group);
> -			return -EINVAL;
> -		}
>  	} else {
>  		if (clustersize != sb->s_blocksize) {
>  			ext4_msg(sb, KERN_ERR,
> @@ -4451,9 +4435,21 @@ static int ext4_handle_clustersize(struct super_block *sb)
>  				 sbi->s_blocks_per_group);
>  			return -EINVAL;
>  		}
> -		sbi->s_clusters_per_group = sbi->s_blocks_per_group;
>  		sbi->s_cluster_bits = 0;
>  	}
> +	sbi->s_clusters_per_group = le32_to_cpu(es->s_clusters_per_group);
> +	if (sbi->s_clusters_per_group > sb->s_blocksize * 8) {
> +		ext4_msg(sb, KERN_ERR, "#clusters per group too big: %lu",
> +			 sbi->s_clusters_per_group);
> +		return -EINVAL;
> +	}
> +	if (sbi->s_blocks_per_group !=
> +	    (sbi->s_clusters_per_group * (clustersize / sb->s_blocksize))) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "blocks per group (%lu) and clusters per group (%lu) inconsistent",
> +			 sbi->s_blocks_per_group, sbi->s_clusters_per_group);
> +		return -EINVAL;
> +	}
>  	sbi->s_cluster_ratio = clustersize / sb->s_blocksize;
>  
>  	/* Do we have standard group size of clustersize * 8 blocks ? */
>