On Thu, Nov 14, 2024 at 6:04 AM Jacob Keller <jacob.e.keller@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> >>
> >> From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> >>
> >> 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
