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

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
To: David Laight <David.Laight@ACULAB.COM>,
	'Kees Cook' <keescook@chromium.org>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Steven Zou <steven.zou@intel.com>,
	"intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>,
	"linux-hardening@vger.kernel.org"
	<linux-hardening@vger.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH net-next v3 1/7] overflow: add DEFINE_FLEX() for on-stack allocs
Date: Fri, 18 Aug 2023 12:28:15 +0200	[thread overview]
Message-ID: <3f61b3bc-61d4-6568-9bcb-6fd50553157c@intel.com> (raw)
In-Reply-To: <e8e109712a1b42288951c958d2f503a5@AcuMS.aculab.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)
> 
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan