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 <[email protected]>
Cc: [email protected]
---
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
+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 <[email protected]>
Cc: [email protected]
> Cc: [email protected]
> ---
> 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
> */
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 <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>
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 <[email protected]>
> Cc: [email protected]
Thanks, applied with the simplification suggested by Balbir. Also
note that I generally use [email protected] instead of
[email protected], since that avoids sending excess mail to
[email protected] mailing list. (The stable kernel scripts look
for [email protected] as well as [email protected].) I've made
that change in the version of the patch that I applied.
- Ted