netdev - Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack

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] [day] [month] [year] [list]

Message-ID: <1b5543ee-c460-4b3d-8a93-a51c9a5d82f1@intel.com>
Date: Fri, 8 Nov 2024 14:53:11 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: Andrew Morton <akpm@...ux-foundation.org>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Przemek
 Kitszel" <przemyslaw.kitszel@...el.com>, Masahiro Yamada
	<masahiroy@...nel.org>, netdev <netdev@...r.kernel.org>,
	<linux-kbuild@...r.kernel.org>, Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and
 unpack_fields()



On 11/8/2024 10:31 AM, Vladimir Oltean wrote:
> On Fri, Nov 08, 2024 at 01:24:07PM +0200, Vladimir Oltean wrote:
>> On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
>>> +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
>>> +	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
>>> +	const struct packed_field_s name[] __pf_section_s
>>
>> This will need sorting out - how to make this declaration macro
>> compatible with the "static" keyword. The obvious way (to group the
>> field array and the buffer size into a structure) doesn't work. It loses
>> the ARRAY_SIZE() of the fields, which is important to the pack_fields()
>> and unpack_fields() wrappers.
>>
>> Maybe a different tool is needed for checking that no packed fields
>> exceed the buffer size? Forcing the buffer size be a symbol just because
>> that's what modpost works with seems unnatural.
>>
>> If we knew the position of the largest field array element in C, and if
>> we enforced that all pack_fields() use a strong type (size included) for
>> the packed buffer, we'd have all information inside the pack_fields()
>> macro, because we only need to compare the largest field against the
>> buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
>> inside the pack_fields() macro itself. And have the compile-time checks
>> spill over between C and modpost...
>>
>> Not to mention, pack_fields() would have one argument less: pbuflen.
> 
> I was thinking something like this (attached). I still don't like
> modpost more than the assertions in C code, because it imposes more
> constraints upon the library user which didn't exist before. Though
> without the extra restrictions added in this patch (just ascending order
> for packed fields + strong type for packed buffer), I don't think the
> modpost variant is going to work, or is going to become extremely complex.

To me, the restrictions seem acceptable. You can always wrap an
arbitrary void buffer into a sized type via a structure.

I do agree that modpost isn't perfect with respect to the C code, but I
dislike the sheer size of the macros generated and the complexity added
to the build system.

My preferred solution is unfortunately not available in C: having the
compiler evaluate a constant function. There is a C++ extension that
does that, but it doesn't seem like GCC has enabled such support in the
C even as an extension.

I'd prefer to keep allowing both ascending and descending order, because
I think those are both valid orderings depending on how you're thinking
about the fields (little vs big endian).