Re: [PATCH] md/md-linear: Annotate struct linear_conf with __counted_by

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 9/15/23 14:03, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> As found with Coccinelle[1], add __counted_by for struct linear_conf.
> Additionally, since the element count member must be set before accessing
> the annotated flexible array member, move its initialization earlier.
> 
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> 
> Cc: Song Liu <song@kernel.org>
> Cc: linux-raid@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks
-- 
Gustavo

> ---
>   drivers/md/md-linear.c | 26 +++++++++++++-------------
>   drivers/md/md-linear.h |  2 +-
>   2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
> index 71ac99646827..ae2826e9645b 100644
> --- a/drivers/md/md-linear.c
> +++ b/drivers/md/md-linear.c
> @@ -69,6 +69,19 @@ static struct linear_conf *linear_conf(struct mddev *mddev, int raid_disks)
>   	if (!conf)
>   		return NULL;
>   
> +	/*
> +	 * conf->raid_disks is copy of mddev->raid_disks. The reason to
> +	 * keep a copy of mddev->raid_disks in struct linear_conf is,
> +	 * mddev->raid_disks may not be consistent with pointers number of
> +	 * conf->disks[] when it is updated in linear_add() and used to
> +	 * iterate old conf->disks[] earray in linear_congested().
> +	 * Here conf->raid_disks is always consitent with number of
> +	 * pointers in conf->disks[] array, and mddev->private is updated
> +	 * with rcu_assign_pointer() in linear_addr(), such race can be
> +	 * avoided.
> +	 */
> +	conf->raid_disks = raid_disks;
> +
>   	cnt = 0;
>   	conf->array_sectors = 0;
>   
> @@ -112,19 +125,6 @@ static struct linear_conf *linear_conf(struct mddev *mddev, int raid_disks)
>   			conf->disks[i-1].end_sector +
>   			conf->disks[i].rdev->sectors;
>   
> -	/*
> -	 * conf->raid_disks is copy of mddev->raid_disks. The reason to
> -	 * keep a copy of mddev->raid_disks in struct linear_conf is,
> -	 * mddev->raid_disks may not be consistent with pointers number of
> -	 * conf->disks[] when it is updated in linear_add() and used to
> -	 * iterate old conf->disks[] earray in linear_congested().
> -	 * Here conf->raid_disks is always consitent with number of
> -	 * pointers in conf->disks[] array, and mddev->private is updated
> -	 * with rcu_assign_pointer() in linear_addr(), such race can be
> -	 * avoided.
> -	 */
> -	conf->raid_disks = raid_disks;
> -
>   	return conf;
>   
>   out:
> diff --git a/drivers/md/md-linear.h b/drivers/md/md-linear.h
> index 24e97db50ebb..5587eeedb882 100644
> --- a/drivers/md/md-linear.h
> +++ b/drivers/md/md-linear.h
> @@ -12,6 +12,6 @@ struct linear_conf
>   	struct rcu_head		rcu;
>   	sector_t		array_sectors;
>   	int			raid_disks; /* a copy of mddev->raid_disks */
> -	struct dev_info		disks[];
> +	struct dev_info		disks[] __counted_by(raid_disks);
>   };
>   #endif