netdev - Re: [RFC net-next 1/2] overflow: add DECLARE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7a83e627-6e03-c447-77f8-1020973fa4a9@intel.com>
Date: Tue, 1 Aug 2023 19:15:16 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: Kees Cook <keescook@...omium.org>, Jacob Keller
	<jacob.e.keller@...el.com>, <intel-wired-lan@...ts.osuosl.org>,
	<netdev@...r.kernel.org>
Subject: Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack
 allocs

From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Date: Tue, 1 Aug 2023 16:18:44 +0200

> On 8/1/23 15:54, Alexander Lobakin wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@...el.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@...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) = {};\
>>
>> 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