| Message ID | 20240213101515.17328-1-jack@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: Verify s_clusters_per_group even without bigalloc | expand |
Hello! On 2024/2/13 18:15, 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 is s_clusters_per_group is invalid. This results in an ^^ if > 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 | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 0f931d0c227d..522683075067 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -4451,7 +4451,15 @@ 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_clusters_per_group = > + le32_to_cpu(es->s_clusters_per_group); > + if (sbi->s_blocks_per_group != sbi->s_clusters_per_group) { > + 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; > + } This is almost the same with the code snippet in bigalloc branch, would it be better to factor them out and reuse this hunk in both branch, just like the check e2fsprogs does? Thanks, Yi. > sbi->s_cluster_bits = 0; > } > sbi->s_cluster_ratio = clustersize / sb->s_blocksize; >
On Sun 18-02-24 10:16:40, Zhang Yi wrote: > Hello! > > On 2024/2/13 18:15, 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 is s_clusters_per_group is invalid. This results in an > ^^ > if > > 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 | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index 0f931d0c227d..522683075067 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -4451,7 +4451,15 @@ 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_clusters_per_group = > > + le32_to_cpu(es->s_clusters_per_group); > > + if (sbi->s_blocks_per_group != sbi->s_clusters_per_group) { > > + 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; > > + } > > This is almost the same with the code snippet in bigalloc branch, would > it be better to factor them out and reuse this hunk in both branch, just > like the check e2fsprogs does? Yeah, good point. Let me send v2. Honza
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 0f931d0c227d..522683075067 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -4451,7 +4451,15 @@ 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_clusters_per_group = + le32_to_cpu(es->s_clusters_per_group); + if (sbi->s_blocks_per_group != sbi->s_clusters_per_group) { + 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_bits = 0; } sbi->s_cluster_ratio = clustersize / sb->s_blocksize;
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 is 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 | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)