| Message ID | 20240219171033.22882-1-jack@suse.cz |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [v2] ext4: Verify s_clusters_per_group even without bigalloc | expand |
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 ? */ >
On Mon, 19 Feb 2024 18:10:33 +0100, 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. > > [...] Applied, thanks! [1/1] ext4: Verify s_clusters_per_group even without bigalloc commit: 1f85b452e07c370448fb4eb4472cd55fc6bf115c Best regards,
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 ? */
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> --- 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