netdev - Re: [PATCH net-next v9 03/10] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241205101501.ufmixuzon7a2cnam@skbuf>
Date: Thu, 5 Dec 2024 12:15:01 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Jacob Keller <jacob.e.keller@...el.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>
Subject: Re: [PATCH net-next v9 03/10] lib: packing: add pack_fields() and
 unpack_fields()

On Wed, Dec 04, 2024 at 05:22:49PM -0800, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> This is new API which caters to the following requirements:
> 
> - Pack or unpack a large number of fields to/from a buffer with a small
>   code footprint. The current alternative is to open-code a large number
>   of calls to pack() and unpack(), or to use packing() to reduce that
>   number to half. But packing() is not const-correct.
> 
> - Use unpacked numbers stored in variables smaller than u64. This
>   reduces the rodata footprint of the stored field arrays.
> 
> - Perform error checking at compile time, rather than runtime, and return
>   void from the API functions. Because the C preprocessor can't generate
>   variable length code (loops), this is a bit tricky to do with macros.
> 
>   To handle this, implement macros which sanity check the packed field
>   definitions based on their size. Finally, a single macro with a chain of
>   __builtin_choose_expr() is used to select the appropriate macros. We
>   enforce the use of ascending or descending order to avoid O(N^2) scaling
>   when checking for overlap. Note that the macros are written with care to
>   ensure that the compilers can correctly evaluate the resulting code at
>   compile time. In particular, care was taken with avoiding too many nested
>   statement expressions. Nested statement expressions trip up some
>   compilers, especially when passing down variables created in previous
>   statement expressions.
> 
>   There are two key design choices intended to keep the overall macro code
>   size small. First, the definition of each CHECK_PACKED_FIELDS_N macro is
>   implemented recursively, by calling the N-1 macro. This avoids needing
>   the code to repeat multiple times.
> 
>   Second, the CHECK_PACKED_FIELD macro enforces that the fields in the
>   array are sorted in order. This allows checking for overlap only with
>   neighboring fields, rather than the general overlap case where each field
>   would need to be checked against other fields.
> 
>   The overlap checks use the first two fields to determine the order of the
>   remaining fields, thus allowing either ascending or descending order.
>   This enables drivers the flexibility to keep the fields ordered in which
>   ever order most naturally fits their hardware design and its associated
>   documentation.
> 
>   The CHECK_PACKED_FIELDS macro is directly called from within pack_fields
>   and unpack_fields, ensuring that all drivers using the API receive the
>   benefits of the compile-time checks. Users do not need to directly call
>   any of the macros directly.
> 
>   The CHECK_PACKED_FIELDS and its helper macros CHECK_PACKED_FIELDS_(0..50)
>   are generated using a simple C program in scripts/gen_packed_field_checks.c
>   This program can be compiled on demand and executed to generate the macro
>   code in include/linux/packing.h. This will aid in the event that a driver
>   needs more than 50 fields. The generator can be updated with a new size,
>   and used to update the packing.h header file. In practice, the ice driver
>   will need to support 27 fields, and the sja1105 driver will need to
>   support 40 fields. This on-demand generation avoids the need to modify
>   Kbuild. We do not anticipate the maximum number of fields to grow very
>   often.
> 
> - Reduced rodata footprint for the storage of the packed field arrays.
>   To that end, we have struct packed_field_s (small) and packed_field_m
>   (medium). More can be added as needed (unlikely for now). On these
>   types, the same generic pack_fields() and unpack_fields() API can be
>   used, thanks to the new C11 _Generic() selection feature, which can
>   call pack_fields_s() or pack_fields_m(), depending on the type of the
>   "fields" array - a simplistic form of polymorphism. It is evaluated at
>   compile time which function will actually be called.
> 
> Over time, packing() is expected to be completely replaced either with
> pack() or with pack_fields().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> Co-developed-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> ---

Tested-by: Vladimir Oltean <vladimir.oltean@....com> # KUnit