[PATCH 0/3] ext4: Fix potential races when performing online resizing

[PATCH 0/3] ext4: Fix potential races when performing online resizing

[Suraj Jitindar Singh]

This patch series fixes 2 additional races between array resizing and
array element access when performing online resizing of the arrays
s_group_info and s_flex_groups.

These patches apply on top of the patch:
[PATCH RFC] ext4: fix potential race between online resizing and write operations

The macro sbi_array_rcu_deref() is introduced for simplicity but can be
removed if undesired.

Tested by performing the following:
truncate -s 100G /tmp/foo
sudo bash -c 'while true; do dd if=/dev/zero of=/mnt/xxx bs=1M count=1; sync; \
rm /mnt/xxx; done' &
while true; do mkfs.ext4 -b 1024 -E resize=26213883 /tmp/foo 2096635 -F; \
sudo mount -o loop /tmp/foo /mnt; sudo resize2fs /dev/loop0 26213883; \
sudo umount /mnt; done

Suraj Jitindar Singh (3):
  ext4: introduce macro sbi_array_rcu_deref() to access rcu protected
    fields
  ext4: fix potential race between s_group_info online resizing and
    access
  ext4: fix potential race between s_flex_groups online resizing and
    access

 fs/ext4/balloc.c  | 11 +++++-----
 fs/ext4/ext4.h    | 25 +++++++++++++++++----
 fs/ext4/ialloc.c  | 21 +++++++++++-------
 fs/ext4/mballoc.c | 19 ++++++++++------
 fs/ext4/resize.c  |  4 ++--
 fs/ext4/super.c   | 56 ++++++++++++++++++++++++++++++++---------------
 6 files changed, 91 insertions(+), 45 deletions(-)

-- 
2.17.1

[PATCH 1/3] ext4: introduce macro sbi_array_rcu_deref() to access rcu protected fields

[Suraj Jitindar Singh]

The s_group_desc field in the super block info (sbi) is protected by rcu to
prevent access to an invalid pointer during online resize operations.
There are 2 other arrays in sbi, s_group_info and s_flex_groups, which
require similar rcu protection which is introduced in the subsequent
patches. Introduce a helper macro sbi_array_rcu_deref() to be used to
provide rcu protected access to such fields.

Also update the current s_group_desc access site to use the macro.

Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
Cc: stable@vger-kernel.org
---
 fs/ext4/balloc.c | 11 +++++------
 fs/ext4/ext4.h   | 17 +++++++++++++++++
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index 5368bf67300b..8fd0b3cdab4c 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -281,14 +281,13 @@ struct ext4_group_desc * ext4_get_group_desc(struct super_block *sb,
 
 	group_desc = block_group >> EXT4_DESC_PER_BLOCK_BITS(sb);
 	offset = block_group & (EXT4_DESC_PER_BLOCK(sb) - 1);
-	rcu_read_lock();
-	bh_p = rcu_dereference(sbi->s_group_desc)[group_desc];
+	bh_p = sbi_array_rcu_deref(sbi, s_group_desc, group_desc);
 	/*
-	 * We can unlock here since the pointer being dereferenced won't be
-	 * dereferenced again. By looking at the usage in add_new_gdb() the
-	 * value isn't modified, just the pointer, and so it remains valid.
+	 * sbi_array_rcu_deref returns with rcu unlocked, this is ok since
+	 * the pointer being dereferenced won't be dereferenced again. By
+	 * looking at the usage in add_new_gdb() the value isn't modified,
+	 * just the pointer, and so it remains valid.
 	 */
-	rcu_read_unlock();
 	if (!bh_p) {
 		ext4_error(sb, "Group descriptor not loaded - "
 			   "block_group = %u, group_desc = %u, desc = %u",
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 149ee0ab6d64..236fc6500340 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1576,6 +1576,23 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
 		 ino <= le32_to_cpu(EXT4_SB(sb)->s_es->s_inodes_count));
 }
 
+/*
+ * Returns: sbi->field[index]
+ * Used to access an array element from the following sbi fields which require
+ * rcu protection to avoid dereferencing an invalid pointer due to reassignment
+ * - s_group_desc
+ * - s_group_info
+ * - s_flex_group
+ */
+#define sbi_array_rcu_deref(sbi, field, index)				   \
+({									   \
+	typeof(*((sbi)->field)) _v;					   \
+	rcu_read_lock();						   \
+	_v = ((typeof((sbi)->field))rcu_dereference((sbi)->field))[index]; \
+	rcu_read_unlock();						   \
+	_v;								   \
+})
+
 /*
  * Simulate_fail codes
  */
-- 
2.17.1

[PATCH 2/3] ext4: fix potential race between s_group_info online resizing and access

[Suraj Jitindar Singh]

During an online resize an array of pointers to s_group_info gets replaced
so it can get enlarged. If there is a concurrent access to the array in
ext4_get_group_info() and this memory has been reused then this can lead to
an invalid memory access.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206443
Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
Cc: stable@vger.kernel.org
---
 fs/ext4/ext4.h    |  6 +++---
 fs/ext4/mballoc.c | 10 ++++++----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 236fc6500340..3f4aaaae7da6 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2994,13 +2994,13 @@ static inline
 struct ext4_group_info *ext4_get_group_info(struct super_block *sb,
 					    ext4_group_t group)
 {
-	 struct ext4_group_info ***grp_info;
+	 struct ext4_group_info **grp_info;
 	 long indexv, indexh;
 	 BUG_ON(group >= EXT4_SB(sb)->s_groups_count);
-	 grp_info = EXT4_SB(sb)->s_group_info;
 	 indexv = group >> (EXT4_DESC_PER_BLOCK_BITS(sb));
 	 indexh = group & ((EXT4_DESC_PER_BLOCK(sb)) - 1);
-	 return grp_info[indexv][indexh];
+	 grp_info = sbi_array_rcu_deref(EXT4_SB(sb), s_group_info, indexv);
+	 return grp_info[indexh];
 }
 
 /*
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f64838187559..0d9b17afc85f 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2356,7 +2356,7 @@ int ext4_mb_alloc_groupinfo(struct super_block *sb, ext4_group_t ngroups)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	unsigned size;
-	struct ext4_group_info ***new_groupinfo;
+	struct ext4_group_info ***old_groupinfo, ***new_groupinfo;
 
 	size = (ngroups + EXT4_DESC_PER_BLOCK(sb) - 1) >>
 		EXT4_DESC_PER_BLOCK_BITS(sb);
@@ -2369,13 +2369,15 @@ int ext4_mb_alloc_groupinfo(struct super_block *sb, ext4_group_t ngroups)
 		ext4_msg(sb, KERN_ERR, "can't allocate buddy meta group");
 		return -ENOMEM;
 	}
-	if (sbi->s_group_info) {
+	old_groupinfo = sbi->s_group_info;
+	if (sbi->s_group_info)
 		memcpy(new_groupinfo, sbi->s_group_info,
 		       sbi->s_group_info_size * sizeof(*sbi->s_group_info));
-		kvfree(sbi->s_group_info);
-	}
 	sbi->s_group_info = new_groupinfo;
+	rcu_assign_pointer(sbi->s_group_info, new_groupinfo);
 	sbi->s_group_info_size = size / sizeof(*sbi->s_group_info);
+	if (old_groupinfo)
+		ext4_kvfree_array_rcu(old_groupinfo);
 	ext4_debug("allocated s_groupinfo array for %d meta_bg's\n", 
 		   sbi->s_group_info_size);
 	return 0;
-- 
2.17.1

[PATCH 3/3] ext4: fix potential race between s_flex_groups online resizing and access

[Suraj Jitindar Singh]

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.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206443
Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
Cc: stable@vger.kernel.org
---
 fs/ext4/ext4.h    |  2 +-
 fs/ext4/ialloc.c  | 21 +++++++++++-------
 fs/ext4/mballoc.c |  9 +++++---
 fs/ext4/resize.c  |  4 ++--
 fs/ext4/super.c   | 56 ++++++++++++++++++++++++++++++++---------------
 5 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 3f4aaaae7da6..e8157ce5988b 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 **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..9324552a2ac2 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -330,9 +330,11 @@ void ext4_free_inode(handle_t *handle, struct inode *inode)
 	if (sbi->s_log_groups_per_flex) {
 		ext4_group_t f = ext4_flex_group(sbi, block_group);
 
-		atomic_inc(&sbi->s_flex_groups[f].free_inodes);
+		atomic_inc(&sbi_array_rcu_deref(sbi, s_flex_groups,
+						f)->free_inodes);
 		if (is_directory)
-			atomic_dec(&sbi->s_flex_groups[f].used_dirs);
+			atomic_dec(&sbi_array_rcu_deref(sbi, s_flex_groups,
+							f)->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_groups, g);
 
 	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;
 	}
 
@@ -1054,7 +1057,8 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 		if (sbi->s_log_groups_per_flex) {
 			ext4_group_t f = ext4_flex_group(sbi, group);
 
-			atomic_inc(&sbi->s_flex_groups[f].used_dirs);
+			atomic_inc(&sbi_array_rcu_deref(sbi, s_flex_groups,
+							f)->used_dirs);
 		}
 	}
 	if (ext4_has_group_desc_csum(sb)) {
@@ -1077,7 +1081,8 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 
 	if (sbi->s_log_groups_per_flex) {
 		flex_group = ext4_flex_group(sbi, group);
-		atomic_dec(&sbi->s_flex_groups[flex_group].free_inodes);
+		atomic_dec(&sbi_array_rcu_deref(sbi, s_flex_groups,
+						flex_group)->free_inodes);
 	}
 
 	inode->i_ino = ino + group * EXT4_INODES_PER_GROUP(sb);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 0d9b17afc85f..0de1191e12a8 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3022,7 +3022,8 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 		ext4_group_t flex_group = ext4_flex_group(sbi,
 							  ac->ac_b_ex.fe_group);
 		atomic64_sub(ac->ac_b_ex.fe_len,
-			     &sbi->s_flex_groups[flex_group].free_clusters);
+			     &sbi_array_rcu_deref(sbi, s_flex_groups,
+						  flex_group)->free_clusters);
 	}
 
 	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
@@ -4920,7 +4921,8 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
 	if (sbi->s_log_groups_per_flex) {
 		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
 		atomic64_add(count_clusters,
-			     &sbi->s_flex_groups[flex_group].free_clusters);
+			     &sbi_array_rcu_deref(sbi, s_flex_groups,
+						  flex_group)->free_clusters);
 	}
 
 	/*
@@ -5077,7 +5079,8 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 	if (sbi->s_log_groups_per_flex) {
 		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
 		atomic64_add(clusters_freed,
-			     &sbi->s_flex_groups[flex_group].free_clusters);
+			     &sbi_array_rcu_deref(sbi, s_flex_groups,
+						  flex_group)->free_clusters);
 	}
 
 	ext4_mb_unload_buddy(&e4b);
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 6fbe8607095f..941941a629a3 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1427,9 +1427,9 @@ static void ext4_update_super(struct super_block *sb,
 		ext4_group_t flex_group;
 		flex_group = ext4_flex_group(sbi, group_data[0].group);
 		atomic64_add(EXT4_NUM_B2C(sbi, free_blocks),
-			     &sbi->s_flex_groups[flex_group].free_clusters);
+			     &sbi->s_flex_groups[flex_group]->free_clusters);
 		atomic_add(EXT4_INODES_PER_GROUP(sb) * flex_gd->count,
-			   &sbi->s_flex_groups[flex_group].free_inodes);
+			   &sbi->s_flex_groups[flex_group]->free_inodes);
 	}
 
 	/*
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f464dff09774..3a401f930bca 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1049,7 +1049,11 @@ static void ext4_put_super(struct super_block *sb)
 	for (i = 0; i < sbi->s_gdb_count; i++)
 		brelse(sbi->s_group_desc[i]);
 	kvfree(sbi->s_group_desc);
-	kvfree(sbi->s_flex_groups);
+	if (sbi->s_flex_groups) {
+		for (i = 0; i < sbi->s_flex_groups_allocated; i++)
+			kvfree(sbi->s_flex_groups[i]);
+		kvfree(sbi->s_flex_groups);
+	}
 	percpu_counter_destroy(&sbi->s_freeclusters_counter);
 	percpu_counter_destroy(&sbi->s_freeinodes_counter);
 	percpu_counter_destroy(&sbi->s_dirs_counter);
@@ -2380,8 +2384,8 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
 int ext4_alloc_flex_bg_array(struct super_block *sb, ext4_group_t ngroup)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	struct flex_groups *new_groups;
-	int size;
+	struct flex_groups **old_groups, **new_groups;
+	int size, i;
 
 	if (!sbi->s_log_groups_per_flex)
 		return 0;
@@ -2390,22 +2394,35 @@ int ext4_alloc_flex_bg_array(struct super_block *sb, ext4_group_t ngroup)
 	if (size <= sbi->s_flex_groups_allocated)
 		return 0;
 
-	size = roundup_pow_of_two(size * sizeof(struct flex_groups));
-	new_groups = kvzalloc(size, GFP_KERNEL);
+	new_groups = kvzalloc(roundup_pow_of_two(size *
+			      sizeof(*sbi->s_flex_groups)), GFP_KERNEL);
 	if (!new_groups) {
-		ext4_msg(sb, KERN_ERR, "not enough memory for %d flex groups",
-			 size / (int) sizeof(struct flex_groups));
+		ext4_msg(sb, KERN_ERR,
+			 "not enough memory for %d flex group pointers", size);
 		return -ENOMEM;
 	}
-
-	if (sbi->s_flex_groups) {
+	for (i = sbi->s_flex_groups_allocated; i < size; i++) {
+		new_groups[i] = kvzalloc(roundup_pow_of_two(
+					 sizeof(struct flex_groups)),
+					 GFP_KERNEL);
+		if (!new_groups[i]) {
+			for (i--; i >= sbi->s_flex_groups_allocated; i--)
+				kvfree(new_groups[i]);
+			kvfree(new_groups);
+			ext4_msg(sb, KERN_ERR,
+				 "not enough memory for %d flex groups", size);
+			return -ENOMEM;
+		}
+	}
+	old_groups = sbi->s_flex_groups;
+	if (sbi->s_flex_groups)
 		memcpy(new_groups, sbi->s_flex_groups,
 		       (sbi->s_flex_groups_allocated *
-			sizeof(struct flex_groups)));
-		kvfree(sbi->s_flex_groups);
-	}
-	sbi->s_flex_groups = new_groups;
-	sbi->s_flex_groups_allocated = size / sizeof(struct flex_groups);
+			sizeof(struct flex_groups *)));
+	rcu_assign_pointer(sbi->s_flex_groups, new_groups);
+	sbi->s_flex_groups_allocated = size;
+	if (old_groups)
+		ext4_kvfree_array_rcu(old_groups);
 	return 0;
 }
 
@@ -2431,11 +2448,11 @@ static int ext4_fill_flex_info(struct super_block *sb)
 
 		flex_group = ext4_flex_group(sbi, i);
 		atomic_add(ext4_free_inodes_count(sb, gdp),
-			   &sbi->s_flex_groups[flex_group].free_inodes);
+			   &sbi->s_flex_groups[flex_group]->free_inodes);
 		atomic64_add(ext4_free_group_clusters(sb, gdp),
-			     &sbi->s_flex_groups[flex_group].free_clusters);
+			     &sbi->s_flex_groups[flex_group]->free_clusters);
 		atomic_add(ext4_used_dirs_count(sb, gdp),
-			   &sbi->s_flex_groups[flex_group].used_dirs);
+			   &sbi->s_flex_groups[flex_group]->used_dirs);
 	}
 
 	return 1;
@@ -4682,8 +4699,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	ext4_unregister_li_request(sb);
 failed_mount6:
 	ext4_mb_release(sb);
-	if (sbi->s_flex_groups)
+	if (sbi->s_flex_groups) {
+		for (i = 0; i < sbi->s_flex_groups_allocated; i++)
+			kvfree(sbi->s_flex_groups[i]);
 		kvfree(sbi->s_flex_groups);
+	}
 	percpu_counter_destroy(&sbi->s_freeclusters_counter);
 	percpu_counter_destroy(&sbi->s_freeinodes_counter);
 	percpu_counter_destroy(&sbi->s_dirs_counter);
-- 
2.17.1

Re: [PATCH 1/3] ext4: introduce macro sbi_array_rcu_deref() to access rcu protected fields

[Jitindar SIngh, Suraj]

+Cc stable
(correctly this time)

On Tue, 2020-02-18 at 19:08 -0800, Suraj Jitindar Singh wrote:
> The s_group_desc field in the super block info (sbi) is protected by
> rcu to
> prevent access to an invalid pointer during online resize operations.
> There are 2 other arrays in sbi, s_group_info and s_flex_groups,
> which
> require similar rcu protection which is introduced in the subsequent
> patches. Introduce a helper macro sbi_array_rcu_deref() to be used to
> provide rcu protected access to such fields.
> 
> Also update the current s_group_desc access site to use the macro.
> 
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
Cc: stable@vger.kernel.org
> Cc: stable@vger-kernel.org
> ---
>  fs/ext4/balloc.c | 11 +++++------
>  fs/ext4/ext4.h   | 17 +++++++++++++++++
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 5368bf67300b..8fd0b3cdab4c 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -281,14 +281,13 @@ struct ext4_group_desc *
> ext4_get_group_desc(struct super_block *sb,
>  
>  	group_desc = block_group >> EXT4_DESC_PER_BLOCK_BITS(sb);
>  	offset = block_group & (EXT4_DESC_PER_BLOCK(sb) - 1);
> -	rcu_read_lock();
> -	bh_p = rcu_dereference(sbi->s_group_desc)[group_desc];
> +	bh_p = sbi_array_rcu_deref(sbi, s_group_desc, group_desc);
>  	/*
> -	 * We can unlock here since the pointer being dereferenced
> won't be
> -	 * dereferenced again. By looking at the usage in add_new_gdb()
> the
> -	 * value isn't modified, just the pointer, and so it remains
> valid.
> +	 * sbi_array_rcu_deref returns with rcu unlocked, this is ok
> since
> +	 * the pointer being dereferenced won't be dereferenced again.
> By
> +	 * looking at the usage in add_new_gdb() the value isn't
> modified,
> +	 * just the pointer, and so it remains valid.
>  	 */
> -	rcu_read_unlock();
>  	if (!bh_p) {
>  		ext4_error(sb, "Group descriptor not loaded - "
>  			   "block_group = %u, group_desc = %u, desc =
> %u",
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 149ee0ab6d64..236fc6500340 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1576,6 +1576,23 @@ static inline int ext4_valid_inum(struct
> super_block *sb, unsigned long ino)
>  		 ino <= le32_to_cpu(EXT4_SB(sb)->s_es-
> >s_inodes_count));
>  }
>  
> +/*
> + * Returns: sbi->field[index]
> + * Used to access an array element from the following sbi fields
> which require
> + * rcu protection to avoid dereferencing an invalid pointer due to
> reassignment
> + * - s_group_desc
> + * - s_group_info
> + * - s_flex_group
> + */
> +#define sbi_array_rcu_deref(sbi, field, index)			
> 	   \
> +({									
>    \
> +	typeof(*((sbi)->field)) _v;					
>    \
> +	rcu_read_lock();						   \
> +	_v = ((typeof((sbi)->field))rcu_dereference((sbi)-
> >field))[index]; \
> +	rcu_read_unlock();						
>    \
> +	_v;								
>    \
> +})
> +
>  /*
>   * Simulate_fail codes
>   */

Re: [PATCH 1/3] ext4: introduce macro sbi_array_rcu_deref() to access rcu protected fields

[Singh, Balbir]

On Tue, 2020-02-18 at 19:08 -0800, Suraj Jitindar Singh wrote:
> The s_group_desc field in the super block info (sbi) is protected by rcu to
> prevent access to an invalid pointer during online resize operations.
> There are 2 other arrays in sbi, s_group_info and s_flex_groups, which
> require similar rcu protection which is introduced in the subsequent
> patches. Introduce a helper macro sbi_array_rcu_deref() to be used to
> provide rcu protected access to such fields.
> 
> Also update the current s_group_desc access site to use the macro.
> 
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
> Cc: stable@vger-kernel.org
> ---
>  fs/ext4/balloc.c | 11 +++++------
>  fs/ext4/ext4.h   | 17 +++++++++++++++++
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 5368bf67300b..8fd0b3cdab4c 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -281,14 +281,13 @@ struct ext4_group_desc * ext4_get_group_desc(struct
> super_block *sb,
>  
>  	group_desc = block_group >> EXT4_DESC_PER_BLOCK_BITS(sb);
>  	offset = block_group & (EXT4_DESC_PER_BLOCK(sb) - 1);
> -	rcu_read_lock();
> -	bh_p = rcu_dereference(sbi->s_group_desc)[group_desc];
> +	bh_p = sbi_array_rcu_deref(sbi, s_group_desc, group_desc);
>  	/*
> -	 * We can unlock here since the pointer being dereferenced won't be
> -	 * dereferenced again. By looking at the usage in add_new_gdb() the
> -	 * value isn't modified, just the pointer, and so it remains valid.
> +	 * sbi_array_rcu_deref returns with rcu unlocked, this is ok since
> +	 * the pointer being dereferenced won't be dereferenced again. By
> +	 * looking at the usage in add_new_gdb() the value isn't modified,
> +	 * just the pointer, and so it remains valid.
>  	 */
> -	rcu_read_unlock();
>  	if (!bh_p) {
>  		ext4_error(sb, "Group descriptor not loaded - "
>  			   "block_group = %u, group_desc = %u, desc = %u",
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 149ee0ab6d64..236fc6500340 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1576,6 +1576,23 @@ static inline int ext4_valid_inum(struct super_block
> *sb, unsigned long ino)
>  		 ino <= le32_to_cpu(EXT4_SB(sb)->s_es->s_inodes_count));
>  }
>  
> +/*
> + * Returns: sbi->field[index]
> + * Used to access an array element from the following sbi fields which
> require
> + * rcu protection to avoid dereferencing an invalid pointer due to
> reassignment
> + * - s_group_desc
> + * - s_group_info
> + * - s_flex_group
> + */
> +#define sbi_array_rcu_deref(sbi, field, index)				
>    \
> +({									   \
> +	typeof(*((sbi)->field)) _v;					   \
> +	rcu_read_lock();						   \
> +	_v = ((typeof((sbi)->field))rcu_dereference((sbi)->field))[index]; \

Minor nit, this can be
  
_v = ((typeof(_v)*)rcu_dereference((sbi)->field))[index];

> +	rcu_read_unlock();						   \
> +	_v;								   \
> +})
> +



Looks good to me
Reviewed-by: Balbir Singh <sblbir@amazon.com>

Re: [PATCH 2/3] ext4: fix potential race between s_group_info online resizing and access

[Singh, Balbir]

On Tue, 2020-02-18 at 19:08 -0800, Suraj Jitindar Singh wrote:
> During an online resize an array of pointers to s_group_info gets replaced
> so it can get enlarged. If there is a concurrent access to the array in
> ext4_get_group_info() and this memory has been reused then this can lead to
> an invalid memory access.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206443
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/ext4/ext4.h    |  6 +++---
>  fs/ext4/mballoc.c | 10 ++++++----
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 236fc6500340..3f4aaaae7da6 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2994,13 +2994,13 @@ static inline
>  struct ext4_group_info *ext4_get_group_info(struct super_block *sb,
>  					    ext4_group_t group)
>  {
> -	 struct ext4_group_info ***grp_info;
> +	 struct ext4_group_info **grp_info;
>  	 long indexv, indexh;
>  	 BUG_ON(group >= EXT4_SB(sb)->s_groups_count);
> -	 grp_info = EXT4_SB(sb)->s_group_info;
>  	 indexv = group >> (EXT4_DESC_PER_BLOCK_BITS(sb));
>  	 indexh = group & ((EXT4_DESC_PER_BLOCK(sb)) - 1);
> -	 return grp_info[indexv][indexh];
> +	 grp_info = sbi_array_rcu_deref(EXT4_SB(sb), s_group_info, indexv);
> +	 return grp_info[indexh];
>  }
>  
>  /*
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index f64838187559..0d9b17afc85f 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2356,7 +2356,7 @@ int ext4_mb_alloc_groupinfo(struct super_block *sb,
> ext4_group_t ngroups)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	unsigned size;
> -	struct ext4_group_info ***new_groupinfo;
> +	struct ext4_group_info ***old_groupinfo, ***new_groupinfo;
>  
>  	size = (ngroups + EXT4_DESC_PER_BLOCK(sb) - 1) >>
>  		EXT4_DESC_PER_BLOCK_BITS(sb);
> @@ -2369,13 +2369,15 @@ int ext4_mb_alloc_groupinfo(struct super_block *sb,
> ext4_group_t ngroups)
>  		ext4_msg(sb, KERN_ERR, "can't allocate buddy meta group");
>  		return -ENOMEM;
>  	}
> -	if (sbi->s_group_info) {
> +	old_groupinfo = sbi->s_group_info;
> +	if (sbi->s_group_info)
>  		memcpy(new_groupinfo, sbi->s_group_info,
>  		       sbi->s_group_info_size * sizeof(*sbi->s_group_info));
> -		kvfree(sbi->s_group_info);
> -	}
>  	sbi->s_group_info = new_groupinfo;
> +	rcu_assign_pointer(sbi->s_group_info, new_groupinfo);
>  	sbi->s_group_info_size = size / sizeof(*sbi->s_group_info);
> +	if (old_groupinfo)
> +		ext4_kvfree_array_rcu(old_groupinfo);
>  	ext4_debug("allocated s_groupinfo array for %d meta_bg's\n", 
>  		   sbi->s_group_info_size);
>  	return 0;

Reviewed-by: Balbir Singh <sblbir@amazon.com>

Re: [PATCH 1/3] ext4: introduce macro sbi_array_rcu_deref() to access rcu protected fields

[Theodore Y. Ts'o]

On Tue, Feb 18, 2020 at 07:08:49PM -0800, Suraj Jitindar Singh wrote:
> The s_group_desc field in the super block info (sbi) is protected by rcu to
> prevent access to an invalid pointer during online resize operations.
> There are 2 other arrays in sbi, s_group_info and s_flex_groups, which
> require similar rcu protection which is introduced in the subsequent
> patches. Introduce a helper macro sbi_array_rcu_deref() to be used to
> provide rcu protected access to such fields.
> 
> Also update the current s_group_desc access site to use the macro.
> 
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
> Cc: stable@vger-kernel.org

Thanks, applied with the simplification suggested by Balbir.  Also
note that I generally use stable@kernel.org instead of
stable@vger.kernel.org, since that avoids sending excess mail to
stable@vger.kernel.org mailing list.  (The stable kernel scripts look
for stable@kernel.org as well as stable@vger.kernel.org.)  I've made
that change in the version of the patch that I applied.

	     	       	   	      	  - Ted

Re: [PATCH 2/3] ext4: fix potential race between s_group_info online resizing and access

[Theodore Y. Ts'o]

On Tue, Feb 18, 2020 at 07:08:50PM -0800, Suraj Jitindar Singh wrote:
> During an online resize an array of pointers to s_group_info gets replaced
> so it can get enlarged. If there is a concurrent access to the array in
> ext4_get_group_info() and this memory has been reused then this can lead to
> an invalid memory access.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206443
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
> Cc: stable@vger.kernel.org

Thanks, applied.

					- Ted

Re: [PATCH 0/3] ext4: Fix potential races when performing online resizing

[Theodore Y. Ts'o]

Hi Suraj,

All of the patches to fix BZ 206443 are now on the ext4 git tree:

https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/log/?h=dev

I'm currently giving them a full regression test set using xfstests.
Could you run your tests to make sure it looks good for you?

I'm hoping to issue a pull request to Linus in time for 5.6-rc3 by
this weekend.

Also, if you can figure out a way to package up the repro as an
xfstests test case, that would be really excellent.  I think the
challenge is that some of them took a *huge* amount of pounding before
they repro'ed, correct?  I wasn't actually able to trigger the repro
using kvm, but I was only using a 2 CPU configuration.

Thanks,

						- Ted

Re: [PATCH 0/3] ext4: Fix potential races when performing online resizing

[Jitindar SIngh, Suraj]

On Thu, 2020-02-20 at 01:14 -0500, Theodore Y. Ts'o wrote:
> Hi Suraj,
> 
> All of the patches to fix BZ 206443 are now on the ext4 git tree:
> 
> 
https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/log/?h=dev
> 
> I'm currently giving them a full regression test set using xfstests.
> Could you run your tests to make sure it looks good for you?

Hi,

Have run my repo case for 6 hours without issue.
Previously this reproduced 100% of the time within <30 mins.

> 
> I'm hoping to issue a pull request to Linus in time for 5.6-rc3 by
> this weekend.
> 
> Also, if you can figure out a way to package up the repro as an
> xfstests test case, that would be really excellent.  I think the
> challenge is that some of them took a *huge* amount of pounding
> before
> they repro'ed, correct?  I wasn't actually able to trigger the repro
> using kvm, but I was only using a 2 CPU configuration.

I will look into this.

Thanks,
Suraj

> 
> Thanks,
> 
> 						- Ted
> 

[PATCH 2/3] ext4: fix potential race between s_group_info online resizing and access

[Theodore Ts'o]

From: Suraj Jitindar Singh <surajjs@amazon.com>

During an online resize an array of pointers to s_group_info gets replaced
so it can get enlarged. If there is a concurrent access to the array in
ext4_get_group_info() and this memory has been reused then this can lead to
an invalid memory access.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206443
Previous-Patch-Link: https://lore.kernel.org/r/20200219030851.2678-3-surajjs@amazon.com
Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Balbir Singh <sblbir@amazon.com>
Cc: stable@kernel.org
---
 fs/ext4/ext4.h    |  8 ++++----
 fs/ext4/mballoc.c | 52 +++++++++++++++++++++++++++++++----------------
 2 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b51003f75568..b1ece5329738 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1462,7 +1462,7 @@ struct ext4_sb_info {
 #endif
 
 	/* for buddy allocator */
-	struct ext4_group_info ***s_group_info;
+	struct ext4_group_info ** __rcu *s_group_info;
 	struct inode *s_buddy_cache;
 	spinlock_t s_md_lock;
 	unsigned short *s_mb_offsets;
@@ -2994,13 +2994,13 @@ static inline
 struct ext4_group_info *ext4_get_group_info(struct super_block *sb,
 					    ext4_group_t group)
 {
-	 struct ext4_group_info ***grp_info;
+	 struct ext4_group_info **grp_info;
 	 long indexv, indexh;
 	 BUG_ON(group >= EXT4_SB(sb)->s_groups_count);
-	 grp_info = EXT4_SB(sb)->s_group_info;
 	 indexv = group >> (EXT4_DESC_PER_BLOCK_BITS(sb));
 	 indexh = group & ((EXT4_DESC_PER_BLOCK(sb)) - 1);
-	 return grp_info[indexv][indexh];
+	 grp_info = sbi_array_rcu_deref(EXT4_SB(sb), s_group_info, indexv);
+	 return grp_info[indexh];
 }
 
 /*
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f64838187559..1b46fb63692a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2356,7 +2356,7 @@ int ext4_mb_alloc_groupinfo(struct super_block *sb, ext4_group_t ngroups)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	unsigned size;
-	struct ext4_group_info ***new_groupinfo;
+	struct ext4_group_info ***old_groupinfo, ***new_groupinfo;
 
 	size = (ngroups + EXT4_DESC_PER_BLOCK(sb) - 1) >>
 		EXT4_DESC_PER_BLOCK_BITS(sb);
@@ -2369,13 +2369,16 @@ int ext4_mb_alloc_groupinfo(struct super_block *sb, ext4_group_t ngroups)
 		ext4_msg(sb, KERN_ERR, "can't allocate buddy meta group");
 		return -ENOMEM;
 	}
-	if (sbi->s_group_info) {
-		memcpy(new_groupinfo, sbi->s_group_info,
+	rcu_read_lock();
+	old_groupinfo = rcu_dereference(sbi->s_group_info);
+	if (old_groupinfo)
+		memcpy(new_groupinfo, old_groupinfo,
 		       sbi->s_group_info_size * sizeof(*sbi->s_group_info));
-		kvfree(sbi->s_group_info);
-	}
-	sbi->s_group_info = new_groupinfo;
+	rcu_read_unlock();
+	rcu_assign_pointer(sbi->s_group_info, new_groupinfo);
 	sbi->s_group_info_size = size / sizeof(*sbi->s_group_info);
+	if (old_groupinfo)
+		ext4_kvfree_array_rcu(old_groupinfo);
 	ext4_debug("allocated s_groupinfo array for %d meta_bg's\n", 
 		   sbi->s_group_info_size);
 	return 0;
@@ -2387,6 +2390,7 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
 {
 	int i;
 	int metalen = 0;
+	int idx = group >> EXT4_DESC_PER_BLOCK_BITS(sb);
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_group_info **meta_group_info;
 	struct kmem_cache *cachep = get_groupinfo_cache(sb->s_blocksize_bits);
@@ -2405,12 +2409,12 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
 				 "for a buddy group");
 			goto exit_meta_group_info;
 		}
-		sbi->s_group_info[group >> EXT4_DESC_PER_BLOCK_BITS(sb)] =
-			meta_group_info;
+		rcu_read_lock();
+		rcu_dereference(sbi->s_group_info)[idx] = meta_group_info;
+		rcu_read_unlock();
 	}
 
-	meta_group_info =
-		sbi->s_group_info[group >> EXT4_DESC_PER_BLOCK_BITS(sb)];
+	meta_group_info = sbi_array_rcu_deref(sbi, s_group_info, idx);
 	i = group & (EXT4_DESC_PER_BLOCK(sb) - 1);
 
 	meta_group_info[i] = kmem_cache_zalloc(cachep, GFP_NOFS);
@@ -2458,8 +2462,13 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
 exit_group_info:
 	/* If a meta_group_info table has been allocated, release it now */
 	if (group % EXT4_DESC_PER_BLOCK(sb) == 0) {
-		kfree(sbi->s_group_info[group >> EXT4_DESC_PER_BLOCK_BITS(sb)]);
-		sbi->s_group_info[group >> EXT4_DESC_PER_BLOCK_BITS(sb)] = NULL;
+		struct ext4_group_info ***group_info;
+
+		rcu_read_lock();
+		group_info = rcu_dereference(sbi->s_group_info);
+		kfree(group_info[idx]);
+		group_info[idx] = NULL;
+		rcu_read_unlock();
 	}
 exit_meta_group_info:
 	return -ENOMEM;
@@ -2472,6 +2481,7 @@ static int ext4_mb_init_backend(struct super_block *sb)
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	int err;
 	struct ext4_group_desc *desc;
+	struct ext4_group_info ***group_info;
 	struct kmem_cache *cachep;
 
 	err = ext4_mb_alloc_groupinfo(sb, ngroups);
@@ -2507,11 +2517,16 @@ static int ext4_mb_init_backend(struct super_block *sb)
 	while (i-- > 0)
 		kmem_cache_free(cachep, ext4_get_group_info(sb, i));
 	i = sbi->s_group_info_size;
+	rcu_read_lock();
+	group_info = rcu_dereference(sbi->s_group_info);
 	while (i-- > 0)
-		kfree(sbi->s_group_info[i]);
+		kfree(group_info[i]);
+	rcu_read_unlock();
 	iput(sbi->s_buddy_cache);
 err_freesgi:
-	kvfree(sbi->s_group_info);
+	rcu_read_lock();
+	kvfree(rcu_dereference(sbi->s_group_info));
+	rcu_read_unlock();
 	return -ENOMEM;
 }
 
@@ -2700,7 +2715,7 @@ int ext4_mb_release(struct super_block *sb)
 	ext4_group_t ngroups = ext4_get_groups_count(sb);
 	ext4_group_t i;
 	int num_meta_group_infos;
-	struct ext4_group_info *grinfo;
+	struct ext4_group_info *grinfo, ***group_info;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct kmem_cache *cachep = get_groupinfo_cache(sb->s_blocksize_bits);
 
@@ -2719,9 +2734,12 @@ int ext4_mb_release(struct super_block *sb)
 		num_meta_group_infos = (ngroups +
 				EXT4_DESC_PER_BLOCK(sb) - 1) >>
 			EXT4_DESC_PER_BLOCK_BITS(sb);
+		rcu_read_lock();
+		group_info = rcu_dereference(sbi->s_group_info);
 		for (i = 0; i < num_meta_group_infos; i++)
-			kfree(sbi->s_group_info[i]);
-		kvfree(sbi->s_group_info);
+			kfree(group_info[i]);
+		kvfree(group_info);
+		rcu_read_unlock();
 	}
 	kfree(sbi->s_mb_offsets);
 	kfree(sbi->s_mb_maxs);
-- 
2.24.1