diff mbox series

[net-next,v5,3/9] lib: packing: add pack_fields() and unpack_fields()

Message ID 20241111-packing-pack-fields-and-ice-implementation-v5-3-80c07349e6b7@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series lib: packing: introduce and use (un)pack_fields | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers warning 4 maintainers not CCed: nicolas@fjasle.eu nathan@kernel.org linux-doc@vger.kernel.org corbet@lwn.net
netdev/build_clang fail Errors and warnings before: 4 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 8 this patch: 13
netdev/checkpatch fail CHECK: Concatenated strings should use spaces between elements CHECK: Macro argument 'pbuf' may be better as '(pbuf)' to avoid precedence issues CHECK: Prefer kernel type 'u16' over 'uint16_t' CHECK: Prefer kernel type 'u64' over 'uint64_t' CHECK: Prefer kernel type 'u8' over 'uint8_t' ERROR: Macros with multiple statements should be enclosed in a do - while loop WARNING: line length of 101 exceeds 80 columns WARNING: line length of 109 exceeds 80 columns WARNING: line length of 115 exceeds 80 columns WARNING: line length of 140 exceeds 80 columns WARNING: line length of 161 exceeds 80 columns WARNING: line length of 162 exceeds 80 columns WARNING: line length of 190 exceeds 80 columns WARNING: line length of 81 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: macros should not use a trailing semicolon
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 27 this patch: 27
netdev/source_inline success Was 0 now: 0

Commit Message

Jacob Keller Nov. 11, 2024, 8:08 a.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.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 allows
  writing C code to validate the data. Enable this by marking the packed
  field arrays in a special data section via use of __section() as part of
  DECLARE_PACKED_FIELD_S and DECLARE_PACKED_FIELD_M macros.

  While modpost can easily check ordering of the packed fields and ensure
  no fields overlap, it is tricky to check that the fields all fit within
  the intended buffer. This is instead handled by BUILD_BUG_ON in the
  pack_fields and unpack_fields() macros. To enable this, the macros
  require that the buffers be represented by a type which has a size. Users
  of the API will need to ensure their buffers are represented by pointers
  to a type of the appropriate size, such as with a struct typedef.

  The use of modpost keeps the logic for size checking contained to one
  place, without requiring drivers to invoke a separate macro like the
  previous C pre-processor solution. It also avoids the need for thousands
  of generated lines of macro.

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

Co-developed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/packing.h            |  43 ++++++++
 include/linux/packing_types.h      |  48 +++++++++
 scripts/mod/modpost.h              |   5 +
 lib/packing.c                      | 145 +++++++++++++++++++++++++++
 lib/packing_test.c                 |  61 ++++++++++++
 scripts/mod/modpost.c              |   3 +-
 scripts/mod/packed_fields.c        | 199 +++++++++++++++++++++++++++++++++++++
 Documentation/core-api/packing.rst |  59 +++++++++++
 MAINTAINERS                        |   2 +
 scripts/mod/Makefile               |   4 +-
 10 files changed, 566 insertions(+), 3 deletions(-)

Comments

kernel test robot Nov. 11, 2024, 10:25 a.m. UTC | #1
Hi Jacob,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 774ca6d3bf24287ff60b7d6dd4171ebb6e47760a]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacob-Keller/lib-packing-create-__pack-and-__unpack-variants-without-error-checking/20241111-161131
base:   774ca6d3bf24287ff60b7d6dd4171ebb6e47760a
patch link:    https://lore.kernel.org/r/20241111-packing-pack-fields-and-ice-implementation-v5-3-80c07349e6b7%40intel.com
patch subject: [PATCH net-next v5 3/9] lib: packing: add pack_fields() and unpack_fields()
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20241111/202411111823.Y2NHM1AE-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241111/202411111823.Y2NHM1AE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411111823.Y2NHM1AE-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from scripts/mod/packed_fields.c:15:
   scripts/mod/packed_fields.c: In function 'handle_packed_field_symbol':
>> scripts/mod/packed_fields.c:134:23: warning: format '%u' expects argument of type 'unsigned int', but argument 5 has type 'Elf64_Xword' {aka 'long unsigned int'} [-Wformat=]
     134 |                 error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     135 |                       mod->name, symname, sym->st_size, field_size);
         |                                           ~~~~~~~~~~~~
         |                                              |
         |                                              Elf64_Xword {aka long unsigned int}
   scripts/mod/modpost.h:207:51: note: in definition of macro 'error'
     207 | #define error(fmt, args...)     modpost_log(true, fmt, ##args)
         |                                                   ^~~
   scripts/mod/packed_fields.c:134:49: note: format string is defined here
     134 |                 error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
         |                                                ~^
         |                                                 |
         |                                                 unsigned int
         |                                                %lu
--
   In file included from scripts/mod/packed_fields.c:15:
   scripts/mod/packed_fields.c: In function 'handle_packed_field_symbol':
>> scripts/mod/packed_fields.c:134:23: warning: format '%u' expects argument of type 'unsigned int', but argument 5 has type 'Elf64_Xword' {aka 'long unsigned int'} [-Wformat=]
     134 |                 error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     135 |                       mod->name, symname, sym->st_size, field_size);
         |                                           ~~~~~~~~~~~~
         |                                              |
         |                                              Elf64_Xword {aka long unsigned int}
   scripts/mod/modpost.h:207:51: note: in definition of macro 'error'
     207 | #define error(fmt, args...)     modpost_log(true, fmt, ##args)
         |                                                   ^~~
   scripts/mod/packed_fields.c:134:49: note: format string is defined here
     134 |                 error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
         |                                                ~^
         |                                                 |
         |                                                 unsigned int
         |                                                %lu


vim +134 scripts/mod/packed_fields.c

    99	
   100	void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
   101					Elf_Sym *sym, const char *symname)
   102	{
   103		unsigned int secindex = get_secindex(info, sym);
   104		struct packed_field_elem elem = {}, prev = {};
   105		enum element_order order = FIRST_ELEMENT;
   106		enum field_type type = UNKNOWN_SECTION;
   107		size_t field_size, count;
   108		const void *data, *ptr;
   109		const char *section;
   110	
   111		/* Skip symbols without a name */
   112		if (*symname == '\0')
   113			return;
   114	
   115		/* Skip symbols with invalid sections */
   116		if (secindex >= info->num_sections)
   117			return;
   118	
   119		section = sec_name(info, secindex);
   120	
   121		if (strcmp(section, ".rodata.packed_fields_s") == 0)
   122			type = PACKED_FIELD_S;
   123		else if (strcmp(section, ".rodata.packed_fields_m") == 0)
   124			type = PACKED_FIELD_M;
   125	
   126		/* Other sections don't relate to packed fields */
   127		if (type == UNKNOWN_SECTION)
   128			return;
   129	
   130		field_size = field_type_to_size(type);
   131	
   132		/* check that the data is a multiple of the size */
   133		if (sym->st_size % field_size != 0) {
 > 134			error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
kernel test robot Nov. 11, 2024, 10:36 a.m. UTC | #2
Hi Jacob,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 774ca6d3bf24287ff60b7d6dd4171ebb6e47760a]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacob-Keller/lib-packing-create-__pack-and-__unpack-variants-without-error-checking/20241111-161131
base:   774ca6d3bf24287ff60b7d6dd4171ebb6e47760a
patch link:    https://lore.kernel.org/r/20241111-packing-pack-fields-and-ice-implementation-v5-3-80c07349e6b7%40intel.com
patch subject: [PATCH net-next v5 3/9] lib: packing: add pack_fields() and unpack_fields()
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20241111/202411111817.tgnRn8v3-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241111/202411111817.tgnRn8v3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411111817.tgnRn8v3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> scripts/mod/packed_fields.c:135:29: warning: format specifies type 'unsigned int' but the argument has type 'Elf64_Xword' (aka 'unsigned long') [-Wformat]
     134 |                 error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
         |                                                ~~
         |                                                %lu
     135 |                       mod->name, symname, sym->st_size, field_size);
         |                                           ^~~~~~~~~~~~
   scripts/mod/modpost.h:207:54: note: expanded from macro 'error'
     207 | #define error(fmt, args...)     modpost_log(true, fmt, ##args)
         |                                                   ~~~    ^~~~
   1 warning generated.
--
>> scripts/mod/packed_fields.c:135:29: warning: format specifies type 'unsigned int' but the argument has type 'Elf64_Xword' (aka 'unsigned long') [-Wformat]
     134 |                 error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
         |                                                ~~
         |                                                %lu
     135 |                       mod->name, symname, sym->st_size, field_size);
         |                                           ^~~~~~~~~~~~
   scripts/mod/modpost.h:207:54: note: expanded from macro 'error'
     207 | #define error(fmt, args...)     modpost_log(true, fmt, ##args)
         |                                                   ~~~    ^~~~
   1 warning generated.
   In file included from arch/x86/kernel/asm-offsets.c:14:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   4 warnings generated.


vim +135 scripts/mod/packed_fields.c

    99	
   100	void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
   101					Elf_Sym *sym, const char *symname)
   102	{
   103		unsigned int secindex = get_secindex(info, sym);
   104		struct packed_field_elem elem = {}, prev = {};
   105		enum element_order order = FIRST_ELEMENT;
   106		enum field_type type = UNKNOWN_SECTION;
   107		size_t field_size, count;
   108		const void *data, *ptr;
   109		const char *section;
   110	
   111		/* Skip symbols without a name */
   112		if (*symname == '\0')
   113			return;
   114	
   115		/* Skip symbols with invalid sections */
   116		if (secindex >= info->num_sections)
   117			return;
   118	
   119		section = sec_name(info, secindex);
   120	
   121		if (strcmp(section, ".rodata.packed_fields_s") == 0)
   122			type = PACKED_FIELD_S;
   123		else if (strcmp(section, ".rodata.packed_fields_m") == 0)
   124			type = PACKED_FIELD_M;
   125	
   126		/* Other sections don't relate to packed fields */
   127		if (type == UNKNOWN_SECTION)
   128			return;
   129	
   130		field_size = field_type_to_size(type);
   131	
   132		/* check that the data is a multiple of the size */
   133		if (sym->st_size % field_size != 0) {
   134			error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
 > 135			      mod->name, symname, sym->st_size, field_size);
Masahiro Yamada Nov. 13, 2024, 8:32 p.m. UTC | #3
On Mon, Nov 11, 2024 at 5:08 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.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
Jacob Keller Nov. 13, 2024, 9:04 p.m. UTC | #4
On 11/13/2024 12:32 PM, Masahiro Yamada wrote:
> On Mon, Nov 11, 2024 at 5:08 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>> From: Vladimir Oltean <vladimir.oltean@nxp.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.

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.
Masahiro Yamada Nov. 15, 2024, 8:48 p.m. UTC | #5
On Thu, Nov 14, 2024 at 6:04 AM Jacob Keller <jacob.e.keller@intel.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@intel.com> wrote:
> >>
> >> From: Vladimir Oltean <vladimir.oltean@nxp.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
Jacob Keller Nov. 15, 2024, 11:44 p.m. UTC | #6
On 11/15/2024 12:48 PM, Masahiro Yamada wrote:
> On Thu, Nov 14, 2024 at 6:04 AM Jacob Keller <jacob.e.keller@intel.com> wrote:
>> 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
> 

This definitely seems compiler specific.

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

Yea, I agree modpost is pretty ugly, and I'm happy to drop it.

I think I've managed to get something that works in GCC and Clang with
~3k lines of macro vs the original 20K lines we had for sizes up to 50.

The version I have now works, but does require the 3K lines of macro to
effectively unwind the loops.

Its also slightly brittle because some slight alterations of the checks
no longer get detected by GCC as compile-time constants :(

I am not a huge fan of only testing on CI, since not every developer
will have things go through CI, so we end up with late reports.

I'm going to post the next version which uses the macros again, but
manages to limit things so that the calls are all done from within
<linux/packing.h> without driver intervention, and seem to work reliably
on my test systems.

> 
> 
> 
> --
> Best Regards
> Masahiro Yamada
diff mbox series

Patch

diff --git a/include/linux/packing.h b/include/linux/packing.h
index 5d36dcd06f60..18e59102ef47 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -7,6 +7,7 @@ 
 
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/packing_types.h>
 
 #define QUIRK_MSB_ON_THE_RIGHT	BIT(0)
 #define QUIRK_LITTLE_ENDIAN	BIT(1)
@@ -26,4 +27,46 @@  int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
 int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
 	   size_t pbuflen, u8 quirks);
 
+void pack_fields_s(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_s *fields, size_t num_fields,
+		   u8 quirks);
+
+void pack_fields_m(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_m *fields, size_t num_fields,
+		   u8 quirks);
+
+void unpack_fields_s(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_s *fields, size_t num_fields,
+		     u8 quirks);
+
+void unpack_fields_m(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_m *fields, size_t num_fields,
+		     u8 quirks);
+
+#define pack_fields(pbuf, ustruct, fields, quirks) \
+	({ \
+		typeof(fields[0]) *__f = fields; \
+		size_t pbuflen = sizeof(*pbuf); \
+		size_t num_fields = ARRAY_SIZE(fields); \
+		BUILD_BUG_ON(__f[0].startbit >= BITS_PER_BYTE * pbuflen); \
+		BUILD_BUG_ON(__f[num_fields - 1].startbit >= BITS_PER_BYTE * pbuflen); \
+		_Generic((fields), \
+			 const struct packed_field_s * : pack_fields_s, \
+			 const struct packed_field_m * : pack_fields_m \
+			)(pbuf, pbuflen, ustruct, __f, num_fields, quirks); \
+	})
+
+#define unpack_fields(pbuf, ustruct, fields, quirks) \
+	({ \
+		typeof(fields[0]) *__f = fields; \
+		size_t pbuflen = sizeof(*pbuf); \
+		size_t num_fields = ARRAY_SIZE(fields); \
+		BUILD_BUG_ON(__f[0].startbit >= BITS_PER_BYTE * pbuflen); \
+		BUILD_BUG_ON(__f[num_fields - 1].startbit >= BITS_PER_BYTE * pbuflen); \
+		_Generic((fields), \
+			 const struct packed_field_s * : unpack_fields_s, \
+			 const struct packed_field_m * : unpack_fields_m \
+			)(pbuf, pbuflen, ustruct, __f, num_fields, quirks); \
+	})
+
 #endif
diff --git a/include/linux/packing_types.h b/include/linux/packing_types.h
new file mode 100644
index 000000000000..e0fa5d38ca48
--- /dev/null
+++ b/include/linux/packing_types.h
@@ -0,0 +1,48 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2024, Intel Corporation
+ * Copyright (c) 2024, Vladimir Oltean <olteanv@gmail.com>
+ */
+#ifndef _LINUX_PACKING_TYPES_H
+#define _LINUX_PACKING_TYPES_H
+
+#include <linux/types.h>
+
+/* If you add another packed field type, please update
+ * scripts/mod/packed_fields.c to enable compile time sanity checks.
+ */
+
+#define GEN_PACKED_FIELD_MEMBERS(__type) \
+	__type startbit; \
+	__type endbit; \
+	__type offset; \
+	__type size;
+
+/* Small packed field. Use with bit offsets < 256, buffers < 32B and
+ * unpacked structures < 256B.
+ */
+struct packed_field_s {
+	GEN_PACKED_FIELD_MEMBERS(u8);
+};
+
+#define DECLARE_PACKED_FIELDS_S(name) \
+	const struct packed_field_s name[] __section(".rodata.packed_fields_s")
+
+/* Medium packed field. Use with bit offsets < 65536, buffers < 8KB and
+ * unpacked structures < 64KB.
+ */
+struct packed_field_m {
+	GEN_PACKED_FIELD_MEMBERS(u16);
+};
+
+#define DECLARE_PACKED_FIELDS_M(name) \
+	const struct packed_field_m name[] __section(".rodata.packed_fields_m")
+
+#define PACKED_FIELD(start, end, struct_name, struct_field) \
+{ \
+	(start), \
+	(end), \
+	offsetof(struct_name, struct_field), \
+	sizeof_field(struct_name, struct_field), \
+}
+
+#endif
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index ada3a36cc4bc..01e678626531 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -175,12 +175,17 @@  void add_moddevtable(struct buffer *buf, struct module *mod);
 /* sumversion.c */
 void get_src_version(const char *modname, char sum[], unsigned sumlen);
 
+/* packed_fields.c */
+void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
+				Elf_Sym *sym, const char *symname);
+
 /* from modpost.c */
 extern bool target_is_big_endian;
 extern bool host_is_big_endian;
 char *read_text_file(const char *filename);
 char *get_line(char **stringp);
 void *sym_get_data(const struct elf_info *info, const Elf_Sym *sym);
+const char *sec_name(const struct elf_info *info, unsigned int secindex);
 
 void __attribute__((format(printf, 2, 3)))
 modpost_log(bool is_error, const char *fmt, ...);
diff --git a/lib/packing.c b/lib/packing.c
index 09a2d195b943..45164f73fe5b 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -5,10 +5,37 @@ 
 #include <linux/packing.h>
 #include <linux/module.h>
 #include <linux/bitops.h>
+#include <linux/bits.h>
 #include <linux/errno.h>
 #include <linux/types.h>
 #include <linux/bitrev.h>
 
+#define __pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks)	\
+	({									\
+		for (size_t i = 0; i < (num_fields); i++) {			\
+			typeof(&(fields)[0]) field = &(fields)[i];		\
+			u64 uval;						\
+										\
+			uval = ustruct_field_to_u64(ustruct, field->offset, field->size); \
+										\
+			__pack(pbuf, uval, field->startbit, field->endbit,	\
+			       pbuflen, quirks);				\
+		}								\
+	})
+
+#define __unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks)	\
+	({									\
+		for (size_t i = 0; i < (num_fields); i++) {			\
+			typeof(&(fields)[0]) field = &fields[i];		\
+			u64 uval;						\
+										\
+			__unpack(pbuf, &uval, field->startbit, field->endbit,	\
+				 pbuflen, quirks);				\
+										\
+			u64_to_ustruct_field(ustruct, field->offset, field->size, uval); \
+		}								\
+	})
+
 /**
  * calculate_box_addr - Determine physical location of byte in buffer
  * @box: Index of byte within buffer seen as a logical big-endian big number
@@ -322,4 +349,122 @@  int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
 }
 EXPORT_SYMBOL(packing);
 
+static u64 ustruct_field_to_u64(const void *ustruct, size_t field_offset,
+				size_t field_size)
+{
+	switch (field_size) {
+	case 1:
+		return *((u8 *)(ustruct + field_offset));
+	case 2:
+		return *((u16 *)(ustruct + field_offset));
+	case 4:
+		return *((u32 *)(ustruct + field_offset));
+	default:
+		return *((u64 *)(ustruct + field_offset));
+	}
+}
+
+static void u64_to_ustruct_field(void *ustruct, size_t field_offset,
+				 size_t field_size, u64 uval)
+{
+	switch (field_size) {
+	case 1:
+		*((u8 *)(ustruct + field_offset)) = uval;
+		break;
+	case 2:
+		*((u16 *)(ustruct + field_offset)) = uval;
+		break;
+	case 4:
+		*((u32 *)(ustruct + field_offset)) = uval;
+		break;
+	default:
+		*((u64 *)(ustruct + field_offset)) = uval;
+		break;
+	}
+}
+
+/**
+ * pack_fields_s - Pack array of small fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of small packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void pack_fields_s(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_s *fields, size_t num_fields,
+		   u8 quirks)
+{
+	__pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(pack_fields_s);
+
+/**
+ * pack_fields_m - Pack array of medium fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of medium packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void pack_fields_m(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_m *fields, size_t num_fields,
+		   u8 quirks)
+{
+	__pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(pack_fields_m);
+
+/**
+ * unpack_fields_s - Unpack array of small fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of small packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void unpack_fields_s(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_s *fields, size_t num_fields,
+		     u8 quirks)
+{
+	__unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(unpack_fields_s);
+
+/**
+ * unpack_fields_m - Unpack array of medium fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of medium packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void unpack_fields_m(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_m *fields, size_t num_fields,
+		     u8 quirks)
+{
+	__unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(unpack_fields_m);
+
 MODULE_DESCRIPTION("Generic bitfield packing and unpacking");
diff --git a/lib/packing_test.c b/lib/packing_test.c
index b38ea43c03fd..d8693e797d9b 100644
--- a/lib/packing_test.c
+++ b/lib/packing_test.c
@@ -396,9 +396,70 @@  static void packing_test_unpack(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, uval, params->uval);
 }
 
+#define PACKED_BUF_SIZE 8
+
+typedef struct __packed { u8 buf[PACKED_BUF_SIZE]; } packed_buf_t;
+
+struct test_data {
+	u32 field3;
+	u16 field2;
+	u16 field4;
+	u16 field6;
+	u8 field1;
+	u8 field5;
+};
+
+static DECLARE_PACKED_FIELDS_S(test_fields) = {
+	PACKED_FIELD(63, 61, struct test_data, field1),
+	PACKED_FIELD(60, 52, struct test_data, field2),
+	PACKED_FIELD(51, 28, struct test_data, field3),
+	PACKED_FIELD(27, 14, struct test_data, field4),
+	PACKED_FIELD(13, 9, struct test_data, field5),
+	PACKED_FIELD(8, 0, struct test_data, field6),
+};
+
+static void packing_test_pack_fields(struct kunit *test)
+{
+	const struct test_data data = {
+		.field1 = 0x2,
+		.field2 = 0x100,
+		.field3 = 0xF00050,
+		.field4 = 0x7D3,
+		.field5 = 0x9,
+		.field6 = 0x10B,
+	};
+	packed_buf_t expect = {
+		.buf = { 0x50, 0x0F, 0x00, 0x05, 0x01, 0xF4, 0xD3, 0x0B },
+	};
+	packed_buf_t buf = {};
+
+	pack_fields(&buf, &data, test_fields, 0);
+
+	KUNIT_EXPECT_MEMEQ(test, &expect, &buf, sizeof(buf));
+}
+
+static void packing_test_unpack_fields(struct kunit *test)
+{
+	const packed_buf_t buf = {
+		.buf = { 0x17, 0x28, 0x10, 0x19, 0x3D, 0xA9, 0x07, 0x9C },
+	};
+	struct test_data data = {};
+
+	unpack_fields(&buf, &data, test_fields, 0);
+
+	KUNIT_EXPECT_EQ(test, 0, data.field1);
+	KUNIT_EXPECT_EQ(test, 0x172, data.field2);
+	KUNIT_EXPECT_EQ(test, 0x810193, data.field3);
+	KUNIT_EXPECT_EQ(test, 0x36A4, data.field4);
+	KUNIT_EXPECT_EQ(test, 0x3, data.field5);
+	KUNIT_EXPECT_EQ(test, 0x19C, data.field6);
+}
+
 static struct kunit_case packing_test_cases[] = {
 	KUNIT_CASE_PARAM(packing_test_pack, packing_gen_params),
 	KUNIT_CASE_PARAM(packing_test_unpack, packing_gen_params),
+	KUNIT_CASE(packing_test_pack_fields),
+	KUNIT_CASE(packing_test_unpack_fields),
 	{},
 };
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 107393a8c48a..5425a2afc2d8 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -327,7 +327,7 @@  static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
 				      sechdr->sh_name);
 }
 
-static const char *sec_name(const struct elf_info *info, unsigned int secindex)
+const char *sec_name(const struct elf_info *info, unsigned int secindex)
 {
 	/*
 	 * If sym->st_shndx is a special section index, there is no
@@ -1602,6 +1602,7 @@  static void read_symbols(const char *modname)
 
 		handle_symbol(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
+		handle_packed_field_symbol(mod, &info, sym, symname);
 	}
 
 	check_sec_ref(mod, &info);
diff --git a/scripts/mod/packed_fields.c b/scripts/mod/packed_fields.c
new file mode 100644
index 000000000000..474c8a8122e2
--- /dev/null
+++ b/scripts/mod/packed_fields.c
@@ -0,0 +1,199 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024, Intel Corporation. */
+
+/* Code to validate struct packed_field_[sm] data, and perform sanity checks
+ * to ensure the packed field data is laid out correctly and fits into the
+ * relevant buffer size.
+ */
+
+#include <fnmatch.h>
+#include <hashtable.h>
+#include <inttypes.h>
+#include <stdint.h>
+#include <xalloc.h>
+
+#include "modpost.h"
+
+typedef uint16_t	u16;
+typedef uint8_t		u8;
+
+#define BITS_PER_BYTE	8
+
+/* Big exception to the "don't include kernel headers into userspace", which
+ * even potentially has different endianness and word sizes, since we handle
+ * those differences explicitly below
+ */
+#include "../../include/linux/packing_types.h"
+
+#define max(a, b) ({\
+		typeof(a) _a = a;\
+		typeof(b) _b = b;\
+		_a > _b ? _a : _b; })
+
+#define min(a, b) ({\
+		typeof(a) _a = a;\
+		typeof(b) _b = b;\
+		_a < _b ? _a : _b; })
+
+struct packed_field_elem {
+	uint64_t startbit;
+	uint64_t endbit;
+	uint64_t offset;
+	uint64_t size;
+};
+
+enum field_type {
+	UNKNOWN_SECTION,
+	PACKED_FIELD_S,
+	PACKED_FIELD_M,
+};
+
+enum element_order {
+	FIRST_ELEMENT,
+	SECOND_ELEMENT,
+	ASCENDING_ORDER,
+	DESCENDING_ORDER,
+};
+
+static size_t field_type_to_size(enum field_type type)
+{
+	switch (type) {
+	case PACKED_FIELD_S:
+		return sizeof(struct packed_field_s);
+	case PACKED_FIELD_M:
+		return sizeof(struct packed_field_m);
+	default:
+		error("attempted to get field size for unknown packed field type %u\n",
+		      type);
+		return 0;
+	}
+}
+
+static void get_field_contents(const void *data, enum field_type type,
+			       struct packed_field_elem *elem)
+{
+	switch (type) {
+	case PACKED_FIELD_S: {
+		const struct packed_field_s *data_field = data;
+
+		elem->startbit = TO_NATIVE(data_field->startbit);
+		elem->endbit = TO_NATIVE(data_field->endbit);
+		elem->offset = TO_NATIVE(data_field->offset);
+		elem->size = TO_NATIVE(data_field->size);
+		return;
+	}
+	case PACKED_FIELD_M: {
+		const struct packed_field_m *data_field = data;
+
+		elem->startbit = TO_NATIVE(data_field->startbit);
+		elem->endbit = TO_NATIVE(data_field->endbit);
+		elem->offset = TO_NATIVE(data_field->offset);
+		elem->size = TO_NATIVE(data_field->size);
+		return;
+	}
+	default:
+		error("attempted to get field contents for unknown packed field type %u\n",
+		      type);
+	}
+}
+
+void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
+				Elf_Sym *sym, const char *symname)
+{
+	unsigned int secindex = get_secindex(info, sym);
+	struct packed_field_elem elem = {}, prev = {};
+	enum element_order order = FIRST_ELEMENT;
+	enum field_type type = UNKNOWN_SECTION;
+	size_t field_size, count;
+	const void *data, *ptr;
+	const char *section;
+
+	/* Skip symbols without a name */
+	if (*symname == '\0')
+		return;
+
+	/* Skip symbols with invalid sections */
+	if (secindex >= info->num_sections)
+		return;
+
+	section = sec_name(info, secindex);
+
+	if (strcmp(section, ".rodata.packed_fields_s") == 0)
+		type = PACKED_FIELD_S;
+	else if (strcmp(section, ".rodata.packed_fields_m") == 0)
+		type = PACKED_FIELD_M;
+
+	/* Other sections don't relate to packed fields */
+	if (type == UNKNOWN_SECTION)
+		return;
+
+	field_size = field_type_to_size(type);
+
+	/* check that the data is a multiple of the size */
+	if (sym->st_size % field_size != 0) {
+		error("[%s.ko] \"%s\" has size %u which is not a multiple of the field size (%zu)\n",
+		      mod->name, symname, sym->st_size, field_size);
+		return;
+	}
+
+	data = sym_get_data(info, sym);
+
+	for (ptr = data, count = 0;
+	     ptr < data + sym->st_size;
+	     ptr += field_size, count++, prev = elem) {
+		get_field_contents(ptr, type, &elem);
+
+		if (elem.size != 1 && elem.size != 2 &&
+		    elem.size != 4 && elem.size != 8)
+			error("[%s.ko] \"%s\" field %zu unpacked size (%" PRIu64 ") must be 1, 2, 4, or 8\n",
+			      mod->name, symname, count, elem.size);
+
+		if (elem.startbit < elem.endbit)
+			error("[%s.ko] \"%s\" field %zu (%" PRIu64 "-%" PRIu64 "): start bit must be >= end bit\n",
+			      mod->name, symname, count,
+			      elem.startbit, elem.endbit);
+
+		if (elem.startbit - elem.endbit + 1 > BITS_PER_BYTE * elem.size)
+			error("[%s.ko] \"%s\" field %zu (%" PRIu64 "-%" PRIu64 ") has a width of %" PRIu64 " bits which does not fit into the unpacked structure field (%" PRIu64 " bytes)\n",
+			      mod->name, symname, count,
+			      elem.startbit, elem.endbit,
+			      elem.startbit - elem.endbit + 1,
+			      elem.size);
+
+		if (order != FIRST_ELEMENT &&
+		    max(elem.endbit, prev.endbit) <=
+		    min(elem.startbit, prev.startbit))
+			error("[%s.ko] \"%s\" field %zu (%" PRIu64 "-%" PRIu64 ") overlaps with previous field (%" PRIu64 "-%" PRIu64 ")\n",
+			      mod->name, symname, count,
+			      elem.startbit, elem.endbit,
+			      prev.startbit, prev.endbit);
+
+		switch (order) {
+		case FIRST_ELEMENT:
+			order = SECOND_ELEMENT;
+			break;
+		case SECOND_ELEMENT:
+			order = prev.startbit < elem.startbit ?
+				ASCENDING_ORDER : DESCENDING_ORDER;
+			break;
+		case ASCENDING_ORDER:
+			if (prev.startbit >= elem.startbit ||
+			    prev.endbit >= elem.endbit)
+				error("[%s.ko] \"%s\" field %zu (%" PRIu64 "-%" PRIu64") not in ascending order with previous field (%" PRIu64 "-%" PRIu64 ")\n",
+				      mod->name, symname, count,
+				      elem.startbit, elem.endbit,
+				      prev.startbit, prev.endbit);
+			break;
+		case DESCENDING_ORDER:
+			if (prev.startbit <= elem.startbit ||
+			    prev.endbit <= elem.endbit)
+				error("[%s.ko] \"%s\" field %zu (%" PRIu64 "-%" PRIu64") not in descending order with previous field (%" PRIu64 "-%" PRIu64 ")\n",
+				      mod->name, symname, count,
+				      elem.startbit, elem.endbit,
+				      prev.startbit, prev.endbit);
+			break;
+		default:
+			break;
+		}
+	}
+}
diff --git a/Documentation/core-api/packing.rst b/Documentation/core-api/packing.rst
index 821691f23c54..d4cdc287704d 100644
--- a/Documentation/core-api/packing.rst
+++ b/Documentation/core-api/packing.rst
@@ -235,3 +235,62 @@  programmer against incorrect API use.  The errors are not expected to occur
 during runtime, therefore it is reasonable for xxx_packing() to return void
 and simply swallow those errors. Optionally it can dump stack or print the
 error description.
+
+The pack_fields() and unpack_fields() macros automatically select the
+appropriate function at compile time based on the type of the fields array
+passed in.
+
+Packed Fields
+-------------
+
+Drivers are encouraged to use the ``pack_fields()`` and ``unpack_fields()``
+APIs over using ``pack()``, ``unpack()``, or ``packing()``.
+
+These APIs use field definitions in arrays of ``struct packed_field_s`` or
+``struct packed_field_m`` stored as ``.rodata``. This significantly reduces
+the code footprint required to pack or unpack many fields. In addition,
+sanity checks on the field definitions are handled at compile time via
+``modpost`` warnings, rather than only when the offending code is executed.
+
+The ``pack_fields()`` and ``unpack_fields()`` macros determine the size of
+the packed buffer by its type. Thus, you must ensure the buffer is a pointer
+to a type with the desired size. This is typically achieved by creating a
+typedef with a packed structure.
+
+Here is an example of how to use the fields APIs:
+
+.. code-block:: c
+
+   struct data {
+        u64 field3;
+        u32 field4;
+        u16 field1;
+        u8 field2;
+   };
+
+   #define SIZE 13
+
+   typdef struct __packed { u8 buf[SIZE]; } packed_buf_t;
+
+   DECLARE_PACKED_FIELDS_S(fields, SIZE) = {
+           PACKED_FIELD(100, 90, struct data, field1),
+           PACKED_FIELD(90, 87, struct data, field2),
+           PACKED_FIELD(86, 30, struct data, field3),
+           PACKED_FIELD(29, 0, struct data, field4),
+   };
+
+   void unpack_your_data(const packed_buf_t *buf, struct data *unpacked)
+   {
+           BUILD_BUG_ON(sizeof(*buf) != SIZE;
+
+           unpack_fields(buf, sizeof(*buf), unpacked, fields,
+                         QUIRK_LITTLE_ENDIAN);
+   }
+
+   void pack_your_data(const struct data *unpacked, packed_buf_t *buf)
+   {
+           BUILD_BUG_ON(sizeof(*buf) != SIZE;
+
+           pack_fields(buf, sizeof(*buf), unpacked, fields,
+                       QUIRK_LITTLE_ENDIAN);
+   }
diff --git a/MAINTAINERS b/MAINTAINERS
index a4855581d62c..daaf4ae5bd9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17445,8 +17445,10 @@  L:	netdev@vger.kernel.org
 S:	Supported
 F:	Documentation/core-api/packing.rst
 F:	include/linux/packing.h
+F:	include/linux/packing_types.h
 F:	lib/packing.c
 F:	lib/packing_test.c
+F:	scripts/mod/packed_fields.c
 
 PADATA PARALLEL EXECUTION MECHANISM
 M:	Steffen Klassert <steffen.klassert@secunet.com>
diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
index c729bc936bae..aa729b6000b6 100644
--- a/scripts/mod/Makefile
+++ b/scripts/mod/Makefile
@@ -4,7 +4,7 @@  CFLAGS_REMOVE_empty.o += $(CC_FLAGS_LTO)
 hostprogs-always-y	+= modpost mk_elfconfig
 always-y		+= empty.o
 
-modpost-objs	:= modpost.o file2alias.o sumversion.o symsearch.o
+modpost-objs	:= modpost.o file2alias.o sumversion.o symsearch.o packed_fields.o
 
 devicetable-offsets-file := devicetable-offsets.h
 
@@ -15,7 +15,7 @@  targets += $(devicetable-offsets-file) devicetable-offsets.s
 
 # dependencies on generated files need to be listed explicitly
 
-$(obj)/modpost.o $(obj)/file2alias.o $(obj)/sumversion.o $(obj)/symsearch.o: $(obj)/elfconfig.h
+$(obj)/modpost.o $(obj)/file2alias.o $(obj)/sumversion.o $(obj)/symsearch.o $(obj)/packed_fields.o: $(obj)/elfconfig.h
 $(obj)/file2alias.o: $(obj)/$(devicetable-offsets-file)
 
 quiet_cmd_elfconfig = MKELF   $@