This checks for incompatible mounting options when using ext4 module to mount
ext3 or ext2 filesystems.
This first attempt 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 warning, 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 have not added all mount options I believe need to raise a warning, just
those with a flag set on superblock, but I expect to generate a discussion here
about which mount options should be warned and get suggestions about how to deal
with argumented options (ex: commit= opt), to be included in a V2 of this patch.
Signed-off-by: Carlos Maiolino <[email protected]>
---
fs/ext4/ext4.h | 15 +++++++++++++++
fs/ext4/super.c | 20 ++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 3c20de1..f6f020b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -977,6 +977,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 80928f7..93f7a80 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 void check_ext3_incompat_mount(struct super_block *sb);
+static inline void 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 = {
@@ -3475,6 +3477,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
"to feature incompatibilities");
goto failed_mount;
}
+ check_ext2_incompat_mount(sb);
}
if (IS_EXT3_SB(sb)) {
@@ -3486,6 +3489,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
"to feature incompatibilities");
goto failed_mount;
}
+ check_ext3_incompat_mount(sb);
}
/*
@@ -5192,6 +5196,22 @@ static inline int ext2_feature_set_ok(struct super_block *sb)
return 0;
return 1;
}
+
+static inline void 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");
+
+}
+
+static inline void 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");
+}
+
MODULE_ALIAS("ext2");
#else
static inline void register_as_ext2(void) { }
--
1.7.11.7
On Thu, Nov 01, 2012 at 05:32:11PM -0200, Carlos Maiolino wrote:
>
> I have not added all mount options I believe need to raise a
> warning, just those with a flag set on superblock, but I expect to
> generate a discussion here about which mount options should be
> warned and get suggestions about how to deal with argumented options
> (ex: commit= opt), to be included in a V2 of this patch.
Let's take a step back, and see if we can be explicit about why it
would be useful to warn when a user uses mount options that we were
not present with the implementation of the file system drivers found
in fs/ext2 and fs/ext3. While we're at it, we can also examine the
same question with respect to file system features --- i.e., if
someone mounts a file system with an extent feature enabled using
mount -t ext2, what if anything should we do. What are we trying to
achieve, or conversely what are we trying to prevent?
Suppose the user does something like this:
mount -t ext3 -o delalloc /dev/sdb /u1
Yes, the traditional ext3 driver code doesn't understand the delalloc
mount option, but what's the concern that is leading us to want to
print a warning message to the log (that the user may or may not even
see). Are we worried that what should happen is not sufficiently
well-defined? Are we worried that while it might work with a
particular kernel which is compiled a certain way, it might not work
with another kernel? Is it part of the larger question of wanting to
warn if the user is using a set of not-well-tested combination of
mount options and/or file system features?
If it is the latter, what is the right approach? At Barcelona, I was
chatting with Ric Wheeler and Ralf Flaxa, and they had differing
opinions about what the right thing to do for features which are not
supported. Ralf felt that warning in the syslog might be sufficient.
I sugested setting a new kernel taint flag, to make it easier for the
supper desk to twig to the fact that the user was doing something
unusual. Ric offerred his opinion that it was better to hard-fail the
mount. And of course, this is with distro kernels; for the upstream
kernel, I think our goal is to (a) warn the user that they are doing
something unusual, and (b) ask them to tell us (at linux-ext4) what
they are doing and why, so we have a better understanding of what
users want, so we can either add it to our test matrix, or perhaps
warn the user off.
So not only do we need to decide which mount options and file system
features we want to support, or warn against etc., it's also useful to
think about what we want to do when the user does something a little
out of the norm.
And since I very much doubt that upstream, Red Hat, SuSE, Canonical,
etc., will ever agree on the right thing is, this is probably one of
those things where we want to have a scheme where it is relatively
easy for distributors to set their own policy, which may differ for a
community versus enterprise distro kernel.
But in order to make sure we don't end up talking past each other, I
think it's useful to be very explicit about why we want to do this,
before we try to figure what's and the how's.
- Ted
On 11/8/12 11:02 AM, Theodore Ts'o wrote:
> On Thu, Nov 01, 2012 at 05:32:11PM -0200, Carlos Maiolino wrote:
>>
>> I have not added all mount options I believe need to raise a
>> warning, just those with a flag set on superblock, but I expect to
>> generate a discussion here about which mount options should be
>> warned and get suggestions about how to deal with argumented options
>> (ex: commit= opt), to be included in a V2 of this patch.
>
> Let's take a step back, and see if we can be explicit about why it
> would be useful to warn when a user uses mount options that we were
> not present with the implementation of the file system drivers found
> in fs/ext2 and fs/ext3. While we're at it, we can also examine the
> same question with respect to file system features --- i.e., if
> someone mounts a file system with an extent feature enabled using
> mount -t ext2, what if anything should we do. What are we trying to
> achieve, or conversely what are we trying to prevent?
>
> Suppose the user does something like this:
>
> mount -t ext3 -o delalloc /dev/sdb /u1
>
> Yes, the traditional ext3 driver code doesn't understand the delalloc
> mount option, but what's the concern that is leading us to want to
> print a warning message to the log (that the user may or may not even
> see). Are we worried that what should happen is not sufficiently
> well-defined? Are we worried that while it might work with a
> particular kernel which is compiled a certain way, it might not work
> with another kernel? Is it part of the larger question of wanting to
> warn if the user is using a set of not-well-tested combination of
> mount options and/or file system features?
>
> If it is the latter, what is the right approach? At Barcelona, I was
> chatting with Ric Wheeler and Ralf Flaxa, and they had differing
> opinions about what the right thing to do for features which are not
> supported. Ralf felt that warning in the syslog might be sufficient.
> I sugested setting a new kernel taint flag, to make it easier for the
> supper desk to twig to the fact that the user was doing something
> unusual. Ric offerred his opinion that it was better to hard-fail the
> mount. And of course, this is with distro kernels; for the upstream
> kernel, I think our goal is to (a) warn the user that they are doing
> something unusual, and (b) ask them to tell us (at linux-ext4) what
> they are doing and why, so we have a better understanding of what
> users want, so we can either add it to our test matrix, or perhaps
> warn the user off.
The overarching reason is to cut down the test matrix if possible, I think.
This has advantages for both distros & upstream, and might even lead
to code simplification if we can exclude some corner-case behaviors.
But the other question that might need answering is this:
If a user has asked ext4.ko to mount an ext3 device, should it behave
as closely as possible to what the ext3.ko driver used to do? And I'd
say yes. Therefore things like -t ext3 -o delalloc should be hard-failed.
If you really want your old ext3 filesystem to be mounted with delalloc,
then I think the right way would be to use mount -t ext4 /dev/ext3-dev.
Then you get the runtime behaviors such as delalloc, and ext4 should
not write anything which is not ext3-compatible.
Perhaps we need to go through in fine detail, but I think mount -t
ext3 should reject any options not applicable to ext3.ko. If you want
the newer ext4 behaviors, then use mount -t ext4.
> So not only do we need to decide which mount options and file system
> features we want to support, or warn against etc., it's also useful to
> think about what we want to do when the user does something a little
> out of the norm.
>
> And since I very much doubt that upstream, Red Hat, SuSE, Canonical,
> etc., will ever agree on the right thing is, this is probably one of
> those things where we want to have a scheme where it is relatively
> easy for distributors to set their own policy, which may differ for a
> community versus enterprise distro kernel.
Perhaps, but a well-thought-out rationale for what combinations make
sense might just fly everywhere.
A complex scheme of configuration policy sounds like potentially another
layer of knobs on an already vast layer of knobs. ;)
(That said, distros may well want to just nuke things like data=writeback
from orbit)
(And that said, I'm of the opinion that upstream should too ;))
Anyway, back to my main point: As a guiding principle I think I would
say that mount -t ext3 with ext4.ko should hard-reject any option not
understood by ext3.ko. It's clear and predictable, and should make for
a decent first cut.
-Eric
> But in order to make sure we don't end up talking past each other, I
> think it's useful to be very explicit about why we want to do this,
> before we try to figure what's and the how's.
>
> - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 11/11/2012 05:16 PM, Eric Sandeen wrote:
> On 11/8/12 11:02 AM, Theodore Ts'o wrote:
>> On Thu, Nov 01, 2012 at 05:32:11PM -0200, Carlos Maiolino wrote:
>>> I have not added all mount options I believe need to raise a
>>> warning, just those with a flag set on superblock, but I expect to
>>> generate a discussion here about which mount options should be
>>> warned and get suggestions about how to deal with argumented options
>>> (ex: commit= opt), to be included in a V2 of this patch.
>> Let's take a step back, and see if we can be explicit about why it
>> would be useful to warn when a user uses mount options that we were
>> not present with the implementation of the file system drivers found
>> in fs/ext2 and fs/ext3. While we're at it, we can also examine the
>> same question with respect to file system features --- i.e., if
>> someone mounts a file system with an extent feature enabled using
>> mount -t ext2, what if anything should we do. What are we trying to
>> achieve, or conversely what are we trying to prevent?
>>
>> Suppose the user does something like this:
>>
>> mount -t ext3 -o delalloc /dev/sdb /u1
>>
>> Yes, the traditional ext3 driver code doesn't understand the delalloc
>> mount option, but what's the concern that is leading us to want to
>> print a warning message to the log (that the user may or may not even
>> see). Are we worried that what should happen is not sufficiently
>> well-defined? Are we worried that while it might work with a
>> particular kernel which is compiled a certain way, it might not work
>> with another kernel? Is it part of the larger question of wanting to
>> warn if the user is using a set of not-well-tested combination of
>> mount options and/or file system features?
>>
>> If it is the latter, what is the right approach? At Barcelona, I was
>> chatting with Ric Wheeler and Ralf Flaxa, and they had differing
>> opinions about what the right thing to do for features which are not
>> supported. Ralf felt that warning in the syslog might be sufficient.
>> I sugested setting a new kernel taint flag, to make it easier for the
>> supper desk to twig to the fact that the user was doing something
>> unusual. Ric offerred his opinion that it was better to hard-fail the
>> mount. And of course, this is with distro kernels; for the upstream
>> kernel, I think our goal is to (a) warn the user that they are doing
>> something unusual, and (b) ask them to tell us (at linux-ext4) what
>> they are doing and why, so we have a better understanding of what
>> users want, so we can either add it to our test matrix, or perhaps
>> warn the user off.
> The overarching reason is to cut down the test matrix if possible, I think.
> This has advantages for both distros & upstream, and might even lead
> to code simplification if we can exclude some corner-case behaviors.
This is my key goal - make sure that we get tested and sane mount options out to
the users. And we normally only test a handful of things, certainly it is not
reasonable to test all combinations of things you could do.
>
> But the other question that might need answering is this:
>
> If a user has asked ext4.ko to mount an ext3 device, should it behave
> as closely as possible to what the ext3.ko driver used to do? And I'd
> say yes. Therefore things like -t ext3 -o delalloc should be hard-failed.
>
> If you really want your old ext3 filesystem to be mounted with delalloc,
> then I think the right way would be to use mount -t ext4 /dev/ext3-dev.
> Then you get the runtime behaviors such as delalloc, and ext4 should
> not write anything which is not ext3-compatible.
>
> Perhaps we need to go through in fine detail, but I think mount -t
> ext3 should reject any options not applicable to ext3.ko. If you want
> the newer ext4 behaviors, then use mount -t ext4.
I agree with that. At some point, that is probably what the user actually meant
(just did not know it at the time :)).
>
>> So not only do we need to decide which mount options and file system
>> features we want to support, or warn against etc., it's also useful to
>> think about what we want to do when the user does something a little
>> out of the norm.
>>
>> And since I very much doubt that upstream, Red Hat, SuSE, Canonical,
>> etc., will ever agree on the right thing is, this is probably one of
>> those things where we want to have a scheme where it is relatively
>> easy for distributors to set their own policy, which may differ for a
>> community versus enterprise distro kernel.
> Perhaps, but a well-thought-out rationale for what combinations make
> sense might just fly everywhere.
>
> A complex scheme of configuration policy sounds like potentially another
> layer of knobs on an already vast layer of knobs. ;)
>
> (That said, distros may well want to just nuke things like data=writeback
> from orbit)
>
> (And that said, I'm of the opinion that upstream should too ;))
>
> Anyway, back to my main point: As a guiding principle I think I would
> say that mount -t ext3 with ext4.ko should hard-reject any option not
> understood by ext3.ko. It's clear and predictable, and should make for
> a decent first cut.
>
> -Eric
Agreed (sounds almost like we had coordinated our answers before I spoke to Ted
in Barcelona!).
Ric
>
>> But in order to make sure we don't end up talking past each other, I
>> think it's useful to be very explicit about why we want to do this,
>> before we try to figure what's and the how's.
>>
>>
Hi, sorry my delayed answer on this thread.
> >Anyway, back to my main point: As a guiding principle I think I would
> >say that mount -t ext3 with ext4.ko should hard-reject any option not
> >understood by ext3.ko. It's clear and predictable, and should make for
> >a decent first cut.
> >
> >-Eric
>
> Agreed (sounds almost like we had coordinated our answers before I
> spoke to Ted in Barcelona!).
>
> Ric
>
that's my main point, and the goal of my first patch. I was thinking in just
warn when mounting a ext3 with ext4.ko using mount options not recognized by
ext3.ko, but, looks like hard-reject these options looks more reasonable than
just warning.
I'm going to re-write the patch using this approach. Is there any concern in
take this direction from any part here?
--
Carlos
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.
Changelog:
V2 - Fail a mount instead of just warning
Signed-off-by: Carlos Maiolino <[email protected]>
---
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
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 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.
Honza
>
> Changelog:
> V2 - Fail a mount instead of just warning
>
> Signed-off-by: Carlos Maiolino <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <[email protected]>
SUSE Labs, CR
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 <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Carlos
On Wed 23-01-13 07:22:49, Carlos Maiolino wrote:
> 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.
If there are mount options that enable features, then these should be
disallowed for ext2/ext3 mounts. But I think we already got rid of these
traps on users...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hi guys, my apologies to have not touched this conversation for a while, got
busy with other stuff.
So, let me continue this.
On Wed, Jan 23, 2013 at 02:00:37PM +0100, Jan Kara wrote:
> On Wed 23-01-13 07:22:49, Carlos Maiolino wrote:
> > 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.
> If there are mount options that enable features, then these should be
> disallowed for ext2/ext3 mounts. But I think we already got rid of these
> traps on users...
>
> Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
Jan, I agree with the idea of check for incompat features and I can work on that
with no problem, but I still think we need to also ensure some mount options are
not allowed to be used with some specific ExtFS versions wheter it change the
disk format (we probably got rid of these as you said) or not. Not sure if my
previous (V2) to restrict mount options lacks something or need to be reworked
in some way (of course now it should to fit new kernel versions).
As an example, commit=<secs> option doesn't make sense to be used on ext2
filesystems, but you can mount an ext2 FS with this option with no problem.
--
Carlos
Ping?
On Thu, Oct 31, 2013 at 12:00:41PM -0200, Carlos Maiolino wrote:
> Hi guys, my apologies to have not touched this conversation for a while, got
> busy with other stuff.
>
> So, let me continue this.
>
> On Wed, Jan 23, 2013 at 02:00:37PM +0100, Jan Kara wrote:
> > On Wed 23-01-13 07:22:49, Carlos Maiolino wrote:
> > > 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.
> > If there are mount options that enable features, then these should be
> > disallowed for ext2/ext3 mounts. But I think we already got rid of these
> > traps on users...
> >
> > Honza
> > --
> > Jan Kara <[email protected]>
> > SUSE Labs, CR
>
> Jan, I agree with the idea of check for incompat features and I can work on that
> with no problem, but I still think we need to also ensure some mount options are
> not allowed to be used with some specific ExtFS versions wheter it change the
> disk format (we probably got rid of these as you said) or not. Not sure if my
> previous (V2) to restrict mount options lacks something or need to be reworked
> in some way (of course now it should to fit new kernel versions).
>
> As an example, commit=<secs> option doesn't make sense to be used on ext2
> filesystems, but you can mount an ext2 FS with this option with no problem.
>
>
> --
> Carlos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Carlos
Nevermind, I noticed Ted committed a patch with a different approach of this
issue.
On Tue, Nov 19, 2013 at 04:39:28PM -0200, Carlos Maiolino wrote:
> Ping?
>
> On Thu, Oct 31, 2013 at 12:00:41PM -0200, Carlos Maiolino wrote:
> > Hi guys, my apologies to have not touched this conversation for a while, got
> > busy with other stuff.
> >
> > So, let me continue this.
> >
> > On Wed, Jan 23, 2013 at 02:00:37PM +0100, Jan Kara wrote:
> > > On Wed 23-01-13 07:22:49, Carlos Maiolino wrote:
> > > > 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.
> > > If there are mount options that enable features, then these should be
> > > disallowed for ext2/ext3 mounts. But I think we already got rid of these
> > > traps on users...
> > >
> > > Honza
> > > --
> > > Jan Kara <[email protected]>
> > > SUSE Labs, CR
> >
> > Jan, I agree with the idea of check for incompat features and I can work on that
> > with no problem, but I still think we need to also ensure some mount options are
> > not allowed to be used with some specific ExtFS versions wheter it change the
> > disk format (we probably got rid of these as you said) or not. Not sure if my
> > previous (V2) to restrict mount options lacks something or need to be reworked
> > in some way (of course now it should to fit new kernel versions).
> >
> > As an example, commit=<secs> option doesn't make sense to be used on ext2
> > filesystems, but you can mount an ext2 FS with this option with no problem.
> >
> >
> > --
> > Carlos
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to [email protected]
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Carlos