| Message ID | 20230801111923.118268-2-przemyslaw.kitszel@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | introduce DECLARE_FLEX() macro | expand |
From: Przemek Kitszel <przemyslaw.kitszel@intel.com> Date: Tue, 1 Aug 2023 13:19:22 +0200 > Add DECLARE_FLEX() macro for on-stack allocations of structs with > flexible array member. > > Using underlying array for on-stack storage lets us to declare known > on compile-time structures without kzalloc(). > > Actual usage for ice driver is in next patch of the series. > > Note that "struct" kw and "*" char is moved to the caller, to both: > have shorter macro name, and have more natural type specification > in the driver code (IOW not hiding an actual type of var). > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- > include/linux/overflow.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index f9b60313eaea..403b7ec120a2 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -309,4 +309,18 @@ 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) > > +/** > + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing > + * flexible array. > + * @type: Pointer to structure type, including "struct" keyword and "*" char. > + * @name: Name for a (pointer) variable to create. > + * @member: Name of the array member. > + * @count: Number of elements in the array; must be compile-time const. > + * > + * Declare an instance of structure *@type with trailing flexible array. > + */ > +#define DECLARE_FLEX(type, name, member, count) \ > + u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\ 1. You can use struct_size_t() instead of open-coding it. 2. Maybe use alignof(type) instead of 8? Some structures have larger alignment requirements. > + type name = (type)&name##_buf In general, I still think DECLARE_FLEX(struct foo) is better than DECLARE_FLEX(struct foo *). Looking at container_of(), struct_size_t() etc., they all take `type`, not `type *`, so even from the consistency perspective your solution is not optimal to me. > + > #endif /* __LINUX_OVERFLOW_H */ Thanks, Olek
On 8/1/23 15:54, Alexander Lobakin wrote: > From: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Date: Tue, 1 Aug 2023 13:19:22 +0200 > >> Add DECLARE_FLEX() macro for on-stack allocations of structs with >> flexible array member. >> >> Using underlying array for on-stack storage lets us to declare known >> on compile-time structures without kzalloc(). >> >> Actual usage for ice driver is in next patch of the series. >> >> Note that "struct" kw and "*" char is moved to the caller, to both: >> have shorter macro name, and have more natural type specification >> in the driver code (IOW not hiding an actual type of var). >> >> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> --- >> include/linux/overflow.h | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >> index f9b60313eaea..403b7ec120a2 100644 >> --- a/include/linux/overflow.h >> +++ b/include/linux/overflow.h >> @@ -309,4 +309,18 @@ 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) >> >> +/** >> + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing >> + * flexible array. >> + * @type: Pointer to structure type, including "struct" keyword and "*" char. >> + * @name: Name for a (pointer) variable to create. >> + * @member: Name of the array member. >> + * @count: Number of elements in the array; must be compile-time const. >> + * >> + * Declare an instance of structure *@type with trailing flexible array. >> + */ >> +#define DECLARE_FLEX(type, name, member, count) \ >> + u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\ > > 1. You can use struct_size_t() instead of open-coding it. with ptr param, not feasible, but otherwise, of course will do it (see below) > 2. Maybe use alignof(type) instead of 8? Some structures have larger > alignment requirements. Sure, thanks! > >> + type name = (type)&name##_buf > > In general, I still think DECLARE_FLEX(struct foo) is better than > DECLARE_FLEX(struct foo *). I have started with that version, and that would prevent your question no. 1 :) So there is additional advantage to that. > Looking at container_of(), struct_size_t() > etc., they all take `type`, not `type *`, so even from the consistency > perspective your solution is not optimal to me. The two you have mentioned are "getter" macros. Random two from me, that actually declare something are: #define DEVICE_ATTR_RW(_name) \ struct device_attribute dev_attr_##_name = __ATTR_RW(_name) #define DECLARE_BITMAP(name, bits) \ unsigned long name[BITS_TO_LONGS(bits)] Even if they don't take @type param, they declare variable of some non-pointer type. Both variants have some logic that supports them, and some disadvantages: ptr-arg: user declares sth as ptr, but it takes "a lot" of space just-type-arg: user declares foo, but it's "*foo" actually, so "foo.bar" does not work. I have no strong opinion here, so will just switch to pure-type param. > Thanks, > Olek
From: Przemek Kitszel <przemyslaw.kitszel@intel.com> Date: Tue, 1 Aug 2023 16:18:44 +0200 > On 8/1/23 15:54, Alexander Lobakin wrote: >> From: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Date: Tue, 1 Aug 2023 13:19:22 +0200 >> >>> Add DECLARE_FLEX() macro for on-stack allocations of structs with >>> flexible array member. >>> >>> Using underlying array for on-stack storage lets us to declare known >>> on compile-time structures without kzalloc(). >>> >>> Actual usage for ice driver is in next patch of the series. >>> >>> Note that "struct" kw and "*" char is moved to the caller, to both: >>> have shorter macro name, and have more natural type specification >>> in the driver code (IOW not hiding an actual type of var). >>> >>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >>> --- >>> include/linux/overflow.h | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >>> index f9b60313eaea..403b7ec120a2 100644 >>> --- a/include/linux/overflow.h >>> +++ b/include/linux/overflow.h >>> @@ -309,4 +309,18 @@ 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) >>> +/** >>> + * DECLARE_FLEX() - Declare an on-stack instance of structure with >>> trailing >>> + * flexible array. >>> + * @type: Pointer to structure type, including "struct" keyword and >>> "*" char. >>> + * @name: Name for a (pointer) variable to create. >>> + * @member: Name of the array member. >>> + * @count: Number of elements in the array; must be compile-time const. >>> + * >>> + * Declare an instance of structure *@type with trailing flexible >>> array. >>> + */ >>> +#define DECLARE_FLEX(type, name, member, count) \ >>> + u8 name##_buf[struct_size((type)NULL, member, count)] >>> __aligned(8) = {};\ >> >> 1. You can use struct_size_t() instead of open-coding it. > > with ptr param, not feasible, but otherwise, of course will do it (see struct_size_t(typeof(*(type)NULL), member, count) Jokin :D > below) > >> 2. Maybe use alignof(type) instead of 8? Some structures have larger >> alignment requirements. > > Sure, thanks! > >> >>> + type name = (type)&name##_buf >> >> In general, I still think DECLARE_FLEX(struct foo) is better than >> DECLARE_FLEX(struct foo *). > > I have started with that version, and that would prevent your question > no. 1 :) So there is additional advantage to that. > >> Looking at container_of(), struct_size_t() >> etc., they all take `type`, not `type *`, so even from the consistency >> perspective your solution is not optimal to me. > > The two you have mentioned are "getter" macros. Random two from me, that > actually declare something are: > > #define DEVICE_ATTR_RW(_name) \ > struct device_attribute dev_attr_##_name = __ATTR_RW(_name) > > #define DECLARE_BITMAP(name, bits) \ > unsigned long name[BITS_TO_LONGS(bits)] > > Even if they don't take @type param, they declare variable of some > non-pointer type. > > Both variants have some logic that supports them, and some disadvantages: > ptr-arg: user declares sth as ptr, but it takes "a lot" of space > just-type-arg: user declares foo, but it's "*foo" actually, so "foo.bar" > does not work. Same as DECLARE_BITMAP() actually: it always declares an array, so that it's then __set_bit(FOO, bitmap), not __set_bit(FOO, &bitmap). One more argument for "just-type": yes, the name you pass to the macro is exported as a pointer, but you occupy the size of the type (plus tail elements), not a pointer, on the stack. > > I have no strong opinion here, so will just switch to pure-type param. > >> Thanks, >> Olek > Thanks, Olek
On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote: > Add DECLARE_FLEX() macro for on-stack allocations of structs with > flexible array member. I like this idea! One terminology nit: I think this should be called "DEFINE_...", since it's a specific instantiation. Other macros in the kernel seem to confuse this a lot, though. Yay naming. > Using underlying array for on-stack storage lets us to declare known > on compile-time structures without kzalloc(). Hmpf, this appears to immediately trip over any (future) use of __counted_by()[1] for these (since the counted-by member would be initialized to zero), but I think I have a solution. (See below.) > > Actual usage for ice driver is in next patch of the series. > > Note that "struct" kw and "*" char is moved to the caller, to both: > have shorter macro name, and have more natural type specification > in the driver code (IOW not hiding an actual type of var). > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- > include/linux/overflow.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index f9b60313eaea..403b7ec120a2 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -309,4 +309,18 @@ 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) > > +/** > + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing > + * flexible array. > + * @type: Pointer to structure type, including "struct" keyword and "*" char. > + * @name: Name for a (pointer) variable to create. > + * @member: Name of the array member. > + * @count: Number of elements in the array; must be compile-time const. > + * > + * Declare an instance of structure *@type with trailing flexible array. > + */ > +#define DECLARE_FLEX(type, name, member, count) \ > + u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\ > + type name = (type)&name##_buf > + > #endif /* __LINUX_OVERFLOW_H */ I was disappointed to discover that only global (static) initializers would work for a flex array member. :( i.e. this works: struct foo { unsigned long flags; unsigned char count; int array[] __counted_by(count); }; struct foo global = { .count = 1, .array = { 0 }, }; But I can't do that on the stack. :P So, yes, it seems like the u8 array trick is needed. It looks like Alexander already suggested this, and I agree, instead of __aligned(8), please use "__aligned(_Alignof(type))". As for "*" or not, I would tend to agree that always requiring "*" when using the macro seems redundant. Initially I was struggling to make __counted_by work, but it seems we can use an initializer for that member, as long as we don't touch the flexible array member in the initializer. So we just need to add the counted-by member to the macro, and use a union to do the initialization. And if we take the address of the union (and not the struct within it), the compiler will see the correct object size with __builtin_object_size: #define DEFINE_FLEX(type, name, flex, counter, count) \ union { \ u8 bytes[struct_size_t(type, flex, count)]; \ type obj; \ } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \ /* take address of whole union to get the correct __builtin_object_size */ \ type *name = (type *)&name##_u i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc) works correctly here, but breaks (sees a zero-sized flex array member) if this macro ends with: type *name = &name##_u.obj -Kees [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb
On 8/2/23 00:31, Kees Cook wrote: > On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote: >> Add DECLARE_FLEX() macro for on-stack allocations of structs with >> flexible array member. > > I like this idea! > > One terminology nit: I think this should be called "DEFINE_...", since > it's a specific instantiation. Other macros in the kernel seem to confuse > this a lot, though. Yay naming. Thanks, makes sense! > >> Using underlying array for on-stack storage lets us to declare known >> on compile-time structures without kzalloc(). > > Hmpf, this appears to immediately trip over any (future) use of > __counted_by()[1] for these (since the counted-by member would be > initialized to zero), but I think I have a solution. (See below.) > >> >> Actual usage for ice driver is in next patch of the series. >> >> Note that "struct" kw and "*" char is moved to the caller, to both: >> have shorter macro name, and have more natural type specification >> in the driver code (IOW not hiding an actual type of var). >> >> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> --- >> include/linux/overflow.h | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >> index f9b60313eaea..403b7ec120a2 100644 >> --- a/include/linux/overflow.h >> +++ b/include/linux/overflow.h >> @@ -309,4 +309,18 @@ 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) >> >> +/** >> + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing >> + * flexible array. >> + * @type: Pointer to structure type, including "struct" keyword and "*" char. >> + * @name: Name for a (pointer) variable to create. >> + * @member: Name of the array member. >> + * @count: Number of elements in the array; must be compile-time const. >> + * >> + * Declare an instance of structure *@type with trailing flexible array. >> + */ >> +#define DECLARE_FLEX(type, name, member, count) \ >> + u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\ >> + type name = (type)&name##_buf >> + >> #endif /* __LINUX_OVERFLOW_H */ > > I was disappointed to discover that only global (static) initializers > would work for a flex array member. :( > > i.e. this works: > > struct foo { > unsigned long flags; > unsigned char count; So bad that in the ice driver (perhaps others too), we have cases that there is no counter or it has different meaning. (potentially "complicated" meaning - ice' struct ice_aqc_alloc_free_res_elem has "__le16 num_elems", so could not be used verbatim, and it's not actual counter either :/ (I was fooled by such assumption here [2]). Perhaps recent series by Olek [3] is also good illustration of hard cases for __counted_by() > int array[] __counted_by(count); > }; > > struct foo global = { > .count = 1, > .array = { 0 }, > }; > > But I can't do that on the stack. :P So, yes, it seems like the u8 array > trick is needed. > > It looks like Alexander already suggested this, and I agree, instead of > __aligned(8), please use "__aligned(_Alignof(type))". > > As for "*" or not, I would tend to agree that always requiring "*" when > using the macro seems redundant. > > Initially I was struggling to make __counted_by work, but it seems we can > use an initializer for that member, as long as we don't touch the flexible > array member in the initializer. So we just need to add the counted-by > member to the macro, and use a union to do the initialization. And if > we take the address of the union (and not the struct within it), the > compiler will see the correct object size with __builtin_object_size: > > #define DEFINE_FLEX(type, name, flex, counter, count) \ > union { \ > u8 bytes[struct_size_t(type, flex, count)]; \ > type obj; \ > } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \ > /* take address of whole union to get the correct __builtin_object_size */ \ > type *name = (type *)&name##_u > > i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc) > works correctly here, but breaks (sees a zero-sized flex array member) > if this macro ends with: > > type *name = &name##_u.obj > > > -Kees I like the union usage here, it's a bit cleaner too. For the counter param [for my, perhaps other] usages it does not fit well (as of now) :/. I will post a v1 series to move this forward though :) > > [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb > [2] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20230731150152.514984-1-przemyslaw.kitszel@intel.com/ in that [2], I have this: - cmd->num_entries = cpu_to_le16(num_entries); + cmd->num_entries = cpu_to_le16(1); as "num_entries" is not a flex array counter :/ [3] https://lore.kernel.org/netdev/a8466f4b-f773-4d0a-f22b-34c83a7aa942@intel.com/T/ -Przemek
On 8/2/23 00:31, Kees Cook wrote: [...] > Initially I was struggling to make __counted_by work, but it seems we can > use an initializer for that member, as long as we don't touch the flexible > array member in the initializer. So we just need to add the counted-by > member to the macro, and use a union to do the initialization. And if > we take the address of the union (and not the struct within it), the > compiler will see the correct object size with __builtin_object_size: > > #define DEFINE_FLEX(type, name, flex, counter, count) \ > union { \ > u8 bytes[struct_size_t(type, flex, count)]; \ > type obj; \ > } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \ > /* take address of whole union to get the correct __builtin_object_size */ \ > type *name = (type *)&name##_u > > i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc) > works correctly here, but breaks (sees a zero-sized flex array member) > if this macro ends with: > > type *name = &name##_u.obj __builtin_object_size(name, 0) works fine for both versions (with and without .obj at the end) however it does not work for builds without -O2 switch, so struct_size_t() is rather a way to go :/ > > > -Kees > > [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb >
From: Przemek Kitszel <przemyslaw.kitszel@intel.com> Date: Fri, 4 Aug 2023 15:47:48 +0200 > On 8/2/23 00:31, Kees Cook wrote: > > [...] > >> Initially I was struggling to make __counted_by work, but it seems we can >> use an initializer for that member, as long as we don't touch the >> flexible >> array member in the initializer. So we just need to add the counted-by >> member to the macro, and use a union to do the initialization. And if >> we take the address of the union (and not the struct within it), the >> compiler will see the correct object size with __builtin_object_size: >> >> #define DEFINE_FLEX(type, name, flex, counter, count) \ >> union { \ >> u8 bytes[struct_size_t(type, flex, count)]; \ >> type obj; \ >> } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \ >> /* take address of whole union to get the correct >> __builtin_object_size */ \ >> type *name = (type *)&name##_u >> >> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc) >> works correctly here, but breaks (sees a zero-sized flex array member) >> if this macro ends with: >> >> type *name = &name##_u.obj > > __builtin_object_size(name, 0) works fine for both versions (with and > without .obj at the end) > > however it does not work for builds without -O2 switch, so > struct_size_t() is rather a way to go :/ You only need to care about -O2 and -Os, since only those 2 are officially supported by Kbuild. Did you mean it doesn't work on -Os as well? > >> >> >> -Kees >> >> [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb >> > Thanks, Olek
From: Przemek Kitszel <przemyslaw.kitszel@intel.com> Date: Fri, 4 Aug 2023 12:59:08 +0200 > On 8/2/23 00:31, Kees Cook wrote: >> On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote: >>> Add DECLARE_FLEX() macro for on-stack allocations of structs with >>> flexible array member. >> >> I like this idea! >> >> One terminology nit: I think this should be called "DEFINE_...", since >> it's a specific instantiation. Other macros in the kernel seem to confuse >> this a lot, though. Yay naming. > > Thanks, makes sense! > >> >>> Using underlying array for on-stack storage lets us to declare known >>> on compile-time structures without kzalloc(). >> >> Hmpf, this appears to immediately trip over any (future) use of >> __counted_by()[1] for these (since the counted-by member would be >> initialized to zero), but I think I have a solution. (See below.) >> >>> >>> Actual usage for ice driver is in next patch of the series. >>> >>> Note that "struct" kw and "*" char is moved to the caller, to both: >>> have shorter macro name, and have more natural type specification >>> in the driver code (IOW not hiding an actual type of var). >>> >>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >>> --- >>> include/linux/overflow.h | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >>> index f9b60313eaea..403b7ec120a2 100644 >>> --- a/include/linux/overflow.h >>> +++ b/include/linux/overflow.h >>> @@ -309,4 +309,18 @@ 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) >>> +/** >>> + * DECLARE_FLEX() - Declare an on-stack instance of structure with >>> trailing >>> + * flexible array. >>> + * @type: Pointer to structure type, including "struct" keyword and >>> "*" char. >>> + * @name: Name for a (pointer) variable to create. >>> + * @member: Name of the array member. >>> + * @count: Number of elements in the array; must be compile-time const. >>> + * >>> + * Declare an instance of structure *@type with trailing flexible >>> array. >>> + */ >>> +#define DECLARE_FLEX(type, name, member, count) \ >>> + u8 name##_buf[struct_size((type)NULL, member, count)] >>> __aligned(8) = {};\ >>> + type name = (type)&name##_buf >>> + >>> #endif /* __LINUX_OVERFLOW_H */ >> >> I was disappointed to discover that only global (static) initializers >> would work for a flex array member. :( >> >> i.e. this works: >> >> struct foo { >> unsigned long flags; >> unsigned char count; > > So bad that in the ice driver (perhaps others too), we have cases that > there is no counter or it has different meaning. > (potentially "complicated" meaning - ice' struct > ice_aqc_alloc_free_res_elem has "__le16 num_elems", so could not be used > verbatim, and it's not actual counter either :/ (I was fooled by such Speaking of __le16 (we already discussed it 1:1): it's not a rare case to define Endianness-sensitive structures with a flex array at the end, so for those with __{be,le}* we could be adding __counted_by() attribute only when the host Endianness matches the structure's to have at least some coverage. By "some" I mean actually a lot when it comes to LE structures, which usually is the case :) > assumption here [2]). Perhaps recent series by Olek [3] is also good > illustration of hard cases for __counted_by() > >> int array[] __counted_by(count); >> }; Thanks, Olek
On Fri, Aug 04, 2023 at 03:47:48PM +0200, Przemek Kitszel wrote: > On 8/2/23 00:31, Kees Cook wrote: > > [...] > > > Initially I was struggling to make __counted_by work, but it seems we can > > use an initializer for that member, as long as we don't touch the flexible > > array member in the initializer. So we just need to add the counted-by > > member to the macro, and use a union to do the initialization. And if > > we take the address of the union (and not the struct within it), the > > compiler will see the correct object size with __builtin_object_size: > > > > #define DEFINE_FLEX(type, name, flex, counter, count) \ > > union { \ > > u8 bytes[struct_size_t(type, flex, count)]; \ > > type obj; \ > > } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \ > > /* take address of whole union to get the correct __builtin_object_size */ \ > > type *name = (type *)&name##_u > > > > i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc) > > works correctly here, but breaks (sees a zero-sized flex array member) > > if this macro ends with: > > > > type *name = &name##_u.obj > > __builtin_object_size(name, 0) works fine for both versions (with and > without .obj at the end) Mode 0 will be unchanged, but mode 1 is what most of FORTIFY uses for keep the scope narrow. > however it does not work for builds without -O2 switch, so struct_size_t() > is rather a way to go :/ FORTIFY depends on -O2, so no worries. :)
On 8/4/23 17:44, Alexander Lobakin wrote: > From: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Date: Fri, 4 Aug 2023 15:47:48 +0200 > >> On 8/2/23 00:31, Kees Cook wrote: >> >> [...] >> >>> Initially I was struggling to make __counted_by work, but it seems we can >>> use an initializer for that member, as long as we don't touch the >>> flexible >>> array member in the initializer. So we just need to add the counted-by >>> member to the macro, and use a union to do the initialization. And if >>> we take the address of the union (and not the struct within it), the >>> compiler will see the correct object size with __builtin_object_size: >>> >>> #define DEFINE_FLEX(type, name, flex, counter, count) \ >>> union { \ >>> u8 bytes[struct_size_t(type, flex, count)]; \ >>> type obj; \ >>> } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \ >>> /* take address of whole union to get the correct >>> __builtin_object_size */ \ >>> type *name = (type *)&name##_u >>> >>> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc) >>> works correctly here, but breaks (sees a zero-sized flex array member) >>> if this macro ends with: >>> >>> type *name = &name##_u.obj >> >> __builtin_object_size(name, 0) works fine for both versions (with and >> without .obj at the end) >> >> however it does not work for builds without -O2 switch, so >> struct_size_t() is rather a way to go :/ > > You only need to care about -O2 and -Os, since only those 2 are > officially supported by Kbuild. Did you mean it doesn't work on -Os as well? Both -Os and -O2 are fine here. One thing is that perhaps a "user friendly" define for "__builtin_object_size(name, 1)" would avoid any potential for misleading "1" with any "counter" variable, will see. > >> >>> >>> >>> -Kees >>> >>> [1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb >>> >> > > Thanks, > Olek
On August 7, 2023 5:42:31 AM PDT, Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote: >On 8/4/23 17:44, Alexander Lobakin wrote: >> From: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Date: Fri, 4 Aug 2023 15:47:48 +0200 >> >>> On 8/2/23 00:31, Kees Cook wrote: >>> >>> [...] >>> >>>> Initially I was struggling to make __counted_by work, but it seems we can >>>> use an initializer for that member, as long as we don't touch the >>>> flexible >>>> array member in the initializer. So we just need to add the counted-by >>>> member to the macro, and use a union to do the initialization. And if >>>> we take the address of the union (and not the struct within it), the >>>> compiler will see the correct object size with __builtin_object_size: >>>> >>>> #define DEFINE_FLEX(type, name, flex, counter, count) \ >>>> union { \ >>>> u8 bytes[struct_size_t(type, flex, count)]; \ >>>> type obj; \ >>>> } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \ >>>> /* take address of whole union to get the correct >>>> __builtin_object_size */ \ >>>> type *name = (type *)&name##_u >>>> >>>> i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc) >>>> works correctly here, but breaks (sees a zero-sized flex array member) >>>> if this macro ends with: >>>> >>>> type *name = &name##_u.obj >>> >>> __builtin_object_size(name, 0) works fine for both versions (with and >>> without .obj at the end) >>> >>> however it does not work for builds without -O2 switch, so >>> struct_size_t() is rather a way to go :/ >> >> You only need to care about -O2 and -Os, since only those 2 are >> officially supported by Kbuild. Did you mean it doesn't work on -Os as well? > >Both -Os and -O2 are fine here. > >One thing is that perhaps a "user friendly" define for "__builtin_object_size(name, 1)" would avoid any potential for misleading "1" with any "counter" variable, will see. I meant that FORTIFY_SOURCE will work correctly in the example I gave. (__builtin_object_size() is used there.) Also note that "max size" (mode 0) is unchanged in either case, but mode 1 needs to see the outer object to get the min size correct. -Kees
diff --git a/include/linux/overflow.h b/include/linux/overflow.h index f9b60313eaea..403b7ec120a2 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -309,4 +309,18 @@ 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) +/** + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing + * flexible array. + * @type: Pointer to structure type, including "struct" keyword and "*" char. + * @name: Name for a (pointer) variable to create. + * @member: Name of the array member. + * @count: Number of elements in the array; must be compile-time const. + * + * Declare an instance of structure *@type with trailing flexible array. + */ +#define DECLARE_FLEX(type, name, member, count) \ + u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\ + type name = (type)&name##_buf + #endif /* __LINUX_OVERFLOW_H */
Add DECLARE_FLEX() macro for on-stack allocations of structs with flexible array member. Using underlying array for on-stack storage lets us to declare known on compile-time structures without kzalloc(). Actual usage for ice driver is in next patch of the series. Note that "struct" kw and "*" char is moved to the caller, to both: have shorter macro name, and have more natural type specification in the driver code (IOW not hiding an actual type of var). Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> --- include/linux/overflow.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)