| Message ID | 20240228204919.3680786-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | iio: core: New macros and making use of them | expand |
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>
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 > >
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.
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.
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;
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)
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.
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! :)
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
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
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 :(
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.
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. :)
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.
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
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")
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
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?
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.
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(-)