Re: [PATCH v3 2/4] iio: core: Add opaque_struct_size() helper and use it

[Jonathan Cameron]

On Mon, 24 Jul 2023 14:02:02 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> Introduce opaque_struct_size() helper, which may be moved
> to overflow.h in the future, and use it in the IIO core.
> 
> Potential users could be (among possible others):
> 
> 	__spi_alloc_controller() in drivers/spi/spi.c
> 	alloc_netdev_mqs in net/core/dev.c
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Cc: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> ---
>  drivers/iio/industrialio-core.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index b153adc5bc84..118ca6b59504 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1607,6 +1607,24 @@ const struct device_type iio_device_type = {
>  	.release = iio_dev_release,
>  };
>  
> +/**
> + * opaque_struct_size() - Calculate size of opaque structure with trailing aligned data.
> + * @p: Pointer to the opaque structure.
> + * @a: Alignment in bytes before trailing data.
> + * @s: Data size in bytes (preferred not to be 0).
> + *
> + * Calculates size of memory needed for structure of @p followed by
> + * the aligned data of size @s.
> + *
> + * Note, when @s is 0, the alignment @a is added to the sizeof(*(@p))
> + * and the result, depending on the @a, may be way off the initial size.

How often is this true?  A quick and dirty grep suggests at least 2 so perhaps
worth retaining the old behaviour.

Can we take that into account?  Maybe something like

#define opaque_struct_size(p, a, s) ((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)): sizeof(*p)) 

Or do it at the call site below.

> + *
> + * Returns: Number of bytes needed or SIZE_MAX on overflow.
> + */
> +#define opaque_struct_size(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))
> +
> +#define opaque_struct_data(p, a)	PTR_ALIGN((void *)((p) + 1), (a))
> +
>  /**
>   * iio_device_alloc() - allocate an iio_dev from a driver
>   * @parent:		Parent device.
> @@ -1618,19 +1636,13 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  	struct iio_dev *indio_dev;
>  	size_t alloc_size;
>  
> -	alloc_size = sizeof(struct iio_dev_opaque);
> -	if (sizeof_priv) {
> -		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN);
> -		alloc_size += sizeof_priv;
> -	}
> -
	if (sizeof_priv)
		alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
	else
		alloc_size = sizeof(struct iio_dev_opaque);


> +	alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
>  	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
>  	if (!iio_dev_opaque)
>  		return NULL;
>  
>  	indio_dev = &iio_dev_opaque->indio_dev;
> -	indio_dev->priv = (char *)iio_dev_opaque +
> -		ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> +	indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN);

Would have been safer if original code set this to NULL if
sizeof_priv == 0

A driver doing that should never have used iio_priv() but nicer if it was NULL rather
than off the end of the allocation.

Jonathan

>  
>  	indio_dev->dev.parent = parent;
>  	indio_dev->dev.type = &iio_device_type;