[-v3] ext4: fix use-after-free race in ext4_remount()'s error path

Message ID 20181007030706.9094-1-tytso@mit.edu
State Accepted
Headers show
Series
  • [-v3] ext4: fix use-after-free race in ext4_remount()'s error path
Related show

Commit Message

Theodore Y. Ts'o Oct. 7, 2018, 3:07 a.m.
It's possible for ext4_show_quota_options() to try reading
s_qf_names[i] while it is being modified by ext4_remount() --- most
notably, in ext4_remount's error path when the original values of the
quota file name gets restored.

Reported-by: syzbot+a2872d6feea6918008a9@syzkaller.appspotmail.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@kernel.org
---

v3: RCU clean ups and fix ups
v2: Use RCU to avoid lock dependency problems

 fs/ext4/ext4.h  |  3 +-
 fs/ext4/super.c | 73 ++++++++++++++++++++++++++++++++-----------------
 2 files changed, 50 insertions(+), 26 deletions(-)

Comments

Paul E. McKenney Oct. 9, 2018, 5:30 a.m. | #1
On Sat, Oct 06, 2018 at 11:07:06PM -0400, Theodore Ts'o wrote:
> It's possible for ext4_show_quota_options() to try reading
> s_qf_names[i] while it is being modified by ext4_remount() --- most
> notably, in ext4_remount's error path when the original values of the
> quota file name gets restored.
> 
> Reported-by: syzbot+a2872d6feea6918008a9@syzkaller.appspotmail.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@kernel.org

Looks good to me!

Acked-by: Paul E. McKenney <paulmck@linux.ibm.com>

> ---
> 
> v3: RCU clean ups and fix ups
> v2: Use RCU to avoid lock dependency problems
> 
>  fs/ext4/ext4.h  |  3 +-
>  fs/ext4/super.c | 73 ++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 86e1bacac757..12f90d48ba61 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1405,7 +1405,8 @@ struct ext4_sb_info {
>  	u32 s_min_batch_time;
>  	struct block_device *journal_bdev;
>  #ifdef CONFIG_QUOTA
> -	char *s_qf_names[EXT4_MAXQUOTAS];	/* Names of quota files with journalled quota */
> +	/* Names of quota files with journalled quota */
> +	char __rcu *s_qf_names[EXT4_MAXQUOTAS];
>  	int s_jquota_fmt;			/* Format of quota to use */
>  #endif
>  	unsigned int s_want_extra_isize; /* New inodes should reserve # bytes */
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index faf293ed8060..1909fbb28583 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -920,6 +920,18 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
>  }
>  #endif
>  
> +/*
> + * This is a helper function which is used in the mount/remount
> + * codepaths (which holds s_umount) to fetch the quota file name.
> + */
> +static inline char *get_qf_name(struct super_block *sb,
> +				struct ext4_sb_info *sbi,
> +				int type)
> +{
> +	return rcu_dereference_protected(sbi->s_qf_names[type],
> +					 lockdep_is_held(&sb->s_umount));
> +}
> +
>  static void ext4_put_super(struct super_block *sb)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> @@ -965,7 +977,7 @@ static void ext4_put_super(struct super_block *sb)
>  	percpu_free_rwsem(&sbi->s_journal_flag_rwsem);
>  #ifdef CONFIG_QUOTA
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> -		kfree(sbi->s_qf_names[i]);
> +		kfree(get_qf_name(sb, sbi, i));
>  #endif
>  
>  	/* Debugging code just in case the in-memory inode orphan list
> @@ -1531,11 +1543,10 @@ static const char deprecated_msg[] =
>  static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> -	char *qname;
> +	char *qname, *old_qname = get_qf_name(sb, sbi, qtype);
>  	int ret = -1;
>  
> -	if (sb_any_quota_loaded(sb) &&
> -		!sbi->s_qf_names[qtype]) {
> +	if (sb_any_quota_loaded(sb) && !old_qname) {
>  		ext4_msg(sb, KERN_ERR,
>  			"Cannot change journaled "
>  			"quota options when quota turned on");
> @@ -1552,8 +1563,8 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  			"Not enough memory for storing quotafile name");
>  		return -1;
>  	}
> -	if (sbi->s_qf_names[qtype]) {
> -		if (strcmp(sbi->s_qf_names[qtype], qname) == 0)
> +	if (old_qname) {
> +		if (strcmp(old_qname, qname) == 0)
>  			ret = 1;
>  		else
>  			ext4_msg(sb, KERN_ERR,
> @@ -1566,7 +1577,7 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  			"quotafile must be on filesystem root");
>  		goto errout;
>  	}
> -	sbi->s_qf_names[qtype] = qname;
> +	rcu_assign_pointer(sbi->s_qf_names[qtype], qname);
>  	set_opt(sb, QUOTA);
>  	return 1;
>  errout:
> @@ -1578,15 +1589,16 @@ static int clear_qf_name(struct super_block *sb, int qtype)
>  {
>  
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	char *old_qname = get_qf_name(sb, sbi, qtype);
>  
> -	if (sb_any_quota_loaded(sb) &&
> -		sbi->s_qf_names[qtype]) {
> +	if (sb_any_quota_loaded(sb) && old_qname) {
>  		ext4_msg(sb, KERN_ERR, "Cannot change journaled quota options"
>  			" when quota turned on");
>  		return -1;
>  	}
> -	kfree(sbi->s_qf_names[qtype]);
> -	sbi->s_qf_names[qtype] = NULL;
> +	rcu_assign_pointer(sbi->s_qf_names[qtype], NULL);
> +	synchronize_rcu();
> +	kfree(old_qname);
>  	return 1;
>  }
>  #endif
> @@ -1961,7 +1973,7 @@ static int parse_options(char *options, struct super_block *sb,
>  			 int is_remount)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> -	char *p;
> +	char *p, *usr_qf_name, *grp_qf_name;
>  	substring_t args[MAX_OPT_ARGS];
>  	int token;
>  
> @@ -1992,11 +2004,13 @@ static int parse_options(char *options, struct super_block *sb,
>  			 "Cannot enable project quota enforcement.");
>  		return 0;
>  	}
> -	if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
> -		if (test_opt(sb, USRQUOTA) && sbi->s_qf_names[USRQUOTA])
> +	usr_qf_name = get_qf_name(sb, sbi, USRQUOTA);
> +	grp_qf_name = get_qf_name(sb, sbi, GRPQUOTA);
> +	if (usr_qf_name || grp_qf_name) {
> +		if (test_opt(sb, USRQUOTA) && usr_qf_name)
>  			clear_opt(sb, USRQUOTA);
>  
> -		if (test_opt(sb, GRPQUOTA) && sbi->s_qf_names[GRPQUOTA])
> +		if (test_opt(sb, GRPQUOTA) && grp_qf_name)
>  			clear_opt(sb, GRPQUOTA);
>  
>  		if (test_opt(sb, GRPQUOTA) || test_opt(sb, USRQUOTA)) {
> @@ -2030,6 +2044,7 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
>  {
>  #if defined(CONFIG_QUOTA)
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	char *usr_qf_name, *grp_qf_name;
>  
>  	if (sbi->s_jquota_fmt) {
>  		char *fmtname = "";
> @@ -2048,11 +2063,14 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
>  		seq_printf(seq, ",jqfmt=%s", fmtname);
>  	}
>  
> -	if (sbi->s_qf_names[USRQUOTA])
> -		seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
> -
> -	if (sbi->s_qf_names[GRPQUOTA])
> -		seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
> +	rcu_read_lock();
> +	usr_qf_name = rcu_dereference(sbi->s_qf_names[USRQUOTA]);
> +	grp_qf_name = rcu_dereference(sbi->s_qf_names[GRPQUOTA]);
> +	if (usr_qf_name)
> +		seq_show_option(seq, "usrjquota", usr_qf_name);
> +	if (grp_qf_name)
> +		seq_show_option(seq, "grpjquota", grp_qf_name);
> +	rcu_read_unlock();
>  #endif
>  }
>  
> @@ -5104,6 +5122,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  	int err = 0;
>  #ifdef CONFIG_QUOTA
>  	int i, j;
> +	char *to_free[EXT4_MAXQUOTAS];
>  #endif
>  	char *orig_data = kstrdup(data, GFP_KERNEL);
>  
> @@ -5123,8 +5142,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  	old_opts.s_jquota_fmt = sbi->s_jquota_fmt;
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++)
>  		if (sbi->s_qf_names[i]) {
> -			old_opts.s_qf_names[i] = kstrdup(sbi->s_qf_names[i],
> -							 GFP_KERNEL);
> +			char *qf_name = get_qf_name(sb, sbi, i);
> +
> +			old_opts.s_qf_names[i] = kstrdup(qf_name, GFP_KERNEL);
>  			if (!old_opts.s_qf_names[i]) {
>  				for (j = 0; j < i; j++)
>  					kfree(old_opts.s_qf_names[j]);
> @@ -5353,9 +5373,12 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  #ifdef CONFIG_QUOTA
>  	sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++) {
> -		kfree(sbi->s_qf_names[i]);
> -		sbi->s_qf_names[i] = old_opts.s_qf_names[i];
> +		to_free[i] = get_qf_name(sb, sbi, i);
> +		rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
>  	}
> +	synchronize_rcu();
> +	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> +		kfree(to_free[i]);
>  #endif
>  	kfree(orig_data);
>  	return err;
> @@ -5546,7 +5569,7 @@ static int ext4_write_info(struct super_block *sb, int type)
>   */
>  static int ext4_quota_on_mount(struct super_block *sb, int type)
>  {
> -	return dquot_quota_on_mount(sb, EXT4_SB(sb)->s_qf_names[type],
> +	return dquot_quota_on_mount(sb, get_qf_name(sb, EXT4_SB(sb), type),
>  					EXT4_SB(sb)->s_jquota_fmt, type);
>  }
>  
> -- 
> 2.18.0.rc0
>
Jan Kara Oct. 11, 2018, 10:28 a.m. | #2
On Sat 06-10-18 23:07:06, Theodore Ts'o wrote:
> It's possible for ext4_show_quota_options() to try reading
> s_qf_names[i] while it is being modified by ext4_remount() --- most
> notably, in ext4_remount's error path when the original values of the
> quota file name gets restored.
> 
> Reported-by: syzbot+a2872d6feea6918008a9@syzkaller.appspotmail.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@kernel.org

Well, honestly the fact that ->show_options can be called while remount is
changing stuff under you looks problematic to me and I bet ext4 is not the
only one that would have issues with that. So I believe we might be better
off with just synchronizing ->show_options with umount / remount properly.
What were the lock dependency problems that made you switch to use RCU?

								Honza

> ---
> 
> v3: RCU clean ups and fix ups
> v2: Use RCU to avoid lock dependency problems
> 
>  fs/ext4/ext4.h  |  3 +-
>  fs/ext4/super.c | 73 ++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 86e1bacac757..12f90d48ba61 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1405,7 +1405,8 @@ struct ext4_sb_info {
>  	u32 s_min_batch_time;
>  	struct block_device *journal_bdev;
>  #ifdef CONFIG_QUOTA
> -	char *s_qf_names[EXT4_MAXQUOTAS];	/* Names of quota files with journalled quota */
> +	/* Names of quota files with journalled quota */
> +	char __rcu *s_qf_names[EXT4_MAXQUOTAS];
>  	int s_jquota_fmt;			/* Format of quota to use */
>  #endif
>  	unsigned int s_want_extra_isize; /* New inodes should reserve # bytes */
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index faf293ed8060..1909fbb28583 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -920,6 +920,18 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
>  }
>  #endif
>  
> +/*
> + * This is a helper function which is used in the mount/remount
> + * codepaths (which holds s_umount) to fetch the quota file name.
> + */
> +static inline char *get_qf_name(struct super_block *sb,
> +				struct ext4_sb_info *sbi,
> +				int type)
> +{
> +	return rcu_dereference_protected(sbi->s_qf_names[type],
> +					 lockdep_is_held(&sb->s_umount));
> +}
> +
>  static void ext4_put_super(struct super_block *sb)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> @@ -965,7 +977,7 @@ static void ext4_put_super(struct super_block *sb)
>  	percpu_free_rwsem(&sbi->s_journal_flag_rwsem);
>  #ifdef CONFIG_QUOTA
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> -		kfree(sbi->s_qf_names[i]);
> +		kfree(get_qf_name(sb, sbi, i));
>  #endif
>  
>  	/* Debugging code just in case the in-memory inode orphan list
> @@ -1531,11 +1543,10 @@ static const char deprecated_msg[] =
>  static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> -	char *qname;
> +	char *qname, *old_qname = get_qf_name(sb, sbi, qtype);
>  	int ret = -1;
>  
> -	if (sb_any_quota_loaded(sb) &&
> -		!sbi->s_qf_names[qtype]) {
> +	if (sb_any_quota_loaded(sb) && !old_qname) {
>  		ext4_msg(sb, KERN_ERR,
>  			"Cannot change journaled "
>  			"quota options when quota turned on");
> @@ -1552,8 +1563,8 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  			"Not enough memory for storing quotafile name");
>  		return -1;
>  	}
> -	if (sbi->s_qf_names[qtype]) {
> -		if (strcmp(sbi->s_qf_names[qtype], qname) == 0)
> +	if (old_qname) {
> +		if (strcmp(old_qname, qname) == 0)
>  			ret = 1;
>  		else
>  			ext4_msg(sb, KERN_ERR,
> @@ -1566,7 +1577,7 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  			"quotafile must be on filesystem root");
>  		goto errout;
>  	}
> -	sbi->s_qf_names[qtype] = qname;
> +	rcu_assign_pointer(sbi->s_qf_names[qtype], qname);
>  	set_opt(sb, QUOTA);
>  	return 1;
>  errout:
> @@ -1578,15 +1589,16 @@ static int clear_qf_name(struct super_block *sb, int qtype)
>  {
>  
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	char *old_qname = get_qf_name(sb, sbi, qtype);
>  
> -	if (sb_any_quota_loaded(sb) &&
> -		sbi->s_qf_names[qtype]) {
> +	if (sb_any_quota_loaded(sb) && old_qname) {
>  		ext4_msg(sb, KERN_ERR, "Cannot change journaled quota options"
>  			" when quota turned on");
>  		return -1;
>  	}
> -	kfree(sbi->s_qf_names[qtype]);
> -	sbi->s_qf_names[qtype] = NULL;
> +	rcu_assign_pointer(sbi->s_qf_names[qtype], NULL);
> +	synchronize_rcu();
> +	kfree(old_qname);
>  	return 1;
>  }
>  #endif
> @@ -1961,7 +1973,7 @@ static int parse_options(char *options, struct super_block *sb,
>  			 int is_remount)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> -	char *p;
> +	char *p, *usr_qf_name, *grp_qf_name;
>  	substring_t args[MAX_OPT_ARGS];
>  	int token;
>  
> @@ -1992,11 +2004,13 @@ static int parse_options(char *options, struct super_block *sb,
>  			 "Cannot enable project quota enforcement.");
>  		return 0;
>  	}
> -	if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
> -		if (test_opt(sb, USRQUOTA) && sbi->s_qf_names[USRQUOTA])
> +	usr_qf_name = get_qf_name(sb, sbi, USRQUOTA);
> +	grp_qf_name = get_qf_name(sb, sbi, GRPQUOTA);
> +	if (usr_qf_name || grp_qf_name) {
> +		if (test_opt(sb, USRQUOTA) && usr_qf_name)
>  			clear_opt(sb, USRQUOTA);
>  
> -		if (test_opt(sb, GRPQUOTA) && sbi->s_qf_names[GRPQUOTA])
> +		if (test_opt(sb, GRPQUOTA) && grp_qf_name)
>  			clear_opt(sb, GRPQUOTA);
>  
>  		if (test_opt(sb, GRPQUOTA) || test_opt(sb, USRQUOTA)) {
> @@ -2030,6 +2044,7 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
>  {
>  #if defined(CONFIG_QUOTA)
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	char *usr_qf_name, *grp_qf_name;
>  
>  	if (sbi->s_jquota_fmt) {
>  		char *fmtname = "";
> @@ -2048,11 +2063,14 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
>  		seq_printf(seq, ",jqfmt=%s", fmtname);
>  	}
>  
> -	if (sbi->s_qf_names[USRQUOTA])
> -		seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
> -
> -	if (sbi->s_qf_names[GRPQUOTA])
> -		seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
> +	rcu_read_lock();
> +	usr_qf_name = rcu_dereference(sbi->s_qf_names[USRQUOTA]);
> +	grp_qf_name = rcu_dereference(sbi->s_qf_names[GRPQUOTA]);
> +	if (usr_qf_name)
> +		seq_show_option(seq, "usrjquota", usr_qf_name);
> +	if (grp_qf_name)
> +		seq_show_option(seq, "grpjquota", grp_qf_name);
> +	rcu_read_unlock();
>  #endif
>  }
>  
> @@ -5104,6 +5122,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  	int err = 0;
>  #ifdef CONFIG_QUOTA
>  	int i, j;
> +	char *to_free[EXT4_MAXQUOTAS];
>  #endif
>  	char *orig_data = kstrdup(data, GFP_KERNEL);
>  
> @@ -5123,8 +5142,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  	old_opts.s_jquota_fmt = sbi->s_jquota_fmt;
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++)
>  		if (sbi->s_qf_names[i]) {
> -			old_opts.s_qf_names[i] = kstrdup(sbi->s_qf_names[i],
> -							 GFP_KERNEL);
> +			char *qf_name = get_qf_name(sb, sbi, i);
> +
> +			old_opts.s_qf_names[i] = kstrdup(qf_name, GFP_KERNEL);
>  			if (!old_opts.s_qf_names[i]) {
>  				for (j = 0; j < i; j++)
>  					kfree(old_opts.s_qf_names[j]);
> @@ -5353,9 +5373,12 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  #ifdef CONFIG_QUOTA
>  	sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++) {
> -		kfree(sbi->s_qf_names[i]);
> -		sbi->s_qf_names[i] = old_opts.s_qf_names[i];
> +		to_free[i] = get_qf_name(sb, sbi, i);
> +		rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
>  	}
> +	synchronize_rcu();
> +	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> +		kfree(to_free[i]);
>  #endif
>  	kfree(orig_data);
>  	return err;
> @@ -5546,7 +5569,7 @@ static int ext4_write_info(struct super_block *sb, int type)
>   */
>  static int ext4_quota_on_mount(struct super_block *sb, int type)
>  {
> -	return dquot_quota_on_mount(sb, EXT4_SB(sb)->s_qf_names[type],
> +	return dquot_quota_on_mount(sb, get_qf_name(sb, EXT4_SB(sb), type),
>  					EXT4_SB(sb)->s_jquota_fmt, type);
>  }
>  
> -- 
> 2.18.0.rc0
>
Paul E. McKenney Oct. 11, 2018, 3:29 p.m. | #3
On Thu, Oct 11, 2018 at 12:28:00PM +0200, Jan Kara wrote:
> On Sat 06-10-18 23:07:06, Theodore Ts'o wrote:
> > It's possible for ext4_show_quota_options() to try reading
> > s_qf_names[i] while it is being modified by ext4_remount() --- most
> > notably, in ext4_remount's error path when the original values of the
> > quota file name gets restored.
> > 
> > Reported-by: syzbot+a2872d6feea6918008a9@syzkaller.appspotmail.com
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Cc: stable@kernel.org
> 
> Well, honestly the fact that ->show_options can be called while remount is
> changing stuff under you looks problematic to me and I bet ext4 is not the
> only one that would have issues with that. So I believe we might be better
> off with just synchronizing ->show_options with umount / remount properly.
> What were the lock dependency problems that made you switch to use RCU?

OK, I will bite...

Ted's patch does in fact just properly synchronize ->show_options with
umount / remount.  Using RCU.  ;-)

							Thanx, Paul

> 								Honza
> 
> > ---
> > 
> > v3: RCU clean ups and fix ups
> > v2: Use RCU to avoid lock dependency problems
> > 
> >  fs/ext4/ext4.h  |  3 +-
> >  fs/ext4/super.c | 73 ++++++++++++++++++++++++++++++++-----------------
> >  2 files changed, 50 insertions(+), 26 deletions(-)
> > 
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 86e1bacac757..12f90d48ba61 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -1405,7 +1405,8 @@ struct ext4_sb_info {
> >  	u32 s_min_batch_time;
> >  	struct block_device *journal_bdev;
> >  #ifdef CONFIG_QUOTA
> > -	char *s_qf_names[EXT4_MAXQUOTAS];	/* Names of quota files with journalled quota */
> > +	/* Names of quota files with journalled quota */
> > +	char __rcu *s_qf_names[EXT4_MAXQUOTAS];
> >  	int s_jquota_fmt;			/* Format of quota to use */
> >  #endif
> >  	unsigned int s_want_extra_isize; /* New inodes should reserve # bytes */
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index faf293ed8060..1909fbb28583 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -920,6 +920,18 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
> >  }
> >  #endif
> >  
> > +/*
> > + * This is a helper function which is used in the mount/remount
> > + * codepaths (which holds s_umount) to fetch the quota file name.
> > + */
> > +static inline char *get_qf_name(struct super_block *sb,
> > +				struct ext4_sb_info *sbi,
> > +				int type)
> > +{
> > +	return rcu_dereference_protected(sbi->s_qf_names[type],
> > +					 lockdep_is_held(&sb->s_umount));
> > +}
> > +
> >  static void ext4_put_super(struct super_block *sb)
> >  {
> >  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > @@ -965,7 +977,7 @@ static void ext4_put_super(struct super_block *sb)
> >  	percpu_free_rwsem(&sbi->s_journal_flag_rwsem);
> >  #ifdef CONFIG_QUOTA
> >  	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> > -		kfree(sbi->s_qf_names[i]);
> > +		kfree(get_qf_name(sb, sbi, i));
> >  #endif
> >  
> >  	/* Debugging code just in case the in-memory inode orphan list
> > @@ -1531,11 +1543,10 @@ static const char deprecated_msg[] =
> >  static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
> >  {
> >  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > -	char *qname;
> > +	char *qname, *old_qname = get_qf_name(sb, sbi, qtype);
> >  	int ret = -1;
> >  
> > -	if (sb_any_quota_loaded(sb) &&
> > -		!sbi->s_qf_names[qtype]) {
> > +	if (sb_any_quota_loaded(sb) && !old_qname) {
> >  		ext4_msg(sb, KERN_ERR,
> >  			"Cannot change journaled "
> >  			"quota options when quota turned on");
> > @@ -1552,8 +1563,8 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
> >  			"Not enough memory for storing quotafile name");
> >  		return -1;
> >  	}
> > -	if (sbi->s_qf_names[qtype]) {
> > -		if (strcmp(sbi->s_qf_names[qtype], qname) == 0)
> > +	if (old_qname) {
> > +		if (strcmp(old_qname, qname) == 0)
> >  			ret = 1;
> >  		else
> >  			ext4_msg(sb, KERN_ERR,
> > @@ -1566,7 +1577,7 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
> >  			"quotafile must be on filesystem root");
> >  		goto errout;
> >  	}
> > -	sbi->s_qf_names[qtype] = qname;
> > +	rcu_assign_pointer(sbi->s_qf_names[qtype], qname);
> >  	set_opt(sb, QUOTA);
> >  	return 1;
> >  errout:
> > @@ -1578,15 +1589,16 @@ static int clear_qf_name(struct super_block *sb, int qtype)
> >  {
> >  
> >  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > +	char *old_qname = get_qf_name(sb, sbi, qtype);
> >  
> > -	if (sb_any_quota_loaded(sb) &&
> > -		sbi->s_qf_names[qtype]) {
> > +	if (sb_any_quota_loaded(sb) && old_qname) {
> >  		ext4_msg(sb, KERN_ERR, "Cannot change journaled quota options"
> >  			" when quota turned on");
> >  		return -1;
> >  	}
> > -	kfree(sbi->s_qf_names[qtype]);
> > -	sbi->s_qf_names[qtype] = NULL;
> > +	rcu_assign_pointer(sbi->s_qf_names[qtype], NULL);
> > +	synchronize_rcu();
> > +	kfree(old_qname);
> >  	return 1;
> >  }
> >  #endif
> > @@ -1961,7 +1973,7 @@ static int parse_options(char *options, struct super_block *sb,
> >  			 int is_remount)
> >  {
> >  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > -	char *p;
> > +	char *p, *usr_qf_name, *grp_qf_name;
> >  	substring_t args[MAX_OPT_ARGS];
> >  	int token;
> >  
> > @@ -1992,11 +2004,13 @@ static int parse_options(char *options, struct super_block *sb,
> >  			 "Cannot enable project quota enforcement.");
> >  		return 0;
> >  	}
> > -	if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
> > -		if (test_opt(sb, USRQUOTA) && sbi->s_qf_names[USRQUOTA])
> > +	usr_qf_name = get_qf_name(sb, sbi, USRQUOTA);
> > +	grp_qf_name = get_qf_name(sb, sbi, GRPQUOTA);
> > +	if (usr_qf_name || grp_qf_name) {
> > +		if (test_opt(sb, USRQUOTA) && usr_qf_name)
> >  			clear_opt(sb, USRQUOTA);
> >  
> > -		if (test_opt(sb, GRPQUOTA) && sbi->s_qf_names[GRPQUOTA])
> > +		if (test_opt(sb, GRPQUOTA) && grp_qf_name)
> >  			clear_opt(sb, GRPQUOTA);
> >  
> >  		if (test_opt(sb, GRPQUOTA) || test_opt(sb, USRQUOTA)) {
> > @@ -2030,6 +2044,7 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
> >  {
> >  #if defined(CONFIG_QUOTA)
> >  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > +	char *usr_qf_name, *grp_qf_name;
> >  
> >  	if (sbi->s_jquota_fmt) {
> >  		char *fmtname = "";
> > @@ -2048,11 +2063,14 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
> >  		seq_printf(seq, ",jqfmt=%s", fmtname);
> >  	}
> >  
> > -	if (sbi->s_qf_names[USRQUOTA])
> > -		seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
> > -
> > -	if (sbi->s_qf_names[GRPQUOTA])
> > -		seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
> > +	rcu_read_lock();
> > +	usr_qf_name = rcu_dereference(sbi->s_qf_names[USRQUOTA]);
> > +	grp_qf_name = rcu_dereference(sbi->s_qf_names[GRPQUOTA]);
> > +	if (usr_qf_name)
> > +		seq_show_option(seq, "usrjquota", usr_qf_name);
> > +	if (grp_qf_name)
> > +		seq_show_option(seq, "grpjquota", grp_qf_name);
> > +	rcu_read_unlock();
> >  #endif
> >  }
> >  
> > @@ -5104,6 +5122,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> >  	int err = 0;
> >  #ifdef CONFIG_QUOTA
> >  	int i, j;
> > +	char *to_free[EXT4_MAXQUOTAS];
> >  #endif
> >  	char *orig_data = kstrdup(data, GFP_KERNEL);
> >  
> > @@ -5123,8 +5142,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> >  	old_opts.s_jquota_fmt = sbi->s_jquota_fmt;
> >  	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> >  		if (sbi->s_qf_names[i]) {
> > -			old_opts.s_qf_names[i] = kstrdup(sbi->s_qf_names[i],
> > -							 GFP_KERNEL);
> > +			char *qf_name = get_qf_name(sb, sbi, i);
> > +
> > +			old_opts.s_qf_names[i] = kstrdup(qf_name, GFP_KERNEL);
> >  			if (!old_opts.s_qf_names[i]) {
> >  				for (j = 0; j < i; j++)
> >  					kfree(old_opts.s_qf_names[j]);
> > @@ -5353,9 +5373,12 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> >  #ifdef CONFIG_QUOTA
> >  	sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
> >  	for (i = 0; i < EXT4_MAXQUOTAS; i++) {
> > -		kfree(sbi->s_qf_names[i]);
> > -		sbi->s_qf_names[i] = old_opts.s_qf_names[i];
> > +		to_free[i] = get_qf_name(sb, sbi, i);
> > +		rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
> >  	}
> > +	synchronize_rcu();
> > +	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> > +		kfree(to_free[i]);
> >  #endif
> >  	kfree(orig_data);
> >  	return err;
> > @@ -5546,7 +5569,7 @@ static int ext4_write_info(struct super_block *sb, int type)
> >   */
> >  static int ext4_quota_on_mount(struct super_block *sb, int type)
> >  {
> > -	return dquot_quota_on_mount(sb, EXT4_SB(sb)->s_qf_names[type],
> > +	return dquot_quota_on_mount(sb, get_qf_name(sb, EXT4_SB(sb), type),
> >  					EXT4_SB(sb)->s_jquota_fmt, type);
> >  }
> >  
> > -- 
> > 2.18.0.rc0
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>
Jan Kara Oct. 11, 2018, 4:48 p.m. | #4
On Thu 11-10-18 08:29:53, Paul E. McKenney wrote:
> On Thu, Oct 11, 2018 at 12:28:00PM +0200, Jan Kara wrote:
> > On Sat 06-10-18 23:07:06, Theodore Ts'o wrote:
> > > It's possible for ext4_show_quota_options() to try reading
> > > s_qf_names[i] while it is being modified by ext4_remount() --- most
> > > notably, in ext4_remount's error path when the original values of the
> > > quota file name gets restored.
> > > 
> > > Reported-by: syzbot+a2872d6feea6918008a9@syzkaller.appspotmail.com
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > Cc: stable@kernel.org
> > 
> > Well, honestly the fact that ->show_options can be called while remount is
> > changing stuff under you looks problematic to me and I bet ext4 is not the
> > only one that would have issues with that. So I believe we might be better
> > off with just synchronizing ->show_options with umount / remount properly.
> > What were the lock dependency problems that made you switch to use RCU?
> 
> OK, I will bite...
> 
> Ted's patch does in fact just properly synchronize ->show_options with
> umount / remount.  Using RCU.  ;-)

Well, it does but only for quota mount options. There may be other mount
options which would need similar treatment and possibly other mount options
in other filesystems. So I think a saner VFS API would be to synchronize
->show_options with umount / remount so that each filesystem does not have
to do it on its own.

								Honza
Paul E. McKenney Oct. 11, 2018, 5:20 p.m. | #5
On Thu, Oct 11, 2018 at 06:48:54PM +0200, Jan Kara wrote:
> On Thu 11-10-18 08:29:53, Paul E. McKenney wrote:
> > On Thu, Oct 11, 2018 at 12:28:00PM +0200, Jan Kara wrote:
> > > On Sat 06-10-18 23:07:06, Theodore Ts'o wrote:
> > > > It's possible for ext4_show_quota_options() to try reading
> > > > s_qf_names[i] while it is being modified by ext4_remount() --- most
> > > > notably, in ext4_remount's error path when the original values of the
> > > > quota file name gets restored.
> > > > 
> > > > Reported-by: syzbot+a2872d6feea6918008a9@syzkaller.appspotmail.com
> > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > Cc: stable@kernel.org
> > > 
> > > Well, honestly the fact that ->show_options can be called while remount is
> > > changing stuff under you looks problematic to me and I bet ext4 is not the
> > > only one that would have issues with that. So I believe we might be better
> > > off with just synchronizing ->show_options with umount / remount properly.
> > > What were the lock dependency problems that made you switch to use RCU?
> > 
> > OK, I will bite...
> > 
> > Ted's patch does in fact just properly synchronize ->show_options with
> > umount / remount.  Using RCU.  ;-)
> 
> Well, it does but only for quota mount options. There may be other mount
> options which would need similar treatment and possibly other mount options
> in other filesystems. So I think a saner VFS API would be to synchronize
> ->show_options with umount / remount so that each filesystem does not have
> to do it on its own.

On that choice, I must of course defer to the various filesystem and
VFS people, yourself included.

							Thanx, Paul
Theodore Y. Ts'o Oct. 11, 2018, 7:02 p.m. | #6
On Thu, Oct 11, 2018 at 12:28:00PM +0200, Jan Kara wrote:
> Well, honestly the fact that ->show_options can be called while remount is
> changing stuff under you looks problematic to me and I bet ext4 is not the
> only one that would have issues with that. So I believe we might be better
> off with just synchronizing ->show_options with umount / remount properly.
> What were the lock dependency problems that made you switch to use RCU?

Agreed that "cat /proc/mounts" racing with "mount -o remount" could
result in inconsistent results getting printed, but other
inconsistencies should not result in an outright crash.  (It's
actually hard to get the crash in the first place, since it requires
really malicious syzbot^H^H^H^H^H^H program to trigger it at all, and
even syzbot was not able to hit this reliably, so it couldn't create a
repro.)

As far as the lock dependency problem is concerned, introducing
down_read(&sb->s_umount) to ->show_options() creates the potential
3-way circular deadlock:

        CPU0                    CPU1
        ----                    ----
   down_write(&inode->i_rwsem)
                                down_read(&sb->s_umount);
                                down_write(&inode->i_rwsem)
   down_write(&namespace_sem);

					- Ted

root: ext4/4k run xfstest generic/306

EXT4-fs (dm-2): mounted filesystem with ordered data mode. Opts: acl,user_xattr,block_validity
EXT4-fs (dm-1): mounted filesystem with ordered data mode. Opts: acl,user_xattr,block_validity
EXT4-fs (dm-1): re-mounted. Opts: (null)

======================================================
WARNING: possible circular locking dependency detected
4.19.0-rc6-xfstests-00017-g97536a3f5dff #644 Not tainted
------------------------------------------------------
mount/25708 is trying to acquire lock:
00000000556213f2 (namespace_sem){++++}, at: lock_mount+0x3b/0x100

but task is already holding lock:
000000006492cd73 (&sb->s_type->i_mutex_key#11){++++}, at: lock_mount+0x2b/0x100

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (&sb->s_type->i_mutex_key#11){++++}:
       down_write+0x48/0xb0
       vfs_load_quota_inode+0x45b/0x4d0
       ext4_quota_on+0x126/0x260
       kernel_quotactl+0x6ce/0x860
       __x64_sys_quotactl+0x1a/0x20
       do_syscall_64+0x56/0x190
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

-> #1 (&type->s_umount_key#26){++++}:
       down_read+0x3f/0xa0
       _ext4_show_options+0x3ba/0x7f0
       show_mountinfo+0x1f1/0x2a0
       seq_read+0x15e/0x400
       __vfs_read+0x36/0x190
       vfs_read+0x9f/0x130
       ksys_read+0x52/0xc0
       do_syscall_64+0x56/0x190
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

-> #0 (namespace_sem){++++}:
       lock_acquire+0xa6/0x180
       down_write+0x48/0xb0
       lock_mount+0x3b/0x100
       do_mount+0x49c/0xdf0
       ksys_mount+0xba/0xd0
       __x64_sys_mount+0x21/0x30
       do_syscall_64+0x56/0x190
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

other info that might help us debug this:

Chain exists of:
  namespace_sem --> &type->s_umount_key#26 --> &sb->s_type->i_mutex_key#11

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&sb->s_type->i_mutex_key#11);
                               lock(&type->s_umount_key#26);
                               lock(&sb->s_type->i_mutex_key#11);
  lock(namespace_sem);

 *** DEADLOCK ***

1 lock held by mount/25708:
 #0: 000000006492cd73 (&sb->s_type->i_mutex_key#11){++++}, at: lock_mount+0x2b/0x100

stack backtrace:
CPU: 1 PID: 25708 Comm: mount Not tainted 4.19.0-rc6-xfstests-00017-g97536a3f5dff #644
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 dump_stack+0x85/0xc0
 print_circular_bug.isra.19.cold.39+0x1c3/0x21f
 check_prev_add.constprop.27+0x4ec/0x730
 __lock_acquire+0xbbd/0xf40
 lock_acquire+0xa6/0x180
 ? lock_mount+0x3b/0x100
 down_write+0x48/0xb0
 ? lock_mount+0x3b/0x100
 lock_mount+0x3b/0x100
 do_mount+0x49c/0xdf0
 ksys_mount+0xba/0xd0
 __x64_sys_mount+0x21/0x30
 do_syscall_64+0x56/0x190
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f7cd5d0824a
Code: 48 8b 0d 51 fc 2a 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1e fc 2a 00 f7 d8 64 89 01 48
RSP: 002b:00007fffe6526c48 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 000055c88728d060 RCX: 00007f7cd5d0824a
RDX: 000055c88728ff30 RSI: 000055c88728d260 RDI: 000055c88728d240
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000020
R10: 00000000c0ed1000 R11: 0000000000000206 R12: 000055c88728d240
R13: 000055c88728ff30 R14: 0000000000000000 R15: 00000000ffffffff
EXT4-fs (dm-2): mounted filesystem with ordered data mode. Opts: acl,user_xattr,block_validity
EXT4-fs (dm-1): mounted filesystem with ordered data mode. Opts: acl,user_xattr,block_validity

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 86e1bacac757..12f90d48ba61 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1405,7 +1405,8 @@  struct ext4_sb_info {
 	u32 s_min_batch_time;
 	struct block_device *journal_bdev;
 #ifdef CONFIG_QUOTA
-	char *s_qf_names[EXT4_MAXQUOTAS];	/* Names of quota files with journalled quota */
+	/* Names of quota files with journalled quota */
+	char __rcu *s_qf_names[EXT4_MAXQUOTAS];
 	int s_jquota_fmt;			/* Format of quota to use */
 #endif
 	unsigned int s_want_extra_isize; /* New inodes should reserve # bytes */
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index faf293ed8060..1909fbb28583 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -920,6 +920,18 @@  static inline void ext4_quota_off_umount(struct super_block *sb)
 }
 #endif
 
+/*
+ * This is a helper function which is used in the mount/remount
+ * codepaths (which holds s_umount) to fetch the quota file name.
+ */
+static inline char *get_qf_name(struct super_block *sb,
+				struct ext4_sb_info *sbi,
+				int type)
+{
+	return rcu_dereference_protected(sbi->s_qf_names[type],
+					 lockdep_is_held(&sb->s_umount));
+}
+
 static void ext4_put_super(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
@@ -965,7 +977,7 @@  static void ext4_put_super(struct super_block *sb)
 	percpu_free_rwsem(&sbi->s_journal_flag_rwsem);
 #ifdef CONFIG_QUOTA
 	for (i = 0; i < EXT4_MAXQUOTAS; i++)
-		kfree(sbi->s_qf_names[i]);
+		kfree(get_qf_name(sb, sbi, i));
 #endif
 
 	/* Debugging code just in case the in-memory inode orphan list
@@ -1531,11 +1543,10 @@  static const char deprecated_msg[] =
 static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	char *qname;
+	char *qname, *old_qname = get_qf_name(sb, sbi, qtype);
 	int ret = -1;
 
-	if (sb_any_quota_loaded(sb) &&
-		!sbi->s_qf_names[qtype]) {
+	if (sb_any_quota_loaded(sb) && !old_qname) {
 		ext4_msg(sb, KERN_ERR,
 			"Cannot change journaled "
 			"quota options when quota turned on");
@@ -1552,8 +1563,8 @@  static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
 			"Not enough memory for storing quotafile name");
 		return -1;
 	}
-	if (sbi->s_qf_names[qtype]) {
-		if (strcmp(sbi->s_qf_names[qtype], qname) == 0)
+	if (old_qname) {
+		if (strcmp(old_qname, qname) == 0)
 			ret = 1;
 		else
 			ext4_msg(sb, KERN_ERR,
@@ -1566,7 +1577,7 @@  static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
 			"quotafile must be on filesystem root");
 		goto errout;
 	}
-	sbi->s_qf_names[qtype] = qname;
+	rcu_assign_pointer(sbi->s_qf_names[qtype], qname);
 	set_opt(sb, QUOTA);
 	return 1;
 errout:
@@ -1578,15 +1589,16 @@  static int clear_qf_name(struct super_block *sb, int qtype)
 {
 
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	char *old_qname = get_qf_name(sb, sbi, qtype);
 
-	if (sb_any_quota_loaded(sb) &&
-		sbi->s_qf_names[qtype]) {
+	if (sb_any_quota_loaded(sb) && old_qname) {
 		ext4_msg(sb, KERN_ERR, "Cannot change journaled quota options"
 			" when quota turned on");
 		return -1;
 	}
-	kfree(sbi->s_qf_names[qtype]);
-	sbi->s_qf_names[qtype] = NULL;
+	rcu_assign_pointer(sbi->s_qf_names[qtype], NULL);
+	synchronize_rcu();
+	kfree(old_qname);
 	return 1;
 }
 #endif
@@ -1961,7 +1973,7 @@  static int parse_options(char *options, struct super_block *sb,
 			 int is_remount)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	char *p;
+	char *p, *usr_qf_name, *grp_qf_name;
 	substring_t args[MAX_OPT_ARGS];
 	int token;
 
@@ -1992,11 +2004,13 @@  static int parse_options(char *options, struct super_block *sb,
 			 "Cannot enable project quota enforcement.");
 		return 0;
 	}
-	if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
-		if (test_opt(sb, USRQUOTA) && sbi->s_qf_names[USRQUOTA])
+	usr_qf_name = get_qf_name(sb, sbi, USRQUOTA);
+	grp_qf_name = get_qf_name(sb, sbi, GRPQUOTA);
+	if (usr_qf_name || grp_qf_name) {
+		if (test_opt(sb, USRQUOTA) && usr_qf_name)
 			clear_opt(sb, USRQUOTA);
 
-		if (test_opt(sb, GRPQUOTA) && sbi->s_qf_names[GRPQUOTA])
+		if (test_opt(sb, GRPQUOTA) && grp_qf_name)
 			clear_opt(sb, GRPQUOTA);
 
 		if (test_opt(sb, GRPQUOTA) || test_opt(sb, USRQUOTA)) {
@@ -2030,6 +2044,7 @@  static inline void ext4_show_quota_options(struct seq_file *seq,
 {
 #if defined(CONFIG_QUOTA)
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	char *usr_qf_name, *grp_qf_name;
 
 	if (sbi->s_jquota_fmt) {
 		char *fmtname = "";
@@ -2048,11 +2063,14 @@  static inline void ext4_show_quota_options(struct seq_file *seq,
 		seq_printf(seq, ",jqfmt=%s", fmtname);
 	}
 
-	if (sbi->s_qf_names[USRQUOTA])
-		seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
-
-	if (sbi->s_qf_names[GRPQUOTA])
-		seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
+	rcu_read_lock();
+	usr_qf_name = rcu_dereference(sbi->s_qf_names[USRQUOTA]);
+	grp_qf_name = rcu_dereference(sbi->s_qf_names[GRPQUOTA]);
+	if (usr_qf_name)
+		seq_show_option(seq, "usrjquota", usr_qf_name);
+	if (grp_qf_name)
+		seq_show_option(seq, "grpjquota", grp_qf_name);
+	rcu_read_unlock();
 #endif
 }
 
@@ -5104,6 +5122,7 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	int err = 0;
 #ifdef CONFIG_QUOTA
 	int i, j;
+	char *to_free[EXT4_MAXQUOTAS];
 #endif
 	char *orig_data = kstrdup(data, GFP_KERNEL);
 
@@ -5123,8 +5142,9 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	old_opts.s_jquota_fmt = sbi->s_jquota_fmt;
 	for (i = 0; i < EXT4_MAXQUOTAS; i++)
 		if (sbi->s_qf_names[i]) {
-			old_opts.s_qf_names[i] = kstrdup(sbi->s_qf_names[i],
-							 GFP_KERNEL);
+			char *qf_name = get_qf_name(sb, sbi, i);
+
+			old_opts.s_qf_names[i] = kstrdup(qf_name, GFP_KERNEL);
 			if (!old_opts.s_qf_names[i]) {
 				for (j = 0; j < i; j++)
 					kfree(old_opts.s_qf_names[j]);
@@ -5353,9 +5373,12 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
 #ifdef CONFIG_QUOTA
 	sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
 	for (i = 0; i < EXT4_MAXQUOTAS; i++) {
-		kfree(sbi->s_qf_names[i]);
-		sbi->s_qf_names[i] = old_opts.s_qf_names[i];
+		to_free[i] = get_qf_name(sb, sbi, i);
+		rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
 	}
+	synchronize_rcu();
+	for (i = 0; i < EXT4_MAXQUOTAS; i++)
+		kfree(to_free[i]);
 #endif
 	kfree(orig_data);
 	return err;
@@ -5546,7 +5569,7 @@  static int ext4_write_info(struct super_block *sb, int type)
  */
 static int ext4_quota_on_mount(struct super_block *sb, int type)
 {
-	return dquot_quota_on_mount(sb, EXT4_SB(sb)->s_qf_names[type],
+	return dquot_quota_on_mount(sb, get_qf_name(sb, EXT4_SB(sb), type),
 					EXT4_SB(sb)->s_jquota_fmt, type);
 }