Gustavo A. R. Silva

A couple of months ago, I submitted a patch to address a dozen -Wflex-array-member-not-at-end warnings in ACPI:

• UAPI: ndctl / acpi: intel: Avoid multiple -Wflex-array-member-not-at-end warnings

Originally, I opted for the __struct_group() approach to fix these issues.

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 73516e263627..34c11644d5d7 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -227,12 +227,15 @@ enum ars_masks {
  */
 
 struct nd_cmd_pkg {
-	__u64   nd_family;		/* family of commands */
-	__u64   nd_command;
-	__u32   nd_size_in;		/* INPUT: size of input args */
-	__u32   nd_size_out;		/* INPUT: size of payload */
-	__u32   nd_reserved2[9];	/* reserved must be zero */
-	__u32   nd_fw_size;		/* OUTPUT: size fw wants to return */
+	/* New members MUST be added within the __struct_group() macro below. */
+	__struct_group(nd_cmd_pkg_hdr, __hdr, /* no attrs */,
+		__u64   nd_family;		/* family of commands */
+		__u64   nd_command;
+		__u32   nd_size_in;		/* INPUT: size of input args */
+		__u32   nd_size_out;		/* INPUT: size of payload */
+		__u32   nd_reserved2[9];	/* reserved must be zero */
+		__u32   nd_fw_size;		/* OUTPUT: size fw wants to return */
+	);
 	unsigned char nd_payload[];	/* Contents of call      */
 };

The idea behind this approach is to _separate_ the flexible-array member from the rest of the members in the flexible structure, while at the same time a new type for the “header part” of the flexible structure is created. We then proceed to use the newly created type to replace the type of the objects causing trouble –the ones causing the -Wflex-array-member-not-at-end warnings– in the composite structures.

However, the approach briefly described above is usually better suited for fixing instances of structures with a flexible-array member in the middle that are intended to live on the heap –I’ll go into more detail about this approach in a future post.

So, after some time, I realized that using the DEFINE_RAW_FLEX() helper was probably a better approach in this particular case, since basically all the instances triggering the -Wflex-array-member-not-at-end warnings were on-stack objects.

The DEFINE_FLEX() and DEFINE_RAW_FLEX() macros were specifically designed to define automatic (on-stack) objects of a flexible structure type, where the size of the flexible-array member is known at compile time.

So, I submitted a new version of the patch above, this time using DEFINE_RAW_FLEX(), and without any changes in the UAPI header because we didn’t need to change struct d_cmd_pkg anymore:

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 3902759abcba..114d5b3bb39b 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -55,21 +55,17 @@ static unsigned long intel_security_flags(struct nvdimm *nvdimm,
 {
 	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
 	unsigned long security_flags = 0;
-	struct {
-		struct nd_cmd_pkg pkg;
-		struct nd_intel_get_security_state cmd;
-	} nd_cmd = {
-		.pkg = {
-			.nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
-			.nd_family = NVDIMM_FAMILY_INTEL,
-			.nd_size_out =
-				sizeof(struct nd_intel_get_security_state),
-			.nd_fw_size =
-				sizeof(struct nd_intel_get_security_state),
-		},
-	};
+	DEFINE_RAW_FLEX(struct nd_cmd_pkg, nd_cmd, nd_payload,
+			sizeof(struct nd_intel_get_security_state));
+	struct nd_intel_get_security_state *cmd =
+			(struct nd_intel_get_security_state *)nd_cmd->nd_payload;
 	int rc;
 
+	nd_cmd->nd_command = NVDIMM_INTEL_GET_SECURITY_STATE;
+	nd_cmd->nd_family = NVDIMM_FAMILY_INTEL;
+	nd_cmd->nd_size_out = sizeof(struct nd_intel_get_security_state);
+	nd_cmd->nd_fw_size = sizeof(struct nd_intel_get_security_state);
+
 	if (!test_bit(NVDIMM_INTEL_GET_SECURITY_STATE, &nfit_mem->dsm_mask))
 		return 0;

• acpi: nfit: intel: Avoid multiple -Wflex-array-member-not-at-end warnings

To what Dan Williams replied:

Can this keep the C99 init-style with something like (untested):

_DEFINE_FLEX(struct nd_cmd_pkg, nd_cmd, nd_payload,
             sizeof(struct nd_intel_get_security_state), {
		.pkg = {
		        .nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
		        .nd_family = NVDIMM_FAMILY_INTEL,
		        .nd_size_out =
		                sizeof(struct nd_intel_get_security_state),
		        .nd_fw_size =
		                sizeof(struct nd_intel_get_security_state),
		},
	});
	

?

At the moment, the short answer was: no –the helper was not able to do that “cleanly.”

However, Dan’s question along with another issue I was recently trying to address, led me to take another look at the internals of _DEFINE_FLEX():

391 /**
392  * _DEFINE_FLEX() - helper macro for DEFINE_FLEX() family.
393  * Enables caller macro to pass (different) initializer.
394  *
395  * @type: structure type name, including "struct" keyword.
396  * @name: Name for a variable to define.
397  * @member: Name of the array member.
398  * @count: Number of elements in the array; must be compile-time const.
399  * @initializer: initializer expression (could be empty for no init).
400  */
401 #define _DEFINE_FLEX(type, name, member, count, initializer...)                 \
402         _Static_assert(__builtin_constant_p(count),                             \
403                        "onstack flex array members require compile-time..."); \
404         union {                                                                 \
405                 u8 bytes[struct_size_t(type, member, count)];                   \
406                 type obj;                                                       \
407         } name##_u initializer;                                                 \
408         type *name = (type *)&name##_u

The “macro wizardry” behind this helper allows for the allocation of the total space needed for an instance of a flexible structure along with its flexible-array member on the stack. As mentioned above, there are situations where the size of the flexible-array member is known at compile time. In these cases, we can rely on the DEFINE_FLEX() family of helpers to declare the necessary objects, ensuring that any flexible-array member remains the last member of the composite structure –not placed in the middle.

Dan noticed that DEFINE_FLEX() and DEFINE_RAW_FLEX() are just wrappers for _DEFINE_FLEX(), and that these wrappers don’t allow for external static initialization:

424 #define DEFINE_RAW_FLEX(type, name, member, count)      \
425         _DEFINE_FLEX(type, name, member, count, = {})

...

442 #define DEFINE_FLEX(TYPE, NAME, MEMBER, COUNTER, COUNT) \
443         _DEFINE_FLEX(TYPE, NAME, MEMBER, COUNT, = { .obj.COUNTER = COUNT, })

However, _DEFINE_FLEX() _does_ have the infrastructure to initialize the members of the TYPE object created by this helper –with just a _small_ tweak: the obj member created by the helper at line 406 must be exposed to the call site, as shown in DEFINE_FLEX() at line 443 above when COUNTER is set to COUNT.

So I replied the following:

The code below works - however, notice that in this case we should
go through 'obj', which is an object defined in _DEFINE_FLEX().

         _DEFINE_FLEX(struct nd_cmd_pkg, nd_cmd, nd_payload,
                         sizeof(struct nd_intel_get_security_state), = {
                 .obj = {
                         .nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
                         .nd_family = NVDIMM_FAMILY_INTEL,
                         .nd_size_out =
                                 sizeof(struct nd_intel_get_security_state),
                         .nd_fw_size =
                                 sizeof(struct nd_intel_get_security_state),
                 },
         });

Then, in a subsequent e-mail I commented:

Now, I can modify the helper like this:

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 69533e703be5..170d3cfe7ecc 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -404,7 +404,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
         union {                                                                 \
                 u8 bytes[struct_size_t(type, member, count)];                   \
                 type obj;                                                       \
-       } name##_u initializer;                                                 \
+       } name##_u = { .obj initializer };                                      \
         type *name = (type *)&name##_u

  /**

and then we can use the helper as follows:

         _DEFINE_FLEX(struct nd_cmd_pkg, nd_cmd, nd_payload,
                         sizeof(struct nd_intel_get_security_state), = {
                         .nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
                         .nd_family = NVDIMM_FAMILY_INTEL,
                         .nd_size_out =
                                 sizeof(struct nd_intel_get_security_state),
                         .nd_fw_size =
                                 sizeof(struct nd_intel_get_security_state),
         });

OK, I'll go and update the helper.

-Gustavo

And that’s exactly what I did in the patch below. With that change, the _DEFINE_FLEX() helper now works correctly and can statically initialize struct members without exposing any internals. 🙂

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f33d74dac06f2b..7b7be27ca11318 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -396,7 +396,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
  * @name: Name for a variable to define.
  * @member: Name of the array member.
  * @count: Number of elements in the array; must be compile-time const.
- * @initializer: initializer expression (could be empty for no init).
+ * @initializer: Initializer expression (e.g., pass `= { }` at minimum).
  */
 #define _DEFINE_FLEX(type, name, member, count, initializer...)			\
 	_Static_assert(__builtin_constant_p(count),				\
@@ -404,7 +404,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
 	union {									\
 		u8 bytes[struct_size_t(type, member, count)];			\
 		type obj;							\
-	} name##_u initializer;							\
+	} name##_u = { .obj initializer };					\
 	type *name = (type *)&name##_u
 
 /**
@@ -444,7 +444,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
  * elements in array @member.
  */
 #define DEFINE_FLEX(TYPE, NAME, MEMBER, COUNTER, COUNT)	\
-	_DEFINE_FLEX(TYPE, NAME, MEMBER, COUNT, = { .obj.COUNTER = COUNT, })
+	_DEFINE_FLEX(TYPE, NAME, MEMBER, COUNT, = { .COUNTER = COUNT, })

Also, as seen in the patch above, I also fixed the DEFINE_FLEX() wrapper.

So, here is a short example of how to use this helper to statically initialize members in a structure –let’s assume FIXED_SIZE == 1:

struct flex {
        int a;
        int b;
        struct foo flex_array[];
};

_DEFINE_FLEX(struct flex, instance, flex_array, 
             FIXED_SIZE, = {
                .a = 0, 
                .b = 1,
             });

In case you don’t need to initialize any members to specific values, just pass = {} as argument, or probably you should just use DEFINE_RAW_FLEX() or DEFINE_FLEX() instead.

_DEFINE_FLEX(struct flex, instance, flex_array, FIXED_SIZE, = {});

For more details, take a look at the patch already in linux-next:

• overflow: Fix direct struct member initialization in _DEFINE_FLEX()

Thanks! ⚔️🛡️🐧

My next post will be titled “(ab)using DEFINE_FLEX() & DEFINE_RAW_FLEX().” Stay tune!