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

Message-ID: <CAK7LNARhMDEMZFjC1tU5oHefGocxwBC5=Vyy9Q=bx3VvQyssVQ@mail.gmail.com>
Date: Sat, 16 Nov 2024 05:48:16 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Vladimir Oltean <olteanv@...il.com>, 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>, 
	netdev <netdev@...r.kernel.org>, linux-kbuild@...r.kernel.org, 
	Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next v5 3/9] lib: packing: add pack_fields() and unpack_fields()

On Thu, Nov 14, 2024 at 6:04 AM Jacob Keller <jacob.e.keller@...el.com> wrote:
>
>
>
> On 11/13/2024 12:32 PM, Masahiro Yamada wrote:
> > On Mon, Nov 11, 2024 at 5:08 PM Jacob Keller <jacob.e.keller@...el.com> 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 generat
> >>   variable length code (loops), we can't easily use macros to implement the
> >>   overlap checks at compile time.
> >>
> >>   Instead, check for field ordering and overlap in modpost.
> >
> > This is over-engineering.
> >
> > modpost should not be bothered just for a small library like this.
> >
> > Please do sanity checks within lib/packing.c
> >
>
> With the goal of maintaining compile time checks, we end up either
> needing to use generated macros which are O(N^2) if we allow arbitrary
> overlap. If we instead allow only only ascending or descending order,
> this would drop to O(N) which would avoid needing to have 20k lines of
> generated code for the case with 50. I think we could implement them
> without forcing drivers to specifically call the correct macro by using
> something like __builtin_choose_expr(), tho implementing that macro to
> select could be quite long.


WIth Clang, the following check seems to work,
but with GCC, it works only when the array size is small.


#define PACKED_FIELDS_OUT_OF_ORDER(fields) \
({ \
        bool res = false; \
        for (unsigned int i = 1; i < ARRAY_SIZE(fields); i++) \
                res |= fields[i - 1].startbit < fields[i].startbit; \
        res; \
})

#define PACKED_FIELDS_OVERWRAP(fields) \
({ \
        bool res = false; \
        for (unsigned int i = 1; i < ARRAY_SIZE(fields); i++) \
                res |= fields[i - 1].endbit <= fields[i].startbit; \
        res; \
})

/*
 * Clang cleverly computes this at compile time.
 * Unfortunately, GCC gives it up when the array size becomes large.
 * Turn on this check only when building the kernel with Clang.
 */
#ifdef CONFIG_CC_IS_CLANG
#define PACKED_FIELDS_SANITY_CHECKS(fields) \
        BUILD_BUG_ON_MSG(PACKED_FIELDS_OUT_OF_ORDER(fields), \
                         #fields ": not sorted decending order"); \
        BUILD_BUG_ON_MSG(PACKED_FIELDS_OVERWRAP(fields), \
                         #fields ": contains overwrap")
#else
#define PACKED_FIELDS_SANITY_CHECKS(fields)
#endif





> Otherwise we can fall back to either module load time checks, or go all
> the way back to only sanity checking at executing of pack_fields or
> unpack_fields.

Is it a big deal?
One solution is a run-time check (for GCC), which is a one-time
for booting or module loading.

Another is to rely on CICD running with Clang to detect overwraps.


It is horrible to include kernel-space structures from user-space
programs that run in a different architecture.

file2alias.c does this because it is only possible at compile-time,
but it is always the source of troubles.
I am search for a way to generate MODULE_ALIAS() without
including mod_devicetable.h from modpost.




--
Best Regards
Masahiro Yamada