On Mon 15-04-19 17:19:45, Barret Rhoden wrote: > When remounting with debug_want_extra_isize, we were not performing the > same checks that we do during a normal mount. That allowed us to set a > value for s_want_extra_isize that reached outside the s_inode_size. > > Reported-by: [email protected] > Signed-off-by: Barret Rhoden <[email protected]> > Cc: [email protected]
The patch looks good to me. You can add: Reviewed-by: Jan Kara <[email protected]> Honza > --- > > - In the current code, it looks like someone could mount with want_extra_isize > with some value > 0 but less than the minimums in the s_es. If that's a > bug, > I can submit a follow-on patch. > > - Similarly, on a failed remount, sbi->s_want_extra_isize is changed to the > remounted value. I can fix that too if it's a problem. > > - Is it OK to remount with a smaller s_want_extra_isize than the previous > mount? > I thought it was, but figured I'd ask while I'm looking at it. > > > fs/ext4/super.c | 58 +++++++++++++++++++++++++++++-------------------- > 1 file changed, 34 insertions(+), 24 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 6ed4eb81e674..184944d4d8d1 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -3513,6 +3513,37 @@ int ext4_calculate_overhead(struct super_block *sb) > return 0; > } > > +static void ext4_clamp_want_extra_isize(struct super_block *sb) > +{ > + struct ext4_sb_info *sbi = EXT4_SB(sb); > + struct ext4_super_block *es = sbi->s_es; > + > + /* determine the minimum size of new large inodes, if present */ > + if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE && > + sbi->s_want_extra_isize == 0) { > + sbi->s_want_extra_isize = sizeof(struct ext4_inode) - > + EXT4_GOOD_OLD_INODE_SIZE; > + if (ext4_has_feature_extra_isize(sb)) { > + if (sbi->s_want_extra_isize < > + le16_to_cpu(es->s_want_extra_isize)) > + sbi->s_want_extra_isize = > + le16_to_cpu(es->s_want_extra_isize); > + if (sbi->s_want_extra_isize < > + le16_to_cpu(es->s_min_extra_isize)) > + sbi->s_want_extra_isize = > + le16_to_cpu(es->s_min_extra_isize); > + } > + } > + /* Check if enough inode space is available */ > + if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize > > + sbi->s_inode_size) { > + sbi->s_want_extra_isize = sizeof(struct ext4_inode) - > + EXT4_GOOD_OLD_INODE_SIZE; > + ext4_msg(sb, KERN_INFO, > + "required extra inode space not available"); > + } > +} > + > static void ext4_set_resv_clusters(struct super_block *sb) > { > ext4_fsblk_t resv_clusters; > @@ -4387,30 +4418,7 @@ static int ext4_fill_super(struct super_block *sb, > void *data, int silent) > } else if (ret) > goto failed_mount4a; > > - /* determine the minimum size of new large inodes, if present */ > - if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE && > - sbi->s_want_extra_isize == 0) { > - sbi->s_want_extra_isize = sizeof(struct ext4_inode) - > - EXT4_GOOD_OLD_INODE_SIZE; > - if (ext4_has_feature_extra_isize(sb)) { > - if (sbi->s_want_extra_isize < > - le16_to_cpu(es->s_want_extra_isize)) > - sbi->s_want_extra_isize = > - le16_to_cpu(es->s_want_extra_isize); > - if (sbi->s_want_extra_isize < > - le16_to_cpu(es->s_min_extra_isize)) > - sbi->s_want_extra_isize = > - le16_to_cpu(es->s_min_extra_isize); > - } > - } > - /* Check if enough inode space is available */ > - if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize > > - sbi->s_inode_size) { > - sbi->s_want_extra_isize = sizeof(struct ext4_inode) - > - EXT4_GOOD_OLD_INODE_SIZE; > - ext4_msg(sb, KERN_INFO, "required extra inode space not" > - "available"); > - } > + ext4_clamp_want_extra_isize(sb); > > ext4_set_resv_clusters(sb); > > @@ -5194,6 +5202,8 @@ static int ext4_remount(struct super_block *sb, int > *flags, char *data) > goto restore_opts; > } > > + ext4_clamp_want_extra_isize(sb); > + > if ((old_opts.s_mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) ^ > test_opt(sb, JOURNAL_CHECKSUM)) { > ext4_msg(sb, KERN_ERR, "changing journal_checksum " > -- > 2.21.0.392.gf8f6787159e-goog > -- Jan Kara <[email protected]> SUSE Labs, CR
