Re: [PATCH net-next v8 03/10] lib: packing: add pack_fields() and unpack_fields()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Dec 03, 2024 at 03:53:49PM -0800, Jacob Keller wrote:
> +#define CHECK_PACKED_FIELD(field) ({ \
> +	typeof(field) __f = (field); \
> +	BUILD_BUG_ON(__f.startbit < __f.endbit); \
> +	BUILD_BUG_ON(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size); \
> +	BUILD_BUG_ON(__f.size != 1 && __f.size != 2 && \
> +		     __f.size != 4 && __f.size != 8); \
> +})
> +
> +
> +#define CHECK_PACKED_FIELD_OVERLAP(ascending, field1, field2) ({ \
> +	typeof(field1) _f1 = (field1); typeof(field2) _f2 = (field2); \
> +	const bool _a = (ascending); \
> +	BUILD_BUG_ON(_a && _f1.startbit >= _f2.startbit); \
> +	BUILD_BUG_ON(!_a && _f1.startbit <= _f2.startbit); \
> +	BUILD_BUG_ON(max(_f1.endbit, _f2.endbit) <= \
> +		     min(_f1.startbit, _f2.startbit)); \
> +})
> +
> +#define CHECK_PACKED_FIELDS_SIZE(fields, pbuflen) ({ \
> +	typeof(&(fields)[0]) _f = (fields); \
> +	typeof(pbuflen) _len = (pbuflen); \
> +	const size_t num_fields = ARRAY_SIZE(fields); \
> +	BUILD_BUG_ON(!__builtin_constant_p(_len)); \
> +	BUILD_BUG_ON(_f[0].startbit >= BITS_PER_BYTE * _len); \

Please add a comment here stating that we check both the first and last
element to cover the ascending as well as descending ordering scenarios.
It took me a while to realize this, I thought the _f[0] check was unnecessary.

> +	BUILD_BUG_ON(_f[num_fields - 1].startbit >= BITS_PER_BYTE * _len); \
> +})
> +
>  #define QUIRK_MSB_ON_THE_RIGHT	BIT(0)
>  #define QUIRK_LITTLE_ENDIAN	BIT(1)
>  #define QUIRK_LSW32_IS_FIRST	BIT(2)

I spent some time today to play around with this version, and it seems
to work, but I took some liberty and made the following changes:

- Tail-call CHECK_PACKED_FIELD_OVERLAP() from CHECK_PACKED_FIELD(). This
  reduces the size of the generated code from 2753 lines to 1478 lines,
  which already brings it a little bit more into the realm of "tolerable" IMO.

- Remove the BUILD_BUG_ON(ARRAY_SIZE(fields) == N), since I think
  that's just wasteful (in terms of space and compiler CPU cycles) and
  ultra-defensive, when the auto-generated __builtin_choose_expr() is
  the only caller. It was justified when the consumer had to explicitly
  select the right checking macro.

- Add some prettier error messages. Compare (for an error injected by me):

../drivers/net/ethernet/intel/ice/ice_common.c:1419:2: error: call to '__compiletime_assert_3302' declared with 'error' attribute: BUILD_BUG_ON failed: max(_f1.endbit, _f2.endbit) <= min(_f1.startbit, _f2.startbit)
        pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
        ^

with:


../drivers/net/ethernet/intel/ice/ice_common.c:1419:2: error: call to '__compiletime_assert_3414' declared with 'error' attribute: ice_rlan_ctx_fields field 3 overlaps with previous field
        pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
        ^


That incremental improvement is below, if you'd be interested in including it
(the auto-generated code is not part of the diff):

diff --git a/include/linux/packing.h b/include/linux/packing.h
index c4fc76ae64a5..1c89a5129b06 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -36,22 +36,38 @@ struct packed_field_m {
 	sizeof_field(struct_name, struct_field), \
 }
 
-#define CHECK_PACKED_FIELD(field) ({ \
-	typeof(field) __f = (field); \
-	BUILD_BUG_ON(__f.startbit < __f.endbit); \
-	BUILD_BUG_ON(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size); \
-	BUILD_BUG_ON(__f.size != 1 && __f.size != 2 && \
-		     __f.size != 4 && __f.size != 8); \
+#define CHECK_PACKED_FIELD_OVERLAP(fields, index1, index2) ({ \
+	typeof(&(fields)[0]) __f = (fields); \
+	typeof(__f[0]) _f1 = __f[index1]; typeof(__f[0]) _f2 = __f[index2]; \
+	const bool _ascending = __f[0].startbit < __f[1].startbit; \
+	BUILD_BUG_ON_MSG(_ascending && _f1.startbit >= _f2.startbit, \
+			 __stringify(fields) " field " __stringify(index2) \
+			 " breaks ascending order"); \
+	BUILD_BUG_ON_MSG(!_ascending && _f1.startbit <= _f2.startbit, \
+			 __stringify(fields) " field " __stringify(index2) \
+			 " breaks descending order"); \
+	BUILD_BUG_ON_MSG(max(_f1.endbit, _f2.endbit) <= \
+			 min(_f1.startbit, _f2.startbit), \
+			 __stringify(fields) " field " __stringify(index2) \
+			 " overlaps with previous field"); \
 })
 
-
-#define CHECK_PACKED_FIELD_OVERLAP(ascending, field1, field2) ({ \
-	typeof(field1) _f1 = (field1); typeof(field2) _f2 = (field2); \
-	const bool _a = (ascending); \
-	BUILD_BUG_ON(_a && _f1.startbit >= _f2.startbit); \
-	BUILD_BUG_ON(!_a && _f1.startbit <= _f2.startbit); \
-	BUILD_BUG_ON(max(_f1.endbit, _f2.endbit) <= \
-		     min(_f1.startbit, _f2.startbit)); \
+#define CHECK_PACKED_FIELD(fields, index) ({ \
+	typeof(&(fields)[0]) _f = (fields); \
+	typeof(_f[0]) __f = _f[index]; \
+	BUILD_BUG_ON_MSG(__f.startbit < __f.endbit, \
+			 __stringify(fields) " field " __stringify(index) \
+			 " start bit must not be smaller than end bit"); \
+	BUILD_BUG_ON_MSG(__f.size != 1 && __f.size != 2 && \
+			 __f.size != 4 && __f.size != 8, \
+			 __stringify(fields) " field " __stringify(index) \
+			" has unsupported unpacked storage size"); \
+	BUILD_BUG_ON_MSG(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size, \
+			 __stringify(fields) " field " __stringify(index) \
+			 " exceeds unpacked storage size"); \
+	__builtin_choose_expr(index != 0, \
+			      CHECK_PACKED_FIELD_OVERLAP(fields, index - 1, index), \
+			      1); \
 })
 
 #define CHECK_PACKED_FIELDS_SIZE(fields, pbuflen) ({ \
diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c
index 09a21afd640b..fabbb741c9a8 100644
--- a/scripts/gen_packed_field_checks.c
+++ b/scripts/gen_packed_field_checks.c
@@ -9,15 +9,9 @@ int main(int argc, char **argv)
 {
 	for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) {
 		printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
-		printf("\ttypeof(&(fields)[0]) _f = (fields); \\\n");
-		printf("\tBUILD_BUG_ON(ARRAY_SIZE(fields) != %d); \\\n", i);
 
 		for (int j = 0; j < i; j++)
-			printf("\tCHECK_PACKED_FIELD(_f[%d]); \\\n", j);
-
-		for (int j = 1; j < i; j++)
-			printf("\tCHECK_PACKED_FIELD_OVERLAP(_f[0].startbit < _f[1].startbit, _f[%d], _f[%d]); \\\n",
-			       j - 1, j);
+			printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);
 
 		printf("})\n\n");
 	}

And there's one more thing I tried, which mostly worked. That was to
express CHECK_PACKED_FIELDS_N in terms of CHECK_PACKED_FIELDS_N-1.
This further reduced the auto-generated code size from 1478 lines to 302
lines, which I think is appealing.

diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c
index fabbb741c9a8..bac85c04ef20 100644
--- a/scripts/gen_packed_field_checks.c
+++ b/scripts/gen_packed_field_checks.c
@@ -10,9 +10,10 @@ int main(int argc, char **argv)
 	for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) {
 		printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
 
-		for (int j = 0; j < i; j++)
-			printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);
+		if (i != 1)
+			printf("\tCHECK_PACKED_FIELDS_%d(fields); \\\n", i - 1);
 
+		printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", i);
 		printf("})\n\n");
 	}
 

The problem is that, for some reason, it introduces this sparse warning:

../lib/packing_test.c:436:9: warning: invalid access past the end of 'test_fields' (24 24)
../lib/packing_test.c:448:9: warning: invalid access past the end of 'test_fields' (24 24)

Nobody accesses past element 6 (ARRAY_SIZE) of test_fields[]. I ran the
KUnit with kasan and I saw no warning. The strace warning comes from
check_access() in flow.c, but I don't have any energy left today to go
further into this.

I'm suspecting either a strace bug/false positive, or some sort of
variable name aliasing issue which I haven't identified yet.