On Fri, 2020-02-21 at 00:34 -0500, Theodore Ts'o wrote: > From: Suraj Jitindar Singh <surajjs@xxxxxxxxxx> > > During an online resize an array of s_flex_groups structures gets > replaced > so it can get enlarged. If there is a concurrent access to the array > and > this memory has been reused then this can lead to an invalid memory > access. > > The s_flex_group array has been converted into an array of pointers > rather > than an array of structures. This is to ensure that the information > contained in the structures cannot get out of sync during a resize > due to > an accessor updating the value in the old structure after it has been > copied but before the array pointer is updated. Since the structures > them- > selves are no longer copied but only the pointers to them this case > is > mitigated. A bug with this patch that I missed has been pointed out on the mailing list: https://lore.kernel.org/linux-ext4/1582293736.7365.109.camel@xxxxxx > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206443 > Previous-Patch-Link: > https://lore.kernel.org/r/20200219030851.2678-4-surajjs@xxxxxxxxxx > Signed-off-by: Suraj Jitindar Singh <surajjs@xxxxxxxxxx> > Signed-off-by: Theodore Ts'o <tytso@xxxxxxx> > Cc: stable@xxxxxxxxxx > --- > fs/ext4/ext4.h | 2 +- > fs/ext4/ialloc.c | 23 +++++++++------ > fs/ext4/mballoc.c | 9 ++++-- > fs/ext4/resize.c | 7 +++-- > fs/ext4/super.c | 72 ++++++++++++++++++++++++++++++++------------- > -- > 5 files changed, 76 insertions(+), 37 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index b1ece5329738..614fefa7dc7a 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -1512,7 +1512,7 @@ struct ext4_sb_info { > unsigned int s_extent_max_zeroout_kb; > > unsigned int s_log_groups_per_flex; > - struct flex_groups *s_flex_groups; > + struct flex_groups * __rcu *s_flex_groups; > ext4_group_t s_flex_groups_allocated; > > /* workqueue for reserved extent conversions (buffered io) */ > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > index c66e8f9451a2..501118b9ba90 100644 > --- a/fs/ext4/ialloc.c > +++ b/fs/ext4/ialloc.c > @@ -328,11 +328,13 @@ void ext4_free_inode(handle_t *handle, struct > inode *inode) > > percpu_counter_inc(&sbi->s_freeinodes_counter); > if (sbi->s_log_groups_per_flex) { > - ext4_group_t f = ext4_flex_group(sbi, block_group); > + struct flex_groups *fg; > > - atomic_inc(&sbi->s_flex_groups[f].free_inodes); > + fg = sbi_array_rcu_deref(sbi, s_flex_groups, > + ext4_flex_group(sbi, > block_group)); > + atomic_inc(&fg->free_inodes); > if (is_directory) > - atomic_dec(&sbi->s_flex_groups[f].used_dirs); > + atomic_dec(&fg->used_dirs); > } > BUFFER_TRACE(bh2, "call ext4_handle_dirty_metadata"); > fatal = ext4_handle_dirty_metadata(handle, NULL, bh2); > @@ -368,12 +370,13 @@ static void get_orlov_stats(struct super_block > *sb, ext4_group_t g, > int flex_size, struct orlov_stats *stats) > { > struct ext4_group_desc *desc; > - struct flex_groups *flex_group = EXT4_SB(sb)->s_flex_groups; > + struct flex_groups *flex_group = > sbi_array_rcu_deref(EXT4_SB(sb), > + s_flex_gro > ups, g); The assignment to flex_group needs to happen within the if (flex_size > 1) {} if statement to avoid a potential null pointer dereference. if (flex_size > 1) { struct flex_groups *flex_group = sbi_array_rcu_deref(EXT4_SB(sb), s_flex_groups, g); ... } Would you like a resend? > > if (flex_size > 1) { > - stats->free_inodes = > atomic_read(&flex_group[g].free_inodes); > - stats->free_clusters = > atomic64_read(&flex_group[g].free_clusters); > - stats->used_dirs = > atomic_read(&flex_group[g].used_dirs); > + stats->free_inodes = atomic_read(&flex_group- > >free_inodes); > + stats->free_clusters = atomic64_read(&flex_group- > >free_clusters); > + stats->used_dirs = atomic_read(&flex_group->used_dirs); > return; > } > [snip]
