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

["Singh, Balbir" <sblbir@amazon.com>]

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>