* [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() [not found] <to=00000000000098a5d505a34d1e48@google.com> @ 2020-04-15 17:48 ` Theodore Ts'o 2020-04-15 18:12 ` Andreas Dilger 2020-04-15 20:25 ` Ira Weiny 0 siblings, 2 replies; 8+ messages in thread From: Theodore Ts'o @ 2020-04-15 17:48 UTC (permalink / raw) To: Ext4 Developers List Cc: syzkaller-bugs, Theodore Ts'o, stable, syzbot+bca9799bf129256190da Rejecting the mount options in ext4_remount() means that some mount options would be enabled for a small amount of time, and then the mount option change would be reverted. In the case of "mount -o remount,dax", this can cause a race where files would temporarily treated as DAX --- and then not. Cc: stable@kernel.org Reported-and-tested-by: syzbot+bca9799bf129256190da@syzkaller.appspotmail.com Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/ext4/super.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index bf5fcb477f66..6fe32f9aa889 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1726,6 +1726,7 @@ static int clear_qf_name(struct super_block *sb, int qtype) #define MOPT_NO_EXT3 0x0200 #define MOPT_EXT4_ONLY (MOPT_NO_EXT2 | MOPT_NO_EXT3) #define MOPT_STRING 0x0400 +#define MOPT_NO_REMOUNT 0x0800 static const struct mount_opts { int token; @@ -1775,12 +1776,12 @@ static const struct mount_opts { {Opt_min_batch_time, 0, MOPT_GTE0}, {Opt_inode_readahead_blks, 0, MOPT_GTE0}, {Opt_init_itable, 0, MOPT_GTE0}, - {Opt_dax, EXT4_MOUNT_DAX, MOPT_SET}, + {Opt_dax, EXT4_MOUNT_DAX, MOPT_SET | MOPT_NO_REMOUNT}, {Opt_stripe, 0, MOPT_GTE0}, {Opt_resuid, 0, MOPT_GTE0}, {Opt_resgid, 0, MOPT_GTE0}, - {Opt_journal_dev, 0, MOPT_NO_EXT2 | MOPT_GTE0}, - {Opt_journal_path, 0, MOPT_NO_EXT2 | MOPT_STRING}, + {Opt_journal_dev, 0, MOPT_NO_EXT2 | MOPT_GTE0 | MOPT_NO_REMOUNT}, + {Opt_journal_path, 0, MOPT_NO_EXT2 | MOPT_STRING | MOPT_NO_REMOUNT}, {Opt_journal_ioprio, 0, MOPT_NO_EXT2 | MOPT_GTE0}, {Opt_data_journal, EXT4_MOUNT_JOURNAL_DATA, MOPT_NO_EXT2 | MOPT_DATAJ}, {Opt_data_ordered, EXT4_MOUNT_ORDERED_DATA, MOPT_NO_EXT2 | MOPT_DATAJ}, @@ -1817,7 +1818,7 @@ static const struct mount_opts { {Opt_jqfmt_vfsv1, QFMT_VFS_V1, MOPT_QFMT}, {Opt_max_dir_size_kb, 0, MOPT_GTE0}, {Opt_test_dummy_encryption, 0, MOPT_GTE0}, - {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, + {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET | MOPT_NO_REMOUNT}, {Opt_err, 0, 0} }; @@ -1915,6 +1916,12 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, "Mount option \"%s\" incompatible with ext3", opt); return -1; } + if ((m->flags & MOPT_NO_REMOUNT) && is_remount) { + ext4_msg(sb, KERN_ERR, + "Mount option \"%s\" not supported when remounting", + opt); + return -1; + } if (args->from && !(m->flags & MOPT_STRING) && match_int(args, &arg)) return -1; @@ -1994,11 +2001,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, } sbi->s_resgid = gid; } else if (token == Opt_journal_dev) { - if (is_remount) { - ext4_msg(sb, KERN_ERR, - "Cannot specify journal on remount"); - return -1; - } *journal_devnum = arg; } else if (token == Opt_journal_path) { char *journal_path; @@ -2006,11 +2008,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, struct path path; int error; - if (is_remount) { - ext4_msg(sb, KERN_ERR, - "Cannot specify journal on remount"); - return -1; - } journal_path = match_strdup(&args[0]); if (!journal_path) { ext4_msg(sb, KERN_ERR, "error: could not dup " @@ -5427,18 +5424,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data) } } - if ((sbi->s_mount_opt ^ old_opts.s_mount_opt) & EXT4_MOUNT_NO_MBCACHE) { - ext4_msg(sb, KERN_ERR, "can't enable nombcache during remount"); - err = -EINVAL; - goto restore_opts; - } - - if ((sbi->s_mount_opt ^ old_opts.s_mount_opt) & EXT4_MOUNT_DAX) { - ext4_msg(sb, KERN_WARNING, "warning: refusing change of " - "dax flag with busy inodes while remounting"); - sbi->s_mount_opt ^= EXT4_MOUNT_DAX; - } - if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED) ext4_abort(sb, EXT4_ERR_ESHUTDOWN, "Abort forced by user"); -- 2.24.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() 2020-04-15 17:48 ` [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() Theodore Ts'o @ 2020-04-15 18:12 ` Andreas Dilger 2020-04-15 20:25 ` Ira Weiny 1 sibling, 0 replies; 8+ messages in thread From: Andreas Dilger @ 2020-04-15 18:12 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Ext4 Developers List, stable [-- Attachment #1: Type: text/plain, Size: 4335 bytes --] On Apr 15, 2020, at 11:48 AM, Theodore Ts'o <tytso@mit.edu> wrote: > > Rejecting the mount options in ext4_remount() means that some mount > options would be enabled for a small amount of time, and then the > mount option change would be reverted. In the case of "mount -o > remount,dax", this can cause a race where files would temporarily > treated as DAX --- and then not. > > Cc: stable@kernel.org > Reported-and-tested-by: syzbot+bca9799bf129256190da@syzkaller.appspotmail.com > Signed-off-by: Theodore Ts'o <tytso@mit.edu> Reviewed-by: Andreas Dilger <adilger@dilger.ca> > --- > fs/ext4/super.c | 37 +++++++++++-------------------------- > 1 file changed, 11 insertions(+), 26 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index bf5fcb477f66..6fe32f9aa889 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1726,6 +1726,7 @@ static int clear_qf_name(struct super_block *sb, int qtype) > #define MOPT_NO_EXT3 0x0200 > #define MOPT_EXT4_ONLY (MOPT_NO_EXT2 | MOPT_NO_EXT3) > #define MOPT_STRING 0x0400 > +#define MOPT_NO_REMOUNT 0x0800 > > static const struct mount_opts { > int token; > @@ -1775,12 +1776,12 @@ static const struct mount_opts { > {Opt_min_batch_time, 0, MOPT_GTE0}, > {Opt_inode_readahead_blks, 0, MOPT_GTE0}, > {Opt_init_itable, 0, MOPT_GTE0}, > - {Opt_dax, EXT4_MOUNT_DAX, MOPT_SET}, > + {Opt_dax, EXT4_MOUNT_DAX, MOPT_SET | MOPT_NO_REMOUNT}, > {Opt_stripe, 0, MOPT_GTE0}, > {Opt_resuid, 0, MOPT_GTE0}, > {Opt_resgid, 0, MOPT_GTE0}, > - {Opt_journal_dev, 0, MOPT_NO_EXT2 | MOPT_GTE0}, > - {Opt_journal_path, 0, MOPT_NO_EXT2 | MOPT_STRING}, > + {Opt_journal_dev, 0, MOPT_NO_EXT2 | MOPT_GTE0 | MOPT_NO_REMOUNT}, > + {Opt_journal_path, 0, MOPT_NO_EXT2 | MOPT_STRING | MOPT_NO_REMOUNT}, > {Opt_journal_ioprio, 0, MOPT_NO_EXT2 | MOPT_GTE0}, > {Opt_data_journal, EXT4_MOUNT_JOURNAL_DATA, MOPT_NO_EXT2 | MOPT_DATAJ}, > {Opt_data_ordered, EXT4_MOUNT_ORDERED_DATA, MOPT_NO_EXT2 | MOPT_DATAJ}, > @@ -1817,7 +1818,7 @@ static const struct mount_opts { > {Opt_jqfmt_vfsv1, QFMT_VFS_V1, MOPT_QFMT}, > {Opt_max_dir_size_kb, 0, MOPT_GTE0}, > {Opt_test_dummy_encryption, 0, MOPT_GTE0}, > - {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, > + {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET | MOPT_NO_REMOUNT}, > {Opt_err, 0, 0} > }; > > @@ -1915,6 +1916,12 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, > "Mount option \"%s\" incompatible with ext3", opt); > return -1; > } > + if ((m->flags & MOPT_NO_REMOUNT) && is_remount) { > + ext4_msg(sb, KERN_ERR, > + "Mount option \"%s\" not supported when remounting", > + opt); > + return -1; > + } > > if (args->from && !(m->flags & MOPT_STRING) && match_int(args, &arg)) > return -1; > @@ -1994,11 +2001,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, > } > sbi->s_resgid = gid; > } else if (token == Opt_journal_dev) { > - if (is_remount) { > - ext4_msg(sb, KERN_ERR, > - "Cannot specify journal on remount"); > - return -1; > - } > *journal_devnum = arg; > } else if (token == Opt_journal_path) { > char *journal_path; > @@ -2006,11 +2008,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, > struct path path; > int error; > > - if (is_remount) { > - ext4_msg(sb, KERN_ERR, > - "Cannot specify journal on remount"); > - return -1; > - } > journal_path = match_strdup(&args[0]); > if (!journal_path) { > ext4_msg(sb, KERN_ERR, "error: could not dup " > @@ -5427,18 +5424,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data) > } > } > > - if ((sbi->s_mount_opt ^ old_opts.s_mount_opt) & EXT4_MOUNT_NO_MBCACHE) { > - ext4_msg(sb, KERN_ERR, "can't enable nombcache during remount"); > - err = -EINVAL; > - goto restore_opts; > - } > - > - if ((sbi->s_mount_opt ^ old_opts.s_mount_opt) & EXT4_MOUNT_DAX) { > - ext4_msg(sb, KERN_WARNING, "warning: refusing change of " > - "dax flag with busy inodes while remounting"); > - sbi->s_mount_opt ^= EXT4_MOUNT_DAX; > - } > - > if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED) > ext4_abort(sb, EXT4_ERR_ESHUTDOWN, "Abort forced by user"); > > -- > 2.24.1 > Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 873 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() 2020-04-15 17:48 ` [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() Theodore Ts'o 2020-04-15 18:12 ` Andreas Dilger @ 2020-04-15 20:25 ` Ira Weiny 2020-04-15 22:07 ` Theodore Y. Ts'o 1 sibling, 1 reply; 8+ messages in thread From: Ira Weiny @ 2020-04-15 20:25 UTC (permalink / raw) To: Theodore Ts'o Cc: Ext4 Developers List, syzkaller-bugs, stable, syzbot+bca9799bf129256190da On Wed, Apr 15, 2020 at 01:48:39PM -0400, Theodore Y. Ts'o wrote: > Rejecting the mount options in ext4_remount() means that some mount > options would be enabled for a small amount of time, and then the > mount option change would be reverted. In the case of "mount -o > remount,dax", this can cause a race where files would temporarily > treated as DAX --- and then not. > > Cc: stable@kernel.org > Reported-and-tested-by: syzbot+bca9799bf129256190da@syzkaller.appspotmail.com > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > fs/ext4/super.c | 37 +++++++++++-------------------------- > 1 file changed, 11 insertions(+), 26 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index bf5fcb477f66..6fe32f9aa889 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1726,6 +1726,7 @@ static int clear_qf_name(struct super_block *sb, int qtype) > #define MOPT_NO_EXT3 0x0200 > #define MOPT_EXT4_ONLY (MOPT_NO_EXT2 | MOPT_NO_EXT3) > #define MOPT_STRING 0x0400 > +#define MOPT_NO_REMOUNT 0x0800 > > static const struct mount_opts { > int token; > @@ -1775,12 +1776,12 @@ static const struct mount_opts { > {Opt_min_batch_time, 0, MOPT_GTE0}, > {Opt_inode_readahead_blks, 0, MOPT_GTE0}, > {Opt_init_itable, 0, MOPT_GTE0}, > - {Opt_dax, EXT4_MOUNT_DAX, MOPT_SET}, > + {Opt_dax, EXT4_MOUNT_DAX, MOPT_SET | MOPT_NO_REMOUNT}, > {Opt_stripe, 0, MOPT_GTE0}, > {Opt_resuid, 0, MOPT_GTE0}, > {Opt_resgid, 0, MOPT_GTE0}, > - {Opt_journal_dev, 0, MOPT_NO_EXT2 | MOPT_GTE0}, > - {Opt_journal_path, 0, MOPT_NO_EXT2 | MOPT_STRING}, > + {Opt_journal_dev, 0, MOPT_NO_EXT2 | MOPT_GTE0 | MOPT_NO_REMOUNT}, > + {Opt_journal_path, 0, MOPT_NO_EXT2 | MOPT_STRING | MOPT_NO_REMOUNT}, > {Opt_journal_ioprio, 0, MOPT_NO_EXT2 | MOPT_GTE0}, > {Opt_data_journal, EXT4_MOUNT_JOURNAL_DATA, MOPT_NO_EXT2 | MOPT_DATAJ}, > {Opt_data_ordered, EXT4_MOUNT_ORDERED_DATA, MOPT_NO_EXT2 | MOPT_DATAJ}, > @@ -1817,7 +1818,7 @@ static const struct mount_opts { > {Opt_jqfmt_vfsv1, QFMT_VFS_V1, MOPT_QFMT}, > {Opt_max_dir_size_kb, 0, MOPT_GTE0}, > {Opt_test_dummy_encryption, 0, MOPT_GTE0}, > - {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, > + {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET | MOPT_NO_REMOUNT}, > {Opt_err, 0, 0} > }; > > @@ -1915,6 +1916,12 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, > "Mount option \"%s\" incompatible with ext3", opt); > return -1; > } > + if ((m->flags & MOPT_NO_REMOUNT) && is_remount) { > + ext4_msg(sb, KERN_ERR, > + "Mount option \"%s\" not supported when remounting", > + opt); > + return -1; > + } > > if (args->from && !(m->flags & MOPT_STRING) && match_int(args, &arg)) > return -1; > @@ -1994,11 +2001,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, > } > sbi->s_resgid = gid; > } else if (token == Opt_journal_dev) { > - if (is_remount) { > - ext4_msg(sb, KERN_ERR, > - "Cannot specify journal on remount"); > - return -1; > - } > *journal_devnum = arg; > } else if (token == Opt_journal_path) { > char *journal_path; > @@ -2006,11 +2008,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, > struct path path; > int error; > > - if (is_remount) { > - ext4_msg(sb, KERN_ERR, > - "Cannot specify journal on remount"); > - return -1; > - } > journal_path = match_strdup(&args[0]); > if (!journal_path) { > ext4_msg(sb, KERN_ERR, "error: could not dup " > @@ -5427,18 +5424,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data) > } > } > > - if ((sbi->s_mount_opt ^ old_opts.s_mount_opt) & EXT4_MOUNT_NO_MBCACHE) { > - ext4_msg(sb, KERN_ERR, "can't enable nombcache during remount"); > - err = -EINVAL; > - goto restore_opts; > - } > - > - if ((sbi->s_mount_opt ^ old_opts.s_mount_opt) & EXT4_MOUNT_DAX) { > - ext4_msg(sb, KERN_WARNING, "warning: refusing change of " > - "dax flag with busy inodes while remounting"); > - sbi->s_mount_opt ^= EXT4_MOUNT_DAX; > - } I like the simplification but I'm confused... This fundamentally changes the behavior from forcing the dax mode to be the same across the remount to only failing if we are going from non-dax to dax, adding -o dax on the remount? But going from -o dax to 'not -o dax' would be ok? FWIW after thinking about it some I _think_ it would be ok to allow the dax mode to change on a remount and let the inodes in memory stay in the mode they are at. And newly loaded inodes would get the new mode... Unfortunately without the STATX patch I have proposed the user does not have any way of knowing which files are in which mode. Ira > - > if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED) > ext4_abort(sb, EXT4_ERR_ESHUTDOWN, "Abort forced by user"); > > -- > 2.24.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() 2020-04-15 20:25 ` Ira Weiny @ 2020-04-15 22:07 ` Theodore Y. Ts'o 2020-04-16 5:23 ` Ira Weiny 0 siblings, 1 reply; 8+ messages in thread From: Theodore Y. Ts'o @ 2020-04-15 22:07 UTC (permalink / raw) To: Ira Weiny Cc: Ext4 Developers List, syzkaller-bugs, stable, syzbot+bca9799bf129256190da On Wed, Apr 15, 2020 at 01:25:37PM -0700, Ira Weiny wrote: > This fundamentally changes the behavior from forcing the dax mode to be the > same across the remount to only failing if we are going from non-dax to dax, > adding -o dax on the remount? > > But going from -o dax to 'not -o dax' would be ok? > > FWIW after thinking about it some I _think_ it would be ok to allow the dax > mode to change on a remount and let the inodes in memory stay in the mode they > are at. And newly loaded inodes would get the new mode... Unfortunately > without the STATX patch I have proposed the user does not have any way of > knowing which files are in which mode. We don't currently support mount -o nodax. So the intention of the current code is that the dax mode can't change in either direction (enabling or disabling) as a remount option. The syzkaller report was because changing dax mode racing with other operations given the current code base, could result in a kernel OOPS. So we *do* need to rule it out at least for now. I certainly don't object to allowing changing dax mode as a remount --- so long as we have tests to make sure that if we stress opening, reading, writing, mmap'ing files, etc., while another thread is flipping back and forth between dax=never and dax=always is mount -o remount --- and make sure that we don't end up crashing. And this test needs to be in xfstests, because trying to figure out what triggers a syzkaller failures in file system land is a pain in the *ss so we really want a dedicated xfstests for this case. Have you tested your patch series to make sure we don't have some potential races here? - Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() 2020-04-15 22:07 ` Theodore Y. Ts'o @ 2020-04-16 5:23 ` Ira Weiny 2020-04-22 16:10 ` Jan Kara 0 siblings, 1 reply; 8+ messages in thread From: Ira Weiny @ 2020-04-16 5:23 UTC (permalink / raw) To: Theodore Y. Ts'o Cc: Ext4 Developers List, syzkaller-bugs, stable, syzbot+bca9799bf129256190da On Wed, Apr 15, 2020 at 06:07:52PM -0400, Theodore Y. Ts'o wrote: > On Wed, Apr 15, 2020 at 01:25:37PM -0700, Ira Weiny wrote: > > This fundamentally changes the behavior from forcing the dax mode to be the > > same across the remount to only failing if we are going from non-dax to dax, > > adding -o dax on the remount? > > > > But going from -o dax to 'not -o dax' would be ok? > > > > FWIW after thinking about it some I _think_ it would be ok to allow the dax > > mode to change on a remount and let the inodes in memory stay in the mode they > > are at. And newly loaded inodes would get the new mode... Unfortunately > > without the STATX patch I have proposed the user does not have any way of > > knowing which files are in which mode. > > We don't currently support mount -o nodax. But we do support not supplying the option which means 'nodax' right? > So the intention of the > current code is that the dax mode can't change in either direction > (enabling or disabling) as a remount option. > > The syzkaller report was because changing dax mode racing with other > operations given the current code base, could result in a kernel OOPS. > So we *do* need to rule it out at least for now. But does this new patch prevent a dax change from '-o dax' to not specifying the option? I admit this option parsing code is confusing me. So I might be missing it completely. > > I certainly don't object to allowing changing dax mode as a remount > --- so long as we have tests to make sure that if we stress opening, > reading, writing, mmap'ing files, etc., while another thread is > flipping back and forth between dax=never and dax=always is mount -o > remount --- and make sure that we don't end up crashing. > > And this test needs to be in xfstests, because trying to figure out > what triggers a syzkaller failures in file system land is a pain in > the *ss so we really want a dedicated xfstests for this case. Agreed. > Have > you tested your patch series to make sure we don't have some potential > races here? No, I've not anticipated the potential of this until today... :-D Ira ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() 2020-04-16 5:23 ` Ira Weiny @ 2020-04-22 16:10 ` Jan Kara 2020-05-14 14:34 ` Theodore Y. Ts'o 0 siblings, 1 reply; 8+ messages in thread From: Jan Kara @ 2020-04-22 16:10 UTC (permalink / raw) To: Ira Weiny Cc: Theodore Y. Ts'o, Ext4 Developers List, syzkaller-bugs, stable, syzbot+bca9799bf129256190da On Wed 15-04-20 22:23:52, Ira Weiny wrote: > On Wed, Apr 15, 2020 at 06:07:52PM -0400, Theodore Y. Ts'o wrote: > > On Wed, Apr 15, 2020 at 01:25:37PM -0700, Ira Weiny wrote: > > > This fundamentally changes the behavior from forcing the dax mode to be the > > > same across the remount to only failing if we are going from non-dax to dax, > > > adding -o dax on the remount? > > > > > > But going from -o dax to 'not -o dax' would be ok? > > > > > > FWIW after thinking about it some I _think_ it would be ok to allow the dax > > > mode to change on a remount and let the inodes in memory stay in the mode they > > > are at. And newly loaded inodes would get the new mode... Unfortunately > > > without the STATX patch I have proposed the user does not have any way of > > > knowing which files are in which mode. > > > > We don't currently support mount -o nodax. > > But we do support not supplying the option which means 'nodax' right? Yeah, I second what Ira wrote. The new code does not seem to properly detect a case when enabled mount option is removed for remount and thus the feature would get disabled during remount as a result... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() 2020-04-22 16:10 ` Jan Kara @ 2020-05-14 14:34 ` Theodore Y. Ts'o 2020-05-16 1:49 ` Ira Weiny 0 siblings, 1 reply; 8+ messages in thread From: Theodore Y. Ts'o @ 2020-05-14 14:34 UTC (permalink / raw) To: Jan Kara Cc: Ira Weiny, Ext4 Developers List, syzkaller-bugs, stable, syzbot+bca9799bf129256190da On Wed, Apr 22, 2020 at 06:10:29PM +0200, Jan Kara wrote: > On Wed 15-04-20 22:23:52, Ira Weiny wrote: > > On Wed, Apr 15, 2020 at 06:07:52PM -0400, Theodore Y. Ts'o wrote: > > > On Wed, Apr 15, 2020 at 01:25:37PM -0700, Ira Weiny wrote: > > > > This fundamentally changes the behavior from forcing the dax mode to be the > > > > same across the remount to only failing if we are going from non-dax to dax, > > > > adding -o dax on the remount? > > > > > > > > But going from -o dax to 'not -o dax' would be ok? > > > > > > > > FWIW after thinking about it some I _think_ it would be ok to allow the dax > > > > mode to change on a remount and let the inodes in memory stay in the mode they > > > > are at. And newly loaded inodes would get the new mode... Unfortunately > > > > without the STATX patch I have proposed the user does not have any way of > > > > knowing which files are in which mode. > > > > > > We don't currently support mount -o nodax. > > > > But we do support not supplying the option which means 'nodax' right? > > Yeah, I second what Ira wrote. The new code does not seem to properly > detect a case when enabled mount option is removed for remount and thus the > feature would get disabled during remount as a result... Sorry for not responding earlier. The way ext4 remounting working is not supplying an mount option which toggles a switch means that we don't change its current setting. For example, if you mount with, say dioread_nolock, if you don't specify it when remounting, the current setting of dioread_nolock remains the same. If you want to change it, you need to specify the mount option nodioread_nolock. The change is true for discard vs nodiscard, etc. We currently don't have nodax at all, which means that once dax is set, there is no way to unset the dax mount option. This was deliberate, because I was aware that the dax->no dax transition would result in badness. Cheers, - Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() 2020-05-14 14:34 ` Theodore Y. Ts'o @ 2020-05-16 1:49 ` Ira Weiny 0 siblings, 0 replies; 8+ messages in thread From: Ira Weiny @ 2020-05-16 1:49 UTC (permalink / raw) To: Theodore Y. Ts'o Cc: Jan Kara, Ext4 Developers List, syzkaller-bugs, stable, syzbot+bca9799bf129256190da On Thu, May 14, 2020 at 10:34:09AM -0400, Theodore Y. Ts'o wrote: > On Wed, Apr 22, 2020 at 06:10:29PM +0200, Jan Kara wrote: > > On Wed 15-04-20 22:23:52, Ira Weiny wrote: > > > On Wed, Apr 15, 2020 at 06:07:52PM -0400, Theodore Y. Ts'o wrote: > > > > On Wed, Apr 15, 2020 at 01:25:37PM -0700, Ira Weiny wrote: > > > > > This fundamentally changes the behavior from forcing the dax mode to be the > > > > > same across the remount to only failing if we are going from non-dax to dax, > > > > > adding -o dax on the remount? > > > > > > > > > > But going from -o dax to 'not -o dax' would be ok? > > > > > > > > > > FWIW after thinking about it some I _think_ it would be ok to allow the dax > > > > > mode to change on a remount and let the inodes in memory stay in the mode they > > > > > are at. And newly loaded inodes would get the new mode... Unfortunately > > > > > without the STATX patch I have proposed the user does not have any way of > > > > > knowing which files are in which mode. > > > > > > > > We don't currently support mount -o nodax. > > > > > > But we do support not supplying the option which means 'nodax' right? > > > > Yeah, I second what Ira wrote. The new code does not seem to properly > > detect a case when enabled mount option is removed for remount and thus the > > feature would get disabled during remount as a result... > > Sorry for not responding earlier. The way ext4 remounting working is > not supplying an mount option which toggles a switch means that we > don't change its current setting. > > For example, if you mount with, say dioread_nolock, if you don't > specify it when remounting, the current setting of dioread_nolock > remains the same. If you want to change it, you need to specify the > mount option nodioread_nolock. The change is true for discard vs > nodiscard, etc. > > We currently don't have nodax at all, which means that once dax is > set, there is no way to unset the dax mount option. This was > deliberate, because I was aware that the dax->no dax transition would > result in badness. At this point I'm not sure if it was working correctly before or not. I did keep this thread in mind and did a bit more testing on the latest version of the new DAX mount option parsing[1]. I have verified that whatever state (always, never, inode) dax was in, a remount does not affect it and the warning is printed. Furthermore I think the lead patches disabling verity and encryption[2] in that series should help, if not fix, this bug. Ted, do you think this series can make 5.8? The prelim patches [2] could be marked stable if you think they help without the rework of the mount option. Thanks, Ira [1] https://lore.kernel.org/lkml/20200515093224.GI9569@quack2.suse.cz/ [2] https://lore.kernel.org/lkml/20200515044121.2987940-3-ira.weiny@intel.com/ https://lore.kernel.org/lkml/20200515044121.2987940-4-ira.weiny@intel.com/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-05-16 1:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <to=00000000000098a5d505a34d1e48@google.com> 2020-04-15 17:48 ` [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt() Theodore Ts'o 2020-04-15 18:12 ` Andreas Dilger 2020-04-15 20:25 ` Ira Weiny 2020-04-15 22:07 ` Theodore Y. Ts'o 2020-04-16 5:23 ` Ira Weiny 2020-04-22 16:10 ` Jan Kara 2020-05-14 14:34 ` Theodore Y. Ts'o 2020-05-16 1:49 ` Ira Weiny
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).