Message-ID: <d67257c3-6f3d-7b69-9689-6437f91a5858@intel.com>
Date: Fri, 4 Aug 2023 12:59:08 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Kees Cook <keescook@...omium.org>
CC: Jacob Keller <jacob.e.keller@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, "Alexander
 Lobakin" <aleksander.lobakin@...el.com>
Subject: Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack
 allocs

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@...el.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 *@...e 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

Powered by blists - more mailing lists