Re: [PATCH net-next v3 1/7] overflow: add DEFINE_FLEX() for on-stack allocs

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 8/18/23 09:14, David Laight wrote:
> From: Kees Cook
>> Sent: Thursday, August 17, 2023 6:00 PM
>>
>> On Thu, Aug 17, 2023 at 02:35:23PM +0000, David Laight wrote:
>>> From: Przemek Kitszel
>>>> Sent: Wednesday, August 16, 2023 3:06 PM
> ...
>>>> +#define DEFINE_FLEX(type, name, member, count)					\
>>>> +	union {									\
>>>> +		u8 bytes[struct_size_t(type, member, count)];			\
>>>> +		type obj;							\
>>>> +	} name##_u __aligned(_Alignof(type)) = {};				\
>>>
>>> You shouldn't need the _Alignof() it is the default.
>>
>> In the sense that since "type" is in the union, it's okay?
> 
> The alignment of the union is the larger of the alignments
> of all its members.
> Which is what you want.
> 
>>> I'm not sure you should be forcing the memset() either.
>>
>> This already got discussed: better to fail safe.
> 
> Perhaps call it DEFINE_FLEX_Z() to make this clear and
> give the option for a non-zeroing version later.
> Not everyone wants the expense of zeroing everything.

per Kees, zeroing should be removed by compiler when not needed:
https://lore.kernel.org/intel-wired-lan/202308101128.C4F0FA235@keescook/

Thanks for education on alignment!

> 
> ..
>>> You might want to add:
>>> 	Static_assert(is_constexpr(count), "DEFINE_FLEX: non-constant count " #count);
>>
>> That would be nice, though can Static_assert()s live in the middle of
>> variable definitions?
> 
> I checked and it is fine.
> (I double-checked by adding a statement and getting an error.)

Static_assert with nice wording definitively would make it better, 
thanks again!

> 
> 	David >
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 
>