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

[Vladimir Oltean <olteanv@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]

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.

[-- Attachment #2: 0001-pack_fields-changes.patch --]
[-- Type: text/x-diff, Size: 13961 bytes --]

From d9bf88a24518afa96fe55beda040a3605b81610a Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 8 Nov 2024 17:07:16 +0200
Subject: [PATCH] pack_fields() changes

Remove ability to declare packed field array in ascending and descending
order, over just ascending.

Actually implement the overlap check in modpost. The simple check:
"elem.endbit <= previous_elem.startbit" enforces both ascending order
and non-overlapping checks.

Move the check that fields don't exceed the buffer size inside the
pack_fields() and unpack_fields() macro. With the assumption that the
packed field array is sorted (enforced by modpost), we only need to
check this for the last element of the array.

Remove the packed buffer size from the DECLARE_PACKED_FIELDS_*() macros,
and thus from the .rodata.packed_fields_* sections. Also remove the
modpost code that looks for those sizes.

Modify the pack_fields() API to always expect a strong type as the
"pbuf" argument, because we take sizeof(*pbuf) as the packed buffer
size. Remove the pbuflen from the required argument list, and adapt ice
accordingly.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c |  16 +--
 include/linux/packing.h                     |  32 +++--
 include/linux/packing_types.h               |  15 +-
 scripts/mod/packed_fields.c                 | 148 ++++++--------------
 4 files changed, 76 insertions(+), 135 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 98845d790791..631963427082 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1381,7 +1381,7 @@ static void ice_copy_rxq_ctx_to_hw(struct ice_hw *hw,
 	PACKED_FIELD((lsb) + (width) - 1, (lsb), struct struct_name, struct_field)
 
 /* LAN Rx Queue Context */
-DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ) = {
+static const DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields) = {
 				 /* Field		Width	LSB */
 	ICE_CTX_STORE(ice_rlan_ctx, head,		13,	0),
 	ICE_CTX_STORE(ice_rlan_ctx, cpuid,		8,	13),
@@ -1416,10 +1416,8 @@ DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ) = {
 static void ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx,
 			     ice_rxq_ctx_buf_t *buf)
 {
-	BUILD_BUG_ON(sizeof(*buf) != ICE_RXQ_CTX_SZ);
-
-	pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
-		    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
+	pack_fields(buf, ctx, ice_rlan_ctx_fields, QUIRK_LITTLE_ENDIAN |
+		    QUIRK_LSW32_IS_FIRST);
 }
 
 /**
@@ -1448,7 +1446,7 @@ int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
 }
 
 /* LAN Tx Queue Context */
-DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ) = {
+static const DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields) = {
 				    /* Field			Width	LSB */
 	ICE_CTX_STORE(ice_tlan_ctx, base,			57,	0),
 	ICE_CTX_STORE(ice_tlan_ctx, port_num,			3,	57),
@@ -1489,10 +1487,8 @@ DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ) = {
  */
 void ice_pack_txq_ctx(const struct ice_tlan_ctx *ctx, ice_txq_ctx_buf_t *buf)
 {
-	BUILD_BUG_ON(sizeof(*buf) != ICE_TXQ_CTX_SZ);
-
-	pack_fields(buf, sizeof(*buf), ctx, ice_tlan_ctx_fields,
-		    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
+	pack_fields(buf, ctx, ice_tlan_ctx_fields, QUIRK_LITTLE_ENDIAN |
+		    QUIRK_LSW32_IS_FIRST);
 }
 
 /* Sideband Queue command wrappers */
diff --git a/include/linux/packing.h b/include/linux/packing.h
index b66c4809e121..d4988131e13a 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -35,16 +35,26 @@ 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, pbuflen, ustruct, fields, quirks) \
-	_Generic((fields), \
-		 const struct packed_field_s * : pack_fields_s, \
-		 const struct packed_field_m * : pack_fields_m \
-		)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)
-
-#define unpack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
-	_Generic((fields), \
-		 const struct packed_field_s * : unpack_fields_s, \
-		 const struct packed_field_m * : unpack_fields_m \
-		)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), 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[num_fields - 1].startbit >= BITS_PER_BYTE * pbuflen); \
+	   _Generic(__f, \
+		    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[num_fields - 1].startbit >= BITS_PER_BYTE * pbuflen); \
+	   _Generic(__f, \
+		    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
index 49ae4329a494..c761d93be235 100644
--- a/include/linux/packing_types.h
+++ b/include/linux/packing_types.h
@@ -24,12 +24,6 @@ struct packed_field_s {
 	GEN_PACKED_FIELD_MEMBERS(u8);
 };
 
-#define __pf_section_s __section(".rodata.packed_fields_s")
-
-#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
-
 /* Medium packed field. Use with bit offsets < 65536, buffers < 8KB and
  * unpacked structures < 64KB.
  */
@@ -37,11 +31,10 @@ struct packed_field_m {
 	GEN_PACKED_FIELD_MEMBERS(u16);
 };
 
-#define __pf_section_m __section(".rodata.packed_fields_m")
-
-#define DECLARE_PACKED_FIELDS_M(name, buffer_sz) \
-	const size_t __ ## name ## _buffer_sz __pf_section_m = buffer_sz; \
-	const struct packed_field_m name[] __pf_section_m
+#define DECLARE_PACKED_FIELDS_S(name) \
+	struct packed_field_s name[] __section(".rodata.packed_fields_s")
+#define DECLARE_PACKED_FIELDS_M(name) \
+	struct packed_field_m name[] __section(".rodata.packed_fields_m")
 
 #define PACKED_FIELD(start, end, struct_name, struct_field) \
 { \
diff --git a/scripts/mod/packed_fields.c b/scripts/mod/packed_fields.c
index aa9dbd704e52..5927b0c7f404 100644
--- a/scripts/mod/packed_fields.c
+++ b/scripts/mod/packed_fields.c
@@ -87,18 +87,18 @@ struct field_symbol {
 	size_t data_size;
 	void *data;
 	struct module *mod;
-	char *name;
+	const char *name;
 };
 
 static HASHTABLE_DEFINE(field_hashtable, 1U << 10);
 
-static struct field_symbol *alloc_field(char *name, struct module *mod)
+static struct field_symbol *alloc_field(const char *name, struct module *mod)
 {
 	struct field_symbol *f = xmalloc(sizeof(*f));
 
 	memset(f, 0, sizeof(*f));
 	f->mod = mod;
-	f->name = name;
+	f->name = xstrdup(name);
 
 	return f;
 }
@@ -124,10 +124,8 @@ void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
 {
 	unsigned int secindex = get_secindex(info, sym);
 	enum field_type type = UNKNOWN_SECTION;
-	bool is_size_symbol = false;
 	struct field_symbol *f;
 	const char *section;
-	char *name;
 
 	/* Skip symbols without a name */
 	if (*symname == '\0')
@@ -148,18 +146,9 @@ void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
 	if (type == UNKNOWN_SECTION)
 		return;
 
-	name = xstrdup(symname);
-
-	/* Extract original field name from the size symbol */
-	if (!fnmatch("__*_buffer_sz", name, 0)) {
-		name += strlen("__");
-		name[strlen(name) - strlen("_buffer_sz")] = '\0';
-		is_size_symbol = true;
-	}
-
-	f = find_field(name, mod);
+	f = find_field(symname, mod);
 	if (!f) {
-		f = alloc_field(name, mod);
+		f = alloc_field(symname, mod);
 		f->type = type;
 		hash_add_field(f);
 	}
@@ -170,118 +159,71 @@ void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
 		return;
 	}
 
-	if (is_size_symbol) {
-		size_t *size_data = sym_get_data(info, sym);
-		size_t size = TO_NATIVE(*size_data);
-
-		if (f->buffer_size && f->buffer_size != size) {
-			error("%s has buffer size %zu, but symbol %s says the size should be %zu\n",
-			      f->name, f->buffer_size, symname, size);
-		}
-
-		f->buffer_size = size;
-	} else {
-		if (f->data)
-			error("%s has multiple data symbols???\n",
-			      f->name);
-
-		f->data_size = sym->st_size;
-		f->data = xmalloc(f->data_size);
-		memcpy(f->data, sym_get_data(info, sym), f->data_size);
+	if (f->data) {
+		error("%s has multiple data symbols???\n",
+		      f->name);
 	}
-}
 
-enum element_order {
-	FIRST_ELEMENT,
-	SECOND_ELEMENT,
-	ASCENDING_ORDER,
-	DESCENDING_ORDER,
-};
+	f->data_size = sym->st_size;
+	f->data = xmalloc(f->data_size);
+	memcpy(f->data, sym_get_data(info, sym), f->data_size);
+}
 
 static void check_packed_field_array(const struct field_symbol *f)
 {
-	struct packed_field_elem previous_elem = {};
 	size_t field_size = field_type_to_size(f->type);
-	enum element_order order = FIRST_ELEMENT;
+	struct packed_field_elem previous_elem = {};
 	void *data_ptr;
-	int count;
+	size_t count;
 
 	/* check that the data is a multiple of the size */
-	if (f->data_size % field_size != 0) {
-		error("symbol %s of module %s has size %zu which is not a multiple of the field size (%zu)\n",
-		      f->name, f->mod->name, f->data_size, field_size);
+	if (f->data_size % field_size) {
+		error("[%s] symbol %s has size %zu which is not a multiple of the field size (%zu)\n",
+		      f->mod->name, f->name, f->data_size, field_size);
 		return;
 	}
 
-	data_ptr = f->data;
-	count = 0;
-
-	while (data_ptr < f->data + f->data_size) {
+	for (data_ptr = f->data, count = 0;
+	     data_ptr < f->data + f->data_size;
+	     data_ptr += field_size, count++) {
 		struct packed_field_elem elem = {};
 
 		get_field_contents(data_ptr, f->type, &elem);
 
-		if (elem.startbit < elem.endbit)
-			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") must be larger than endbit (%" PRIu64 ")\n",
-			      f->name, f->mod->name, count, elem.startbit,
-			      elem.endbit);
+		if (elem.size != 1 && elem.size != 2 &&
+		    elem.size != 4 && elem.size != 8) {
+			error("[%s] \"%s\" field %zu unpacked size (%" PRIu64
+			      ") must be 1, 2, 4, or 8 bytes\n",
+			      f->mod->name, f->name, count, elem.size);
+		}
 
-		if (elem.startbit >= BITS_PER_BYTE * f->buffer_size)
-			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") puts field outsize of the packed buffer size (%" PRIu64 ")\n",
-			      f->name, f->mod->name, count, elem.startbit,
-			      f->buffer_size);
+		if (elem.startbit < elem.endbit) {
+			error("[%s] \"%s\" field %zu (%" PRIu64 "-%" PRIu64
+			      "): start bit must be >= end bit\n",
+			      f->mod->name, f->name, count, elem.startbit,
+			      elem.endbit);
+		}
 
-		if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size)
-			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") and endbit (%" PRIu64 ") indicate a field of width (%" PRIu64 ") which does not fit into the field size (%" PRIu64 ")\n",
-			      f->name, f->mod->name, count, elem.startbit,
+		if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size) {
+			error("[%s] \"%s\" field %zu (%" PRIu64 "-%" PRIu64
+			      ") has a width of %" PRIu64
+			      " which does not fit into the unpacked structure field (%"
+			      PRIu64 " bytes)\n",
+			      f->mod->name, f->name, count, elem.startbit,
 			      elem.endbit, elem.startbit - elem.endbit,
 			      elem.size);
-
-		if (elem.size != 1 && elem.size != 2 && elem.size != 4 && elem.size != 8)
-			error("\"%s\" [%s.ko] element %u size (%" PRIu64 ") must be 1, 2, 4, or 8\n",
-			      f->name, f->mod->name, count, elem.size);
-
-		switch (order) {
-		case FIRST_ELEMENT:
-			order = SECOND_ELEMENT;
-			break;
-		case SECOND_ELEMENT:
-			order = previous_elem.startbit < elem.startbit ?
-				ASCENDING_ORDER : DESCENDING_ORDER;
-			break;
-		default:
-			break;
 		}
 
-		switch (order) {
-		case ASCENDING_ORDER:
-			if (previous_elem.startbit >= elem.startbit)
-				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element startbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count,
-				      elem.startbit, previous_elem.startbit);
-			if (previous_elem.endbit >= elem.endbit)
-				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element endbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count, elem.endbit,
-				      previous_elem.endbit);
-
-			break;
-		case DESCENDING_ORDER:
-			if (previous_elem.startbit <= elem.startbit)
-				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in descending order, but previous element startbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count,
-				      elem.startbit, previous_elem.startbit);
-			if (previous_elem.endbit <= elem.endbit)
-				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in descending order, but previous element endbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count,
-				      elem.endbit, previous_elem.endbit);
-			break;
-		default:
-			break;
+		if (count && elem.endbit <= previous_elem.startbit) {
+			error("[%s] \"%s\" field %zu (%" PRIu64 "-%" PRIu64
+			      ") overlaps with previous field (%" PRIu64 "-%" PRIu64
+			      ") or array is not declared in ascending order\n",
+			      f->mod->name, f->name, count, elem.startbit,
+			      elem.endbit, previous_elem.startbit,
+			      previous_elem.endbit);
 		}
 
 		previous_elem = elem;
-		data_ptr += field_size;
-		count++;
 	}
 }
 
-- 
2.43.0