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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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