[PATCH v4 0/8] iio: core: New macros and making use of them

[PATCH v4 0/8] iio: core: New macros and making use of them

[Andy Shevchenko]

Added new macros to overflow.h and reuse it in IIO. For the sake of examples
a few more places were updated (requested by Kees). In case maintainers are okay,
tags will be appreciated.

v4:
- dropped applied patches
- refactored macros and code to make them simpler (Jonathan)
- moved (renamed) macros to overflow.h

v3: https://lore.kernel.org/r/20230724110204.46285-1-andriy.shevchenko@linux.intel.com
- dropped applied patches
- use switch-case for the supported clocks (Jonathan)                                                            - redone opaque_struct_size() to be simpler (Uwe)
- dropped wrong hunk for krealloc_array() conversion (Jonathan)                                                  - dropped initcall move (Jonathan)

v2:
- sprintf() --> sysfs_emit() (Nuno)
- added tag (Nuno)

Andy Shevchenko (8):
  overflow: Use POD in check_shl_overflow()
  overflow: Add struct_size_with_data() and struct_data_pointer()
    helpers
  iio: core: NULLify private pointer when there is no private data
  iio: core: Calculate alloc_size only once in iio_device_alloc()
  iio: core: Use new helpers from overflow.h in iio_device_alloc()
  spi: Use new helpers from overflow.h in __spi_alloc_controller()
  net-device: Use new helpers from overflow.h in netdevice APIs
  dmaengine: ste_dma40: Use new helpers from overflow.h

 drivers/dma/ste_dma40.c         | 12 ++++++------
 drivers/iio/industrialio-core.c | 16 +++++++++-------
 drivers/spi/spi.c               |  6 +++---
 include/linux/netdevice.h       |  3 ++-
 include/linux/overflow.h        | 29 +++++++++++++++++++++++++++--
 net/core/dev.c                  | 10 +++++-----
 6 files changed, 52 insertions(+), 24 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v4 1/8] overflow: Use POD in check_shl_overflow()

[Andy Shevchenko]

The check_shl_overflow() uses u64 type that is defined in types.h.
Instead of including that header, just switch to use POD type
directly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/overflow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index dede374832c9..bc390f026128 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -197,7 +197,7 @@ static inline bool __must_check __must_check_overflow(bool overflow)
 	typeof(a) _a = a;						\
 	typeof(s) _s = s;						\
 	typeof(d) _d = d;						\
-	u64 _a_full = _a;						\
+	unsigned long long _a_full = _a;				\
 	unsigned int _to_shift =					\
 		is_non_negative(_s) && _s < 8 * sizeof(*d) ? _s : 0;	\
 	*_d = (_a_full << _to_shift);					\
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 1/8] overflow: Use POD in check_shl_overflow()

[Kees Cook]

On Wed, Feb 28, 2024 at 10:41:31PM +0200, Andy Shevchenko wrote:
> The check_shl_overflow() uses u64 type that is defined in types.h.
> Instead of including that header, just switch to use POD type
> directly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v4 1/8] overflow: Use POD in check_shl_overflow()

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 01:33:15PM -0800, Kees Cook wrote:
> On Wed, Feb 28, 2024 at 10:41:31PM +0200, Andy Shevchenko wrote:
> > The check_shl_overflow() uses u64 type that is defined in types.h.
> > Instead of including that header, just switch to use POD type
> > directly.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Kees Cook <keescook@chromium.org>

Based on the further discussion this seems pretty independent. Can you take it
through your tree?

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v4 1/8] overflow: Use POD in check_shl_overflow()

[Kees Cook]

On Wed, 28 Feb 2024 22:41:31 +0200, Andy Shevchenko wrote:
> The check_shl_overflow() uses u64 type that is defined in types.h.
> Instead of including that header, just switch to use POD type
> directly.
> 
> 

Applied to for-next/hardening, thanks!

[1/8] overflow: Use POD in check_shl_overflow()
      https://git.kernel.org/kees/c/4e55a75495b7

Take care,

-- 
Kees Cook

[PATCH v4 2/8] overflow: Add struct_size_with_data() and struct_data_pointer() helpers

[Andy Shevchenko]

Introduce two helper macros to calculate the size of the structure
with trailing aligned data and to retrieve the pointer to that data.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/overflow.h | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index bc390f026128..b93bbf1b6aaa 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -2,9 +2,10 @@
 #ifndef __LINUX_OVERFLOW_H
 #define __LINUX_OVERFLOW_H
 
+#include <linux/align.h>
 #include <linux/compiler.h>
-#include <linux/limits.h>
 #include <linux/const.h>
+#include <linux/limits.h>
 
 /*
  * We need to compute the minimum and maximum values representable in a given
@@ -337,6 +338,30 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
  */
 #define array3_size(a, b, c)	size_mul(size_mul(a, b), c)
 
+/**
+ * struct_size_with_data() - Calculate size of structure with trailing aligned data.
+ * @p: Pointer to the structure.
+ * @a: Alignment in bytes before trailing data.
+ * @s: Data size in bytes (must not be 0).
+ *
+ * Calculates size of memory needed for structure of @p followed by an
+ * aligned data of size @s.
+ *
+ * Return: number of bytes needed or SIZE_MAX on overflow.
+ */
+#define struct_size_with_data(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))
+
+/**
+ * struct_data_pointer - Calculate offset of the trailing data reserved with
+ * struct_size_with_data().
+ * @p: Pointer to the structure.
+ * @a: Alignment in bytes before trailing data.
+ *
+ * Return: offset in bytes to the trailing data reserved with
+ * struct_size_with_data().
+ */
+#define struct_data_pointer(p, a)	PTR_ALIGN((void *)((p) + 1), (a))
+
 /**
  * flex_array_size() - Calculate size of a flexible array member
  *                     within an enclosing structure.
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 2/8] overflow: Add struct_size_with_data() and struct_data_pointer() helpers

[Kees Cook]

On Wed, Feb 28, 2024 at 10:41:32PM +0200, Andy Shevchenko wrote:
> Introduce two helper macros to calculate the size of the structure
> with trailing aligned data and to retrieve the pointer to that data.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/overflow.h | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index bc390f026128..b93bbf1b6aaa 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -2,9 +2,10 @@
>  #ifndef __LINUX_OVERFLOW_H
>  #define __LINUX_OVERFLOW_H
>  
> +#include <linux/align.h>
>  #include <linux/compiler.h>
> -#include <linux/limits.h>
>  #include <linux/const.h>
> +#include <linux/limits.h>
>  
>  /*
>   * We need to compute the minimum and maximum values representable in a given
> @@ -337,6 +338,30 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>   */
>  #define array3_size(a, b, c)	size_mul(size_mul(a, b), c)
>  
> +/**
> + * struct_size_with_data() - Calculate size of structure with trailing aligned data.
> + * @p: Pointer to the structure.
> + * @a: Alignment in bytes before trailing data.
> + * @s: Data size in bytes (must not be 0).
> + *
> + * Calculates size of memory needed for structure of @p followed by an
> + * aligned data of size @s.
> + *
> + * Return: number of bytes needed or SIZE_MAX on overflow.
> + */
> +#define struct_size_with_data(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))

I don't like this -- "p" should have a trailing flexible array. (See
below.)

> +
> +/**
> + * struct_data_pointer - Calculate offset of the trailing data reserved with
> + * struct_size_with_data().
> + * @p: Pointer to the structure.
> + * @a: Alignment in bytes before trailing data.
> + *
> + * Return: offset in bytes to the trailing data reserved with
> + * struct_size_with_data().
> + */
> +#define struct_data_pointer(p, a)	PTR_ALIGN((void *)((p) + 1), (a))

I'm not super excited about propagating the "p + 1" code pattern to find
things after an allocation. This leads to the compiler either being
blind to accesses beyond an allocation, or being too conservative about
accesses beyond an object. Instead of these helpers I would much prefer
that data structures that use this code pattern be converted to using
trailing flexible arrays, at which point the compiler is in a much
better position to reason about sizes.

-- 
Kees Cook

Re: [PATCH v4 2/8] overflow: Add struct_size_with_data() and struct_data_pointer() helpers

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 01:37:36PM -0800, Kees Cook wrote:
> On Wed, Feb 28, 2024 at 10:41:32PM +0200, Andy Shevchenko wrote:

...

> > +#define struct_data_pointer(p, a)	PTR_ALIGN((void *)((p) + 1), (a))
> 
> I'm not super excited about propagating the "p + 1" code pattern to find
> things after an allocation. This leads to the compiler either being
> blind to accesses beyond an allocation, or being too conservative about
> accesses beyond an object. Instead of these helpers I would much prefer
> that data structures that use this code pattern be converted to using
> trailing flexible arrays, at which point the compiler is in a much
> better position to reason about sizes.

There is nothing about flexible arrays in this.
Maybe you have been confused by my choice for name of the macros.
In that case I also can argue that current struct_size() is a good one.
(something like struct_size_with_flex_array() can be more specific)

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 3/8] iio: core: NULLify private pointer when there is no private data

[Andy Shevchenko]

In iio_device_alloc() when size of the private data is 0,
the private pointer is calculated to behind the valid data.
NULLify it for good.

Fixes: 6d4ebd565d15 ("iio: core: wrap IIO device into an iio_dev_opaque object")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/industrialio-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 4302093b92c7..bd305fa87093 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1654,8 +1654,12 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 		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);
+
+	if (sizeof_priv)
+		indio_dev->priv = (char *)iio_dev_opaque +
+			ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
+	else
+		indio_dev->priv = NULL;
 
 	indio_dev->dev.parent = parent;
 	indio_dev->dev.type = &iio_device_type;
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 3/8] iio: core: NULLify private pointer when there is no private data

[David Lechner]

On Wed, Feb 28, 2024 at 2:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> In iio_device_alloc() when size of the private data is 0,
> the private pointer is calculated to behind the valid data.
> NULLify it for good.
>
> Fixes: 6d4ebd565d15 ("iio: core: wrap IIO device into an iio_dev_opaque object")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/industrialio-core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 4302093b92c7..bd305fa87093 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1654,8 +1654,12 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>                 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);
> +
> +       if (sizeof_priv)
> +               indio_dev->priv = (char *)iio_dev_opaque +
> +                       ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> +       else
> +               indio_dev->priv = NULL;

Do we actually need the else branch here since we use kzalloc() and
therefore indio_dev->priv should already be NULL?

>
>         indio_dev->dev.parent = parent;
>         indio_dev->dev.type = &iio_device_type;
> --
> 2.43.0.rc1.1.gbec44491f096
>
>

Re: [PATCH v4 3/8] iio: core: NULLify private pointer when there is no private data

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 03:06:42PM -0600, David Lechner wrote:
> On Wed, Feb 28, 2024 at 2:50 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > -       indio_dev->priv = (char *)iio_dev_opaque +
> > -               ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> > +
> > +       if (sizeof_priv)
> > +               indio_dev->priv = (char *)iio_dev_opaque +
> > +                       ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> > +       else
> > +               indio_dev->priv = NULL;
> 
> Do we actually need the else branch here since we use kzalloc() and
> therefore indio_dev->priv should already be NULL?

This is more robust, but I'm okay to drop this. Up to Jonathan.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/8] iio: core: NULLify private pointer when there is no private data

[Jonathan Cameron]

On Wed, 28 Feb 2024 23:36:43 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Feb 28, 2024 at 03:06:42PM -0600, David Lechner wrote:
> > On Wed, Feb 28, 2024 at 2:50 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:  
> 
> ...
> 
> > > -       indio_dev->priv = (char *)iio_dev_opaque +
> > > -               ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> > > +
> > > +       if (sizeof_priv)
> > > +               indio_dev->priv = (char *)iio_dev_opaque +
> > > +                       ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> > > +       else
> > > +               indio_dev->priv = NULL;  
> > 
> > Do we actually need the else branch here since we use kzalloc() and
> > therefore indio_dev->priv should already be NULL?  
> 
> This is more robust, but I'm okay to drop this. Up to Jonathan.
> 

Given the allocation is just above here fine to drop the else in this
I think.

[PATCH v4 4/8] iio: core: Calculate alloc_size only once in iio_device_alloc()

[Andy Shevchenko]

No need to rewrite the value, instead use 'else' branch.
This will also help further refactoring the code later on.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/industrialio-core.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index bd305fa87093..1986b3386307 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1643,11 +1643,10 @@ 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 = ALIGN(alloc_size, IIO_DMA_MINALIGN) + sizeof_priv;
+	else
+		alloc_size = sizeof(struct iio_dev_opaque);
 
 	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
 	if (!iio_dev_opaque)
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 4/8] iio: core: Calculate alloc_size only once in iio_device_alloc()

[David Lechner]

On Wed, Feb 28, 2024 at 2:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> No need to rewrite the value, instead use 'else' branch.
> This will also help further refactoring the code later on.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/industrialio-core.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index bd305fa87093..1986b3386307 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1643,11 +1643,10 @@ 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 = ALIGN(alloc_size, IIO_DMA_MINALIGN) + sizeof_priv;

Isn't this using alloc_size before it is assigned? Perhaps you meant this:

    alloc_size = ALIGN(sizeof(struct iio_dev_opaque),
IIO_DMA_MINALIGN) + sizeof_priv;


> +       else
> +               alloc_size = sizeof(struct iio_dev_opaque);
>
>         iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
>         if (!iio_dev_opaque)

Re: [PATCH v4 4/8] iio: core: Calculate alloc_size only once in iio_device_alloc()

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 02:57:36PM -0600, David Lechner wrote:
> On Wed, Feb 28, 2024 at 2:49 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > -       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 = ALIGN(alloc_size, IIO_DMA_MINALIGN) + sizeof_priv;
> 
> Isn't this using alloc_size before it is assigned? Perhaps you meant this:
> 
>     alloc_size = ALIGN(sizeof(struct iio_dev_opaque),
> IIO_DMA_MINALIGN) + sizeof_priv;

However the end result of the series is correct, this is a good catch!
Thanks!

> > +       else
> > +               alloc_size = sizeof(struct iio_dev_opaque);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 5/8] iio: core: Use new helpers from overflow.h in iio_device_alloc()

[Andy Shevchenko]

We have two new helpers struct_size_with_data() and struct_data_pointer()
that we can utilize in iio_device_alloc(). Do it so.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 1986b3386307..223013725e32 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1644,7 +1644,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	size_t alloc_size;
 
 	if (sizeof_priv)
-		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN) + sizeof_priv;
+		alloc_size = struct_size_with_data(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
 	else
 		alloc_size = sizeof(struct iio_dev_opaque);
 
@@ -1655,8 +1655,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	indio_dev = &iio_dev_opaque->indio_dev;
 
 	if (sizeof_priv)
-		indio_dev->priv = (char *)iio_dev_opaque +
-			ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
+		indio_dev->priv = struct_data_pointer(iio_dev_opaque, IIO_DMA_MINALIGN);
 	else
 		indio_dev->priv = NULL;
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 5/8] iio: core: Use new helpers from overflow.h in iio_device_alloc()

[Nuno Sá]

On Wed, 2024-02-28 at 22:41 +0200, Andy Shevchenko wrote:
> We have two new helpers struct_size_with_data() and struct_data_pointer()
> that we can utilize in iio_device_alloc(). Do it so.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 1986b3386307..223013725e32 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1644,7 +1644,7 @@ struct iio_dev *iio_device_alloc(struct device *parent,
> int sizeof_priv)
>  	size_t alloc_size;
>  
>  	if (sizeof_priv)
> -		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN) +
> sizeof_priv;
> +		alloc_size = struct_size_with_data(iio_dev_opaque,
> IIO_DMA_MINALIGN, sizeof_priv);
>  	else
>  		alloc_size = sizeof(struct iio_dev_opaque);
>  
> @@ -1655,8 +1655,7 @@ struct iio_dev *iio_device_alloc(struct device *parent,
> int sizeof_priv)
>  	indio_dev = &iio_dev_opaque->indio_dev;
>  
>  	if (sizeof_priv)
> -		indio_dev->priv = (char *)iio_dev_opaque +
> -			ALIGN(sizeof(struct iio_dev_opaque),
> IIO_DMA_MINALIGN);
> +		indio_dev->priv = struct_data_pointer(iio_dev_opaque,
> IIO_DMA_MINALIGN);

I'd +1 for implementing what Kees suggested in IIO. Only thing is (I think), we
need to move struct iio_dev indioo_dev to the end of struct iio_dev_opaque.

- Nuno Sá

Re: [PATCH v4 5/8] iio: core: Use new helpers from overflow.h in iio_device_alloc()

[Jonathan Cameron]

On Thu, 29 Feb 2024 16:29:43 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2024-02-28 at 22:41 +0200, Andy Shevchenko wrote:
> > We have two new helpers struct_size_with_data() and struct_data_pointer()
> > that we can utilize in iio_device_alloc(). Do it so.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/industrialio-core.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 1986b3386307..223013725e32 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1644,7 +1644,7 @@ struct iio_dev *iio_device_alloc(struct device *parent,
> > int sizeof_priv)
> >  	size_t alloc_size;
> >  
> >  	if (sizeof_priv)
> > -		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN) +
> > sizeof_priv;
> > +		alloc_size = struct_size_with_data(iio_dev_opaque,
> > IIO_DMA_MINALIGN, sizeof_priv);
> >  	else
> >  		alloc_size = sizeof(struct iio_dev_opaque);
> >  
> > @@ -1655,8 +1655,7 @@ struct iio_dev *iio_device_alloc(struct device *parent,
> > int sizeof_priv)
> >  	indio_dev = &iio_dev_opaque->indio_dev;
> >  
> >  	if (sizeof_priv)
> > -		indio_dev->priv = (char *)iio_dev_opaque +
> > -			ALIGN(sizeof(struct iio_dev_opaque),
> > IIO_DMA_MINALIGN);
> > +		indio_dev->priv = struct_data_pointer(iio_dev_opaque,
> > IIO_DMA_MINALIGN);  
> 
> I'd +1 for implementing what Kees suggested in IIO. Only thing is (I think), we
> need to move struct iio_dev indioo_dev to the end of struct iio_dev_opaque.

That is going to be messy and without horrible hacks (I think) add more padding we
don't need.  At the moment the struct iio_dev and the struct iio_dev_opaque
are aligned as at the start of the structure.

The priv data is aligned by padding the larger struct iio_dev_opaque,
so if you want the priv handle to be to data defined in struct iio_dev you would
need to add additional padding so that

struct iio_dev_opaque {
	stuff...
	// this next __aligned() is implicit anyway because of the rules for
	// a structure always being aligned to the alignment of it's max aligned
	// element.
	struct iio_dev __aligned (IIO_DMA_ALIGN) {  
		stuff
		u8 priv[] __aligned(IIO_DMA_ALIGN);
	}
}

How about using what Kees suggests on the iio_dev_opaque (which think is cleaner
anyway as that's what we are allocating) and keeping the magic pointer to priv
in the struct iio_dev; The compiler looses some visibility for iio_priv() accesses
but can it do much with those anyway? They always get cast to a struct driver_specific *
and getting the original allocation wrong is not easy to do as we pass
that struct size in.  Note, for others not aware of what is going on here, the
priv pointer in iio_dev is to allow efficient static inline iio_priv() calls without
needing to either make a function call, or expose the internals of the opaque
structure in which the iio_dev and the priv data are embedded.

Standard pattern is:

struct driver_specific *bob;
struct iio_dev *indio_dev = dev_iio_device_alloc(dev, sizeof(*bob));
// which allocates the iio_dev_opaque, but returns the contained iio_dev
bob = iio_priv(indio_dev);

So

struct iio_dev_opaque {
	struct iio_dev indio_dev {
		stuff..
		void *priv;	
	};
	stuff..
	int priv_count;
	u8 priv[] __aligned(IIO_DMA_ALIGN) __counted_by(priv_count);
}
with indio_dev->priv = iio_dev_opaque->dev?

This cleanups up a few IIO core bits but no impact outside them.
Nice to have those cleanups.

Is there any way to have that internal iio_dev->priv pointer associated with
a __counted_by even though it's pointing elsewhere than a local variable sized
trailing element?  

struct iio_dev {
	stuff

	u32 count;
	void *priv __counted_by(count);
}
compiles with gcc but without digging further I have no idea if it does anything useful!

Jonathan

> 
> - Nuno Sá
> 
> 
> 

[PATCH v4 6/8] spi: Use new helpers from overflow.h in __spi_alloc_controller()

[Andy Shevchenko]

We have two new helpers struct_size_with_data() and struct_data_pointer()
that we can utilize in __spi_alloc_controller(). Do it so.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ba4d3fde2054..de7a23da58c6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3089,12 +3089,12 @@ struct spi_controller *__spi_alloc_controller(struct device *dev,
 					      unsigned int size, bool slave)
 {
 	struct spi_controller	*ctlr;
-	size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());
+	int align = dma_get_cache_alignment();
 
 	if (!dev)
 		return NULL;
 
-	ctlr = kzalloc(size + ctlr_size, GFP_KERNEL);
+	ctlr = kzalloc(struct_size_with_data(ctlr, align, size), GFP_KERNEL);
 	if (!ctlr)
 		return NULL;
 
@@ -3114,7 +3114,7 @@ struct spi_controller *__spi_alloc_controller(struct device *dev,
 		ctlr->dev.class = &spi_master_class;
 	ctlr->dev.parent = dev;
 	pm_suspend_ignore_children(&ctlr->dev, true);
-	spi_controller_set_devdata(ctlr, (void *)ctlr + ctlr_size);
+	spi_controller_set_devdata(ctlr, struct_data_pointer(ctlr, align));
 
 	return ctlr;
 }
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 6/8] spi: Use new helpers from overflow.h in __spi_alloc_controller()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

On Wed, Feb 28, 2024 at 10:41:36PM +0200, Andy Shevchenko wrote:
> We have two new helpers struct_size_with_data() and struct_data_pointer()
> that we can utilize in __spi_alloc_controller(). Do it so.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Andy Shevchenko]

We have two new helpers struct_size_with_data() and struct_data_pointer()
that we can utilize in alloc_netdev_mqs() and netdev_priv(). Do it so.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/netdevice.h |  3 ++-
 net/core/dev.c            | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c41019f34179..d046dca18854 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -25,6 +25,7 @@
 #include <linux/bug.h>
 #include <linux/delay.h>
 #include <linux/atomic.h>
+#include <linux/overflow.h>
 #include <linux/prefetch.h>
 #include <asm/cache.h>
 #include <asm/byteorder.h>
@@ -2668,7 +2669,7 @@ void dev_net_set(struct net_device *dev, struct net *net)
  */
 static inline void *netdev_priv(const struct net_device *dev)
 {
-	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
+	return struct_data_pointer(dev, NETDEV_ALIGN);
 }
 
 /* Set the sysfs physical device reference for the network logical device
diff --git a/net/core/dev.c b/net/core/dev.c
index 69c3e3613372..80b765bb8ba2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10859,12 +10859,12 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 		return NULL;
 	}
 
-	alloc_size = sizeof(struct net_device);
-	if (sizeof_priv) {
+	if (sizeof_priv)
 		/* ensure 32-byte alignment of private area */
-		alloc_size = ALIGN(alloc_size, NETDEV_ALIGN);
-		alloc_size += sizeof_priv;
-	}
+		alloc_size = struct_size_with_data(p, NETDEV_ALIGN, sizeof_priv);
+	else
+		alloc_size = sizeof(struct net_device);
+
 	/* ensure 32-byte alignment of whole construct */
 	alloc_size += NETDEV_ALIGN - 1;
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Kees Cook]

On Wed, Feb 28, 2024 at 10:41:37PM +0200, Andy Shevchenko wrote:
> We have two new helpers struct_size_with_data() and struct_data_pointer()
> that we can utilize in alloc_netdev_mqs() and netdev_priv(). Do it so.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/netdevice.h |  3 ++-
>  net/core/dev.c            | 10 +++++-----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c41019f34179..d046dca18854 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -25,6 +25,7 @@
>  #include <linux/bug.h>
>  #include <linux/delay.h>
>  #include <linux/atomic.h>
> +#include <linux/overflow.h>
>  #include <linux/prefetch.h>
>  #include <asm/cache.h>
>  #include <asm/byteorder.h>
> @@ -2668,7 +2669,7 @@ void dev_net_set(struct net_device *dev, struct net *net)
>   */
>  static inline void *netdev_priv(const struct net_device *dev)
>  {
> -	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> +	return struct_data_pointer(dev, NETDEV_ALIGN);
>  }

I really don't like hiding these trailing allocations from the compiler.
Why can't something like this be done (totally untested):


diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 118c40258d07..dae6df4fb177 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2475,6 +2475,8 @@ struct net_device {
 	/** @page_pools: page pools created for this netdevice */
 	struct hlist_head	page_pools;
 #endif
+	u32			priv_size;
+	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
@@ -2665,7 +2667,7 @@ void dev_net_set(struct net_device *dev, struct net *net)
  */
 static inline void *netdev_priv(const struct net_device *dev)
 {
-	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
+	return dev->priv_data;
 }
 
 /* Set the sysfs physical device reference for the network logical device
diff --git a/net/core/dev.c b/net/core/dev.c
index cb2dab0feee0..afaaa3224656 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10814,18 +10814,14 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 		return NULL;
 	}
 
-	alloc_size = sizeof(struct net_device);
-	if (sizeof_priv) {
-		/* ensure 32-byte alignment of private area */
-		alloc_size = ALIGN(alloc_size, NETDEV_ALIGN);
-		alloc_size += sizeof_priv;
-	}
+	alloc_size = struct_size(p, priv_data, sizeof_priv);
 	/* ensure 32-byte alignment of whole construct */
 	alloc_size += NETDEV_ALIGN - 1;
 
 	p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL);
 	if (!p)
 		return NULL;
+	p->priv_size = sizeof_priv;
 
 	dev = PTR_ALIGN(p, NETDEV_ALIGN);
 	dev->padded = (char *)dev - (char *)p;


-- 
Kees Cook

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 01:46:10PM -0800, Kees Cook wrote:
> On Wed, Feb 28, 2024 at 10:41:37PM +0200, Andy Shevchenko wrote:

...

> >  static inline void *netdev_priv(const struct net_device *dev)
> >  {
> > -	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> > +	return struct_data_pointer(dev, NETDEV_ALIGN);
> >  }
> 
> I really don't like hiding these trailing allocations from the compiler.
> Why can't something like this be done (totally untested):

Below is interesting idea, now at least I started understanding your previous
comments.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Jakub Kicinski]

On Wed, 28 Feb 2024 13:46:10 -0800 Kees Cook wrote:
> I really don't like hiding these trailing allocations from the compiler.
> Why can't something like this be done (totally untested):
> 
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 118c40258d07..dae6df4fb177 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2475,6 +2475,8 @@ struct net_device {
>  	/** @page_pools: page pools created for this netdevice */
>  	struct hlist_head	page_pools;
>  #endif
> +	u32			priv_size;
> +	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);

I like, FWIW, please submit! :)

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Kees Cook]

On Wed, Feb 28, 2024 at 02:41:48PM -0800, Jakub Kicinski wrote:
> On Wed, 28 Feb 2024 13:46:10 -0800 Kees Cook wrote:
> > I really don't like hiding these trailing allocations from the compiler.
> > Why can't something like this be done (totally untested):
> > 
> > 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 118c40258d07..dae6df4fb177 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -2475,6 +2475,8 @@ struct net_device {
> >  	/** @page_pools: page pools created for this netdevice */
> >  	struct hlist_head	page_pools;
> >  #endif
> > +	u32			priv_size;
> > +	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
> 
> I like, FWIW, please submit! :)

So, I found several cases where struct net_device is included in the
middle of another structure, which makes my proposal more awkward. But I
also don't understand why it's in the _middle_. Shouldn't it always be
at the beginning (with priv stuff following it?)
Quick search and examined manually: git grep 'struct net_device [a-z0-9_]*;'

struct rtw89_dev
struct ath10k
etc.

Some even have two included (?)

But I still like the idea -- Gustavo has been solving these cases with
having two structs, e.g.:

struct net_device {
	...unchanged...
};

struct net_device_alloc {
	struct net_device	dev;
	u32			priv_size;
	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
};

And internals can use struct net_device_alloc...

-Kees

-- 
Kees Cook

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Gustavo A. R. Silva]

On 2/28/24 18:01, Kees Cook wrote:
> On Wed, Feb 28, 2024 at 02:41:48PM -0800, Jakub Kicinski wrote:
>> On Wed, 28 Feb 2024 13:46:10 -0800 Kees Cook wrote:
>>> I really don't like hiding these trailing allocations from the compiler.
>>> Why can't something like this be done (totally untested):
>>>
>>>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index 118c40258d07..dae6df4fb177 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -2475,6 +2475,8 @@ struct net_device {
>>>   	/** @page_pools: page pools created for this netdevice */
>>>   	struct hlist_head	page_pools;
>>>   #endif
>>> +	u32			priv_size;
>>> +	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
>>
>> I like, FWIW, please submit! :)
> 
> So, I found several cases where struct net_device is included in the
> middle of another structure, which makes my proposal more awkward. But I
> also don't understand why it's in the _middle_. Shouldn't it always be
> at the beginning (with priv stuff following it?)
> Quick search and examined manually: git grep 'struct net_device [a-z0-9_]*;'
> 
> struct rtw89_dev
> struct ath10k
> etc.
> 
> Some even have two included (?)
> 
> But I still like the idea -- Gustavo has been solving these cases with
> having two structs, e.g.:
> 
> struct net_device {
> 	...unchanged...
> };
> 
> struct net_device_alloc {
> 	struct net_device	dev;
> 	u32			priv_size;
> 	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
> };
> 
> And internals can use struct net_device_alloc...

Yep, we should really consider going with the above, otherwise we would
have to do something like the following, to avoid having the flexible-array
member nested in the middle of other structs:

struct net_device {
	struct_group_tagged(net_device_hdr, hdr,
		...
		u32			priv_size;
	);
	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
}

We are grouping together the members in `struct net_device`, except the
flexible-array member, into a tagged `struct net_device_hdr`. This allows
us to exclude the flex array from its inclusion in any other struct
that contains `struct net_device` as a member without having to create
a completely separate struct definition.

And let's take as example `struct hfi1_netdev_rx`, where `struct net_device` is
included in the beginning:

drivers/infiniband/hw/hfi1/netdev.h:
struct hfi1_netdev_rx {

-	struct net_device rx_napi;
+       struct net_device_hdr rx_napi;


         struct hfi1_devdata *dd;
         struct hfi1_netdev_rxq *rxq;
         int num_rx_q;
         int rmt_start;
         struct xarray dev_tbl;
         /* count of enabled napi polls */
         atomic_t enabled;
         /* count of netdevs on top */
         atomic_t netdevs;
};

Of course we would also have to update the code that access `struct net_device`
members through `rx_napi` in `struct hfi1_netdev_rx`.

I'm currently working on the above solution for all the cases where having two
separate structs is not currently feasible. And with that we are looking to enable
`-Wflex-array-member-not-at-end`

So, if we can prevent this from the beginning it'd be really great. :)

--
Gustavo

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Jakub Kicinski]

On Wed, 28 Feb 2024 18:49:25 -0600 Gustavo A. R. Silva wrote:
> struct net_device {
> 	struct_group_tagged(net_device_hdr, hdr,
> 		...
> 		u32			priv_size;
> 	);
> 	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
> }

No, no, that's not happening.

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Gustavo A. R. Silva]

On 2/28/24 18:57, Jakub Kicinski wrote:
> On Wed, 28 Feb 2024 18:49:25 -0600 Gustavo A. R. Silva wrote:
>> struct net_device {
>> 	struct_group_tagged(net_device_hdr, hdr,
>> 		...
>> 		u32			priv_size;
>> 	);
>> 	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
>> }
> 
> No, no, that's not happening.

Thanks, one less flex-struct to change. :)

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Jakub Kicinski]

On Wed, 28 Feb 2024 19:03:12 -0600 Gustavo A. R. Silva wrote:
> On 2/28/24 18:57, Jakub Kicinski wrote:
> > On Wed, 28 Feb 2024 18:49:25 -0600 Gustavo A. R. Silva wrote:  
> >> struct net_device {
> >> 	struct_group_tagged(net_device_hdr, hdr,
> >> 		...
> >> 		u32			priv_size;
> >> 	);
> >> 	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
> >> }  
> > 
> > No, no, that's not happening.  
> 
> Thanks, one less flex-struct to change. :)

I like the flex struct.
I don't like struct group around a 360LoC declaration just to avoid
having to fix up a handful of users.

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Gustavo A. R. Silva]

On 2/28/24 19:15, Jakub Kicinski wrote:
> On Wed, 28 Feb 2024 19:03:12 -0600 Gustavo A. R. Silva wrote:
>> On 2/28/24 18:57, Jakub Kicinski wrote:
>>> On Wed, 28 Feb 2024 18:49:25 -0600 Gustavo A. R. Silva wrote:
>>>> struct net_device {
>>>> 	struct_group_tagged(net_device_hdr, hdr,
>>>> 		...
>>>> 		u32			priv_size;
>>>> 	);
>>>> 	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
>>>> }
>>>
>>> No, no, that's not happening.
>>
>> Thanks, one less flex-struct to change. :)
> 
> I like the flex struct.
> I don't like struct group around a 360LoC declaration just to avoid
> having to fix up a handful of users.

That's what I mean. If we can prevent the flex array ending up in the
middle of a struct by any means, then I wouldn't have to change the
flex struct.

--
Gustavo

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Jakub Kicinski]

On Wed, 28 Feb 2024 16:01:49 -0800 Kees Cook wrote:
> So, I found several cases where struct net_device is included in the
> middle of another structure, which makes my proposal more awkward. But I
> also don't understand why it's in the _middle_. Shouldn't it always be
> at the beginning (with priv stuff following it?)
> Quick search and examined manually: git grep 'struct net_device [a-z0-9_]*;'
> 
> struct rtw89_dev
> struct ath10k
> etc.

Ugh, yes, the (ab)use of NAPI.

> Some even have two included (?)

And some seem to be cargo-culted from out-of-tree code and are unused :S

> But I still like the idea -- Gustavo has been solving these cases with
> having two structs, e.g.:
> 
> struct net_device {
> 	...unchanged...
> };
> 
> struct net_device_alloc {
> 	struct net_device	dev;
> 	u32			priv_size;
> 	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
> };
> 
> And internals can use struct net_device_alloc...

That's... less pretty. I'd rather push the ugly into the questionable
users. Make them either allocate the netdev dynamically and store 
a pointer, or move the netdev to the end of the struct.

But yeah, that's a bit of a cleanup :(

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Kees Cook]

On Wed, Feb 28, 2024 at 04:56:09PM -0800, Jakub Kicinski wrote:
> On Wed, 28 Feb 2024 16:01:49 -0800 Kees Cook wrote:
> > So, I found several cases where struct net_device is included in the
> > middle of another structure, which makes my proposal more awkward. But I
> > also don't understand why it's in the _middle_. Shouldn't it always be
> > at the beginning (with priv stuff following it?)
> > Quick search and examined manually: git grep 'struct net_device [a-z0-9_]*;'
> > 
> > struct rtw89_dev
> > struct ath10k
> > etc.
> 
> Ugh, yes, the (ab)use of NAPI.
> 
> > Some even have two included (?)
> 
> And some seem to be cargo-culted from out-of-tree code and are unused :S

Ah, which can I remove?

> That's... less pretty. I'd rather push the ugly into the questionable
> users. Make them either allocate the netdev dynamically and store 
> a pointer, or move the netdev to the end of the struct.
> 
> But yeah, that's a bit of a cleanup :(

So far, it's not too bad. I'm doing some test builds now.


As a further aside, this code:

        struct net_device *dev;
	...
        struct net_device *p;
	...
        /* ensure 32-byte alignment of whole construct */
        alloc_size += NETDEV_ALIGN - 1;
        p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL);
	...
        dev = PTR_ALIGN(p, NETDEV_ALIGN);

Really screams for a dynamic-sized (bucketed) kmem_cache_alloc
API. Alignment constraints can be described in a regular kmem_cache
allocator (rather than this open-coded version). I've been intending to
build that for struct msg_msg for a while now, and here's another user. :)

-Kees

-- 
Kees Cook

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Jakub Kicinski]

On Thu, 29 Feb 2024 11:08:58 -0800 Kees Cook wrote:
> > And some seem to be cargo-culted from out-of-tree code and are unused :S  
> 
> Ah, which can I remove?

The one in igc.h does not seem to be referenced by anything in the igc
directory. Pretty sure it's unused.

> As a further aside, this code:
> 
>         struct net_device *dev;
> 	...
>         struct net_device *p;
> 	...
>         /* ensure 32-byte alignment of whole construct */
>         alloc_size += NETDEV_ALIGN - 1;
>         p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL);
> 	...
>         dev = PTR_ALIGN(p, NETDEV_ALIGN);
> 
> Really screams for a dynamic-sized (bucketed) kmem_cache_alloc
> API. Alignment constraints can be described in a regular kmem_cache
> allocator (rather than this open-coded version). I've been intending to
> build that for struct msg_msg for a while now, and here's another user. :)

TBH I'm not sure why we align it :S
NETDEV_ALIGN is 32B so maybe some old cache aligning thing?

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Kees Cook]

On Thu, Feb 29, 2024 at 11:37:06AM -0800, Jakub Kicinski wrote:
> On Thu, 29 Feb 2024 11:08:58 -0800 Kees Cook wrote:
> > > And some seem to be cargo-culted from out-of-tree code and are unused :S  
> > 
> > Ah, which can I remove?
> 
> The one in igc.h does not seem to be referenced by anything in the igc
> directory. Pretty sure it's unused.

I'll double check. After trying to do a few conversions I really hit
stuff I didn't like, so I took a slightly different approach in the
patch I sent.

-- 
Kees Cook

Re: [PATCH v4 7/8] net-device: Use new helpers from overflow.h in netdevice APIs

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 04:01:49PM -0800, Kees Cook wrote:
> On Wed, Feb 28, 2024 at 02:41:48PM -0800, Jakub Kicinski wrote:
> > On Wed, 28 Feb 2024 13:46:10 -0800 Kees Cook wrote:

...

> But I still like the idea -- Gustavo has been solving these cases with
> having two structs, e.g.:
> 
> struct net_device {
> 	...unchanged...
> };
> 
> struct net_device_alloc {
> 	struct net_device	dev;
> 	u32			priv_size;
> 	u8			priv_data[] __counted_by(priv_size) __aligned(NETDEV_ALIGN);
> };
> 
> And internals can use struct net_device_alloc...

I just realized that I made same approach in 

f6d7f050e258 ("spi: Don't use flexible array in struct spi_message definition")
75e308ffc4f0 ("spi: Use struct_size() helper")


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 8/8] dmaengine: ste_dma40: Use new helpers from overflow.h

[Andy Shevchenko]

We have two new helpers struct_size_with_data() and struct_data_pointer()
that we can utilize in d40_hw_detect_init(). Do it so.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/ste_dma40.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
index 2c489299148e..bead3b8836c7 100644
--- a/drivers/dma/ste_dma40.c
+++ b/drivers/dma/ste_dma40.c
@@ -15,6 +15,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/log2.h>
+#include <linux/overflow.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/err.h>
@@ -3141,6 +3142,7 @@ static int __init d40_hw_detect_init(struct platform_device *pdev,
 	int num_log_chans;
 	int num_phy_chans;
 	int num_memcpy_chans;
+	size_t sz;
 	int i;
 	u32 pid;
 	u32 cid;
@@ -3207,11 +3209,9 @@ static int __init d40_hw_detect_init(struct platform_device *pdev,
 		 "hardware rev: %d with %d physical and %d logical channels\n",
 		 rev, num_phy_chans, num_log_chans);
 
-	base = devm_kzalloc(dev,
-		ALIGN(sizeof(struct d40_base), 4) +
-		(num_phy_chans + num_log_chans + num_memcpy_chans) *
-		sizeof(struct d40_chan), GFP_KERNEL);
-
+	sz = array_size(num_phy_chans + num_log_chans + num_memcpy_chans,
+			sizeof(struct d40_chan));
+	base = devm_kzalloc(dev, struct_size_with_data(base, 4, sz), GFP_KERNEL);
 	if (!base)
 		return -ENOMEM;
 
@@ -3223,7 +3223,7 @@ static int __init d40_hw_detect_init(struct platform_device *pdev,
 	base->virtbase = virtbase;
 	base->plat_data = plat_data;
 	base->dev = dev;
-	base->phy_chans = ((void *)base) + ALIGN(sizeof(struct d40_base), 4);
+	base->phy_chans = struct_data_pointer(base, 4);
 	base->log_chans = &base->phy_chans[num_phy_chans];
 
 	if (base->plat_data->num_of_phy_chans == 14) {
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v4 8/8] dmaengine: ste_dma40: Use new helpers from overflow.h

[Linus Walleij]

On Wed, Feb 28, 2024 at 9:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> We have two new helpers struct_size_with_data() and struct_data_pointer()
> that we can utilize in d40_hw_detect_init(). Do it so.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Wow really neat! Much easier to read and understand the code like this.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 8/8] dmaengine: ste_dma40: Use new helpers from overflow.h

[Andy Shevchenko]

On Thu, Feb 29, 2024 at 03:14:03PM +0100, Linus Walleij wrote:
> On Wed, Feb 28, 2024 at 9:49 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > We have two new helpers struct_size_with_data() and struct_data_pointer()
> > that we can utilize in d40_hw_detect_init(). Do it so.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Wow really neat! Much easier to read and understand the code like this.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks, but Kees has seems even better suggestion.

-- 
With Best Regards,
Andy Shevchenko