| Message ID | 20181007030706.9094-1-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [-v3] ext4: fix use-after-free race in ext4_remount()'s error path | expand |
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 >
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 >
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 >
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
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
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
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); }
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(-)