[PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Theodore Ts'o]

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

Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Andreas Dilger]

[-- 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 --]

Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Ira Weiny]

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
> 

Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Theodore Y. Ts'o]

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

Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Ira Weiny]

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

Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Jan Kara]

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

Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Theodore Y. Ts'o]

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

Re: [PATCH] ext4: reject mount options not supported when remounting in handle_mount_opt()

[Ira Weiny]

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/