On Wed, Jan 23, 2013 at 11:39:17AM +0100, Jan Kara wrote:
> On Tue 22-01-13 11:07:58, Carlos Maiolino wrote:
> > This checks for incompatible mounting options when using ext4 module to mount
> > ext3 or ext2 filesystems.
> >
> > Sets two new flags to group ext4 mount options that are incompatible with ext2
> > and ext3, and then add two functions -- check_ext2/3_incompat_mount() -- to
> > check and warn/fail mount, if any of these options are being used.
> >
> > I believe, some options like those expecting an argument needs to be checked
> > during parsing time.
> >
> > barrier mount, although it has a flag, when mounting an ext2fs, where
> > barriers are not supported (afaik), should also be checked during parse
> > time, otherwise the BARRIER mount flag will be set.
> >
> > I didn't add all mount options I believe to need to raise a warning, just
> > those with a flag set on superblock, another flags should be added after a
> > discussion to reach a concensus of all ext2/3 options that should be rejected by
> > ext4 mount.
> Thinking about it a bit more I'm not sure if restricting mount options is
> the right thing to start with. IMHO what we should restrict is mounting
> filesystem with certain *features* as ext3/ext2. So e.g. filesystem with
> EXT4_FEATURE_INCOMPAT_EXTENTS cannot be mounted as ext2 or ext3. Similarly
> as currently we forbid mounting ext3 filesystem with
> EXT3_FEATURE_INCOMPAT_RECOVER as ext2... This should avoid the confusion
> which could arise when someone reports problems with "ext3" filesystem but
> actually has some of the ext4 features enabled.
>
This is interesting, but I wonder if not restricting mount options, but
features, would open a 'window' to let users change their filesystem on-disk
format without know what they are doing, but I might be wrong.
> This also naturally rules out some options such as journal checksum. Then
> for mount options which don't affect the disk format (and thus are
> effectively usable for ext2 or ext3) we can restrict the mount options.
> There are options like dioread_nolock which actually don't matter (option
> is noop for non-extent based files) but I'd forbid them just to reduce the
> confusion. OTOH I would accept 'barrier' option even for ext2 as that
> actually fixes fsync() for it. And then there are options like
> 'inode_readahead' which are boundary. They make sence for all the
> filesystem flavors and hardly can cause any confusion - they just allow
> more tweaking. I'd be inclined to allow these but it's a case by case
> discussion I guess.
Agreed, IMHO we first should define how to restrict mounts (by feature / by
mount option/ or both), then organize what should and shouldn't be restricted
for each filesystem
>
> Honza
>
> >
> > Changelog:
> > V2 - Fail a mount instead of just warning
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > ---
> > fs/ext4/ext4.h | 15 +++++++++++++++
> > fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++
> > 2 files changed, 46 insertions(+)
> >
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 8462eb3..9478f1d 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -995,6 +995,21 @@ struct ext4_inode_info {
> > #define EXT4_MOUNT2_EXPLICIT_DELALLOC 0x00000001 /* User explicitly
> > specified delalloc */
> >
> > +#define EXT4_MOUNT_INCOMPAT_EXT3 (EXT4_MOUNT_JOURNAL_CHECKSUM | \
> > + EXT4_MOUNT_JOURNAL_ASYNC_COMMIT | \
> > + EXT4_MOUNT_DELALLOC | \
> > + EXT4_MOUNT_DISCARD | \
> > + EXT4_MOUNT_BLOCK_VALIDITY | \
> > + EXT4_MOUNT_DATA_ERR_ABORT | \
> > + EXT4_MOUNT_DIOREAD_NOLOCK)
> > +
> > +#define EXT4_MOUNT_INCOMPAT_EXT2 (EXT4_MOUNT_INCOMPAT_EXT3 | \
> > + EXT4_MOUNT_NOLOAD | \
> > + EXT4_MOUNT_JOURNAL_DATA | \
> > + EXT4_MOUNT_WRITEBACK_DATA | \
> > + EXT4_MOUNT_UPDATE_JOURNAL | \
> > + EXT4_MOUNT_ORDERED_DATA)
> > +
> > #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \
> > ~EXT4_MOUNT_##opt
> > #define set_opt(sb, opt) EXT4_SB(sb)->s_mount_opt |= \
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 3d4fb81..63c529e 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -83,6 +83,8 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly);
> > static void ext4_destroy_lazyinit_thread(void);
> > static void ext4_unregister_li_request(struct super_block *sb);
> > static void ext4_clear_request_list(void);
> > +static inline int check_ext3_incompat_mount(struct super_block *sb);
> > +static inline int check_ext2_incompat_mount(struct super_block *sb);
> >
> > #if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT23)
> > static struct file_system_type ext2_fs_type = {
> > @@ -3470,6 +3472,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > "to feature incompatibilities");
> > goto failed_mount;
> > }
> > + ret = check_ext2_incompat_mount(sb);
> > }
> >
> > if (IS_EXT3_SB(sb)) {
> > @@ -3481,8 +3484,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > "to feature incompatibilities");
> > goto failed_mount;
> > }
> > + ret = check_ext3_incompat_mount(sb);
> > }
> >
> > + if (ret == -EPERM)
> > + goto failed_mount;
> > +
> > /*
> > * Check feature flags regardless of the revision level, since we
> > * previously didn't change the revision level when setting the flags,
> > @@ -5194,6 +5201,30 @@ static inline int ext2_feature_set_ok(struct super_block *sb)
> > return 0;
> > return 1;
> > }
> > +
> > +static inline int check_ext3_incompat_mount(struct super_block *sb)
> > +{
> > + if (test_opt(sb, INCOMPAT_EXT3)) {
> > + printk(KERN_WARNING
> > + "EXT4-fs: Mount options incompatible with ext3\n");
> > + return -EPERM;
> > + }
> > +
> > + return 0;
> > +
> > +}
> > +
> > +static inline int check_ext2_incompat_mount(struct super_block *sb)
> > +{
> > + if (test_opt(sb, INCOMPAT_EXT2)) {
> > + printk(KERN_WARNING
> > + "EXT4-fs: Mount options incompatible with ext2\n");
> > + return -EPERM;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > MODULE_ALIAS("ext2");
> > #else
> > static inline void register_as_ext2(void) { }
> > --
> > 1.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Carlos
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
