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: <47815c47-b8dc-6d37-b869-0fba22e3a71b@intel.com>
Date: Tue, 1 Aug 2023 16:18:44 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...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

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 
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.

I have no strong opinion here, so will just switch to pure-type param.

> Thanks,
> Olek