| Message ID | 20230810103509.163225-2-przemyslaw.kitszel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | introduce DEFINE_FLEX() macro | expand |
From: Przemek Kitszel <przemyslaw.kitszel@intel.com> Date: Thu, 10 Aug 2023 06:35:03 -0400 > Add DEFINE_FLEX() macro for on-stack allocations of structs with > flexible array member. > > Add also const_flex_size() macro, that reads size of structs > allocated by DEFINE_FLEX(). > > Using underlying array for on-stack storage lets us to declare > known-at-compile-time structures without kzalloc(). > > Actual usage for ice driver is in following patches of the series. > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- > v1: change macro name; add macro for size read; > accept struct type instead of ptr to it; change alignment; > --- > include/linux/overflow.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index f9b60313eaea..21a4410799eb 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -309,4 +309,31 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) > #define struct_size_t(type, member, count) \ > struct_size((type *)NULL, member, count) > > +/** > + * DEFINE_FLEX() - Define a zeroed, on-stack, instance of @type structure with > + * a trailing flexible array member. > + * > + * @type: structure type name, including "struct" keyword. > + * @name: Name for a variable to define. > + * @member: Name of the array member. > + * @count: Number of elements in the array; must be compile-time const. > + */ > +#define DEFINE_FLEX(type, name, member, count) \ > + union { \ > + u8 bytes[struct_size_t(type, member, count)]; \ > + type obj; \ > + } name##_u __aligned(_Alignof(type)) = {}; \ Hmm. Should we always zero it? The onstack variables are not zeroed automatically. I realize the onstack structures declared via this macro can't be initialized on the same line via = { }, but OTOH memset() with const len and for onstack structs usually gets expanded into static initialization. The main reason why I'm asking is that sometimes we don't need zeroing at all, for example for small structures when we then manually set all the fields either way. I don't think hiding static initialization inside the macro is a good move. > + type *name = (type *)&name##_u > + > +/** > + * const_flex_size() - Get size of on-stack instance of structure with > + * a trailing flexible array member. > + * > + * @name: Name of the variable, the one defined by DEFINE_FLEX() macro above. > + * > + * Get size of @name, which is equivalent to struct_size(name, array, count), > + * but does not require (repeating) last two arguments. > + */ > +#define const_flex_size(name) __builtin_object_size(name, 1) > + > #endif /* __LINUX_OVERFLOW_H */ Thanks, Olek
On Thu, Aug 10, 2023 at 06:24:47PM +0200, Alexander Lobakin wrote: > From: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Date: Thu, 10 Aug 2023 06:35:03 -0400 > > > Add DEFINE_FLEX() macro for on-stack allocations of structs with > > flexible array member. > > > > Add also const_flex_size() macro, that reads size of structs > > allocated by DEFINE_FLEX(). > > > > Using underlying array for on-stack storage lets us to declare > > known-at-compile-time structures without kzalloc(). > > > > Actual usage for ice driver is in following patches of the series. > > > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > --- > > v1: change macro name; add macro for size read; > > accept struct type instead of ptr to it; change alignment; > > --- > > include/linux/overflow.h | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > index f9b60313eaea..21a4410799eb 100644 > > --- a/include/linux/overflow.h > > +++ b/include/linux/overflow.h > > @@ -309,4 +309,31 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) > > #define struct_size_t(type, member, count) \ > > struct_size((type *)NULL, member, count) > > > > +/** > > + * DEFINE_FLEX() - Define a zeroed, on-stack, instance of @type structure with > > + * a trailing flexible array member. > > + * > > + * @type: structure type name, including "struct" keyword. > > + * @name: Name for a variable to define. > > + * @member: Name of the array member. > > + * @count: Number of elements in the array; must be compile-time const. > > + */ > > +#define DEFINE_FLEX(type, name, member, count) \ > > + union { \ > > + u8 bytes[struct_size_t(type, member, count)]; \ > > + type obj; \ > > + } name##_u __aligned(_Alignof(type)) = {}; \ > > Hmm. Should we always zero it? The onstack variables are not zeroed > automatically. > I realize the onstack structures declared via this macro can't be > initialized on the same line via = { }, but OTOH memset() with const len > and for onstack structs usually gets expanded into static initialization. > The main reason why I'm asking is that sometimes we don't need zeroing > at all, for example for small structures when we then manually set all > the fields either way. I don't think hiding static initialization inside > the macro is a good move. I strongly think this should be always zeroed. In the case where all members are initialized, the zeroing will be elided by the compiler during Dead Store Elimination optimization passes. Additionally, padding, if present, would not get zeroed even if all members got initialized separately, and if any memcpy() of the structure was made, it would contain leaked memory contents. Any redundant initializations will be avoided by the compiler, so let's be safe by default and init the whole thing to zero. -Kees
On Thu, Aug 10, 2023 at 06:35:03AM -0400, Przemek Kitszel wrote: > Add DEFINE_FLEX() macro for on-stack allocations of structs with > flexible array member. > > Add also const_flex_size() macro, that reads size of structs > allocated by DEFINE_FLEX(). > > Using underlying array for on-stack storage lets us to declare > known-at-compile-time structures without kzalloc(). > > Actual usage for ice driver is in following patches of the series. > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- > v1: change macro name; add macro for size read; > accept struct type instead of ptr to it; change alignment; > --- > include/linux/overflow.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index f9b60313eaea..21a4410799eb 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -309,4 +309,31 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) > #define struct_size_t(type, member, count) \ > struct_size((type *)NULL, member, count) > > +/** > + * DEFINE_FLEX() - Define a zeroed, on-stack, instance of @type structure with > + * a trailing flexible array member. > + * > + * @type: structure type name, including "struct" keyword. > + * @name: Name for a variable to define. > + * @member: Name of the array member. > + * @count: Number of elements in the array; must be compile-time const. > + */ > +#define DEFINE_FLEX(type, name, member, count) \ > + union { \ > + u8 bytes[struct_size_t(type, member, count)]; \ > + type obj; \ > + } name##_u __aligned(_Alignof(type)) = {}; \ > + type *name = (type *)&name##_u We'll need another macro when __counted_by is needed, but yes, if all of these structs use non-native endian counters, we can't require it in the base macro. (i.e. not now -- this is fine as-is.) > + > +/** > + * const_flex_size() - Get size of on-stack instance of structure with > + * a trailing flexible array member. > + * > + * @name: Name of the variable, the one defined by DEFINE_FLEX() macro above. > + * > + * Get size of @name, which is equivalent to struct_size(name, array, count), > + * but does not require (repeating) last two arguments. > + */ > +#define const_flex_size(name) __builtin_object_size(name, 1) Naming is hard. ;) I don't like "const" here (it's not a storage class). But more importantly, this calculation ("how big is this thing actually?") gets used a lot in the fortify routines, so I'd prefer exposing those macros (from fortify-string.h): diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index c88488715a39..4b788fa0c576 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -352,6 +352,18 @@ struct ftrace_likely_data { # define __realloc_size(x, ...) #endif +/* + * When the size of an allocated object is needed, use the best available + * mechanism to find it. (For cases where sizeof() cannot be used.) + */ +#if __has_builtin(__builtin_dynamic_object_size) +#define __struct_size(p) __builtin_dynamic_object_size(p, 0) +#define __member_size(p) __builtin_dynamic_object_size(p, 1) +#else +#define __struct_size(p) __builtin_object_size(p, 0) +#define __member_size(p) __builtin_object_size(p, 1) +#endif + #ifndef asm_volatile_goto #define asm_volatile_goto(x...) asm goto(x) #endif diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index da51a83b2829..1e7711185ec6 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -93,13 +93,9 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) #if __has_builtin(__builtin_dynamic_object_size) #define POS __pass_dynamic_object_size(1) #define POS0 __pass_dynamic_object_size(0) -#define __struct_size(p) __builtin_dynamic_object_size(p, 0) -#define __member_size(p) __builtin_dynamic_object_size(p, 1) #else #define POS __pass_object_size(1) #define POS0 __pass_object_size(0) -#define __struct_size(p) __builtin_object_size(p, 0) -#define __member_size(p) __builtin_object_size(p, 1) #endif #define __compiletime_lessthan(bounds, length) ( \ And the way DEFINE_FLEX is built, __struct_size() and __member_size() will give the same result (which is what I was concerned about for FORTIFY_SOURCE's use of __member_size not "seeing" the flexible array members). In this case, I think using __struct_size() in place of const_flex_size() in the patch series is the way to go.
On 8/10/23 20:46, Kees Cook wrote: > On Thu, Aug 10, 2023 at 06:35:03AM -0400, Przemek Kitszel wrote: >> Add DEFINE_FLEX() macro for on-stack allocations of structs with >> flexible array member. >> >> Add also const_flex_size() macro, that reads size of structs >> allocated by DEFINE_FLEX(). >> >> Using underlying array for on-stack storage lets us to declare >> known-at-compile-time structures without kzalloc(). >> >> Actual usage for ice driver is in following patches of the series. >> >> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> --- >> v1: change macro name; add macro for size read; >> accept struct type instead of ptr to it; change alignment; >> --- >> include/linux/overflow.h | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >> index f9b60313eaea..21a4410799eb 100644 >> --- a/include/linux/overflow.h >> +++ b/include/linux/overflow.h >> @@ -309,4 +309,31 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) >> #define struct_size_t(type, member, count) \ >> struct_size((type *)NULL, member, count) >> >> +/** >> + * DEFINE_FLEX() - Define a zeroed, on-stack, instance of @type structure with >> + * a trailing flexible array member. >> + * >> + * @type: structure type name, including "struct" keyword. >> + * @name: Name for a variable to define. >> + * @member: Name of the array member. >> + * @count: Number of elements in the array; must be compile-time const. >> + */ >> +#define DEFINE_FLEX(type, name, member, count) \ >> + union { \ >> + u8 bytes[struct_size_t(type, member, count)]; \ >> + type obj; \ >> + } name##_u __aligned(_Alignof(type)) = {}; \ >> + type *name = (type *)&name##_u > > We'll need another macro when __counted_by is needed, but yes, if all of > these structs use non-native endian counters, we can't require it in the > base macro. (i.e. not now -- this is fine as-is.) > >> + >> +/** >> + * const_flex_size() - Get size of on-stack instance of structure with >> + * a trailing flexible array member. >> + * >> + * @name: Name of the variable, the one defined by DEFINE_FLEX() macro above. >> + * >> + * Get size of @name, which is equivalent to struct_size(name, array, count), >> + * but does not require (repeating) last two arguments. >> + */ >> +#define const_flex_size(name) __builtin_object_size(name, 1) > > Naming is hard. ;) I don't like "const" here (it's not a storage > class). But more importantly, this calculation ("how big is this thing > actually?") gets used a lot in the fortify routines, so I'd prefer > exposing those macros (from fortify-string.h): > > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index c88488715a39..4b788fa0c576 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -352,6 +352,18 @@ struct ftrace_likely_data { > # define __realloc_size(x, ...) > #endif > > +/* > + * When the size of an allocated object is needed, use the best available > + * mechanism to find it. (For cases where sizeof() cannot be used.) > + */ > +#if __has_builtin(__builtin_dynamic_object_size) > +#define __struct_size(p) __builtin_dynamic_object_size(p, 0) > +#define __member_size(p) __builtin_dynamic_object_size(p, 1) > +#else > +#define __struct_size(p) __builtin_object_size(p, 0) > +#define __member_size(p) __builtin_object_size(p, 1) > +#endif > + > #ifndef asm_volatile_goto > #define asm_volatile_goto(x...) asm goto(x) > #endif > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index da51a83b2829..1e7711185ec6 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -93,13 +93,9 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) > #if __has_builtin(__builtin_dynamic_object_size) > #define POS __pass_dynamic_object_size(1) > #define POS0 __pass_dynamic_object_size(0) > -#define __struct_size(p) __builtin_dynamic_object_size(p, 0) > -#define __member_size(p) __builtin_dynamic_object_size(p, 1) > #else > #define POS __pass_object_size(1) > #define POS0 __pass_object_size(0) > -#define __struct_size(p) __builtin_object_size(p, 0) > -#define __member_size(p) __builtin_object_size(p, 1) > #endif > > #define __compiletime_lessthan(bounds, length) ( \ > > > And the way DEFINE_FLEX is built, __struct_size() and __member_size() > will give the same result (which is what I was concerned about for > FORTIFY_SOURCE's use of __member_size not "seeing" the flexible array > members). > > In this case, I think using __struct_size() in place of const_flex_size() > in the patch series is the way to go. > Thanks! I was a bit too afraid to move it out of fortify-string.h, I guess whole family have to go together (to keep related code close to each other)?
From: Kees Cook <keescook@chromium.org> Date: Thu, 10 Aug 2023 11:31:45 -0700 > On Thu, Aug 10, 2023 at 06:24:47PM +0200, Alexander Lobakin wrote: >> From: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Date: Thu, 10 Aug 2023 06:35:03 -0400 >> >>> Add DEFINE_FLEX() macro for on-stack allocations of structs with >>> flexible array member. >>> >>> Add also const_flex_size() macro, that reads size of structs >>> allocated by DEFINE_FLEX(). >>> >>> Using underlying array for on-stack storage lets us to declare >>> known-at-compile-time structures without kzalloc(). >>> >>> Actual usage for ice driver is in following patches of the series. >>> >>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >>> --- >>> v1: change macro name; add macro for size read; >>> accept struct type instead of ptr to it; change alignment; >>> --- >>> include/linux/overflow.h | 27 +++++++++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> >>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >>> index f9b60313eaea..21a4410799eb 100644 >>> --- a/include/linux/overflow.h >>> +++ b/include/linux/overflow.h >>> @@ -309,4 +309,31 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) >>> #define struct_size_t(type, member, count) \ >>> struct_size((type *)NULL, member, count) >>> >>> +/** >>> + * DEFINE_FLEX() - Define a zeroed, on-stack, instance of @type structure with >>> + * a trailing flexible array member. >>> + * >>> + * @type: structure type name, including "struct" keyword. >>> + * @name: Name for a variable to define. >>> + * @member: Name of the array member. >>> + * @count: Number of elements in the array; must be compile-time const. >>> + */ >>> +#define DEFINE_FLEX(type, name, member, count) \ >>> + union { \ >>> + u8 bytes[struct_size_t(type, member, count)]; \ >>> + type obj; \ >>> + } name##_u __aligned(_Alignof(type)) = {}; \ >> >> Hmm. Should we always zero it? The onstack variables are not zeroed >> automatically. >> I realize the onstack structures declared via this macro can't be >> initialized on the same line via = { }, but OTOH memset() with const len >> and for onstack structs usually gets expanded into static initialization. >> The main reason why I'm asking is that sometimes we don't need zeroing >> at all, for example for small structures when we then manually set all >> the fields either way. I don't think hiding static initialization inside >> the macro is a good move. > > I strongly think this should be always zeroed. In the case where all > members are initialized, the zeroing will be elided by the compiler > during Dead Store Elimination optimization passes. > > Additionally, padding, if present, would not get zeroed even if all > members got initialized separately, and if any memcpy() of the structure > was made, it would contain leaked memory contents. > > Any redundant initializations will be avoided by the compiler, so let's > be safe by default and init the whole thing to zero. Sounds good, thanks for the explanation! Always nice to hear about some compiler internals :) > > -Kees > Thanks, Olek
diff --git a/include/linux/overflow.h b/include/linux/overflow.h index f9b60313eaea..21a4410799eb 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -309,4 +309,31 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) #define struct_size_t(type, member, count) \ struct_size((type *)NULL, member, count) +/** + * DEFINE_FLEX() - Define a zeroed, on-stack, instance of @type structure with + * a trailing flexible array member. + * + * @type: structure type name, including "struct" keyword. + * @name: Name for a variable to define. + * @member: Name of the array member. + * @count: Number of elements in the array; must be compile-time const. + */ +#define DEFINE_FLEX(type, name, member, count) \ + union { \ + u8 bytes[struct_size_t(type, member, count)]; \ + type obj; \ + } name##_u __aligned(_Alignof(type)) = {}; \ + type *name = (type *)&name##_u + +/** + * const_flex_size() - Get size of on-stack instance of structure with + * a trailing flexible array member. + * + * @name: Name of the variable, the one defined by DEFINE_FLEX() macro above. + * + * Get size of @name, which is equivalent to struct_size(name, array, count), + * but does not require (repeating) last two arguments. + */ +#define const_flex_size(name) __builtin_object_size(name, 1) + #endif /* __LINUX_OVERFLOW_H */
Add DEFINE_FLEX() macro for on-stack allocations of structs with flexible array member. Add also const_flex_size() macro, that reads size of structs allocated by DEFINE_FLEX(). Using underlying array for on-stack storage lets us to declare known-at-compile-time structures without kzalloc(). Actual usage for ice driver is in following patches of the series. Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> --- v1: change macro name; add macro for size read; accept struct type instead of ptr to it; change alignment; --- include/linux/overflow.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)