diff mbox series

[next] net/smc: Avoid -Wflex-array-member-not-at-end warnings

Message ID ZeIhOT44ON5rjPiP@neat
State New
Headers
Series [next] net/smc: Avoid -Wflex-array-member-not-at-end warnings |

Commit Message

Gustavo A. R. Silva March 1, 2024, 6:40 p.m. UTC 
  -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.

There are currently a couple of objects in `struct smc_clc_msg_proposal_area`
that contain a couple of flexible structures:

struct smc_clc_msg_proposal_area {
	...
	struct smc_clc_v2_extension             pclc_v2_ext;
	...
	struct smc_clc_smcd_v2_extension        pclc_smcd_v2_ext;
	...
};

So, in order to avoid ending up with a couple of flexible-array members
in the middle of a struct, we use the `struct_group_tagged()` helper to
separate the flexible array from the rest of the members in the flexible
structure:

struct smc_clc_smcd_v2_extension {
        struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
                            u8 system_eid[SMC_MAX_EID_LEN];
                            u8 reserved[16];
        );
        struct smc_clc_smcd_gid_chid gidchid[];
};

With the change described above, we now declare objects of the type of
the tagged struct without embedding flexible arrays in the middle of
another struct:

struct smc_clc_msg_proposal_area {
        ...
        struct smc_clc_v2_extension_hdr		pclc_v2_ext;
        ...
        struct smc_clc_smcd_v2_extension_hdr	pclc_smcd_v2_ext;
        ...
};

We also use `container_of()` when we need to retrieve a pointer to the
flexible structures.

So, with these changes, fix the following warnings:

In file included from net/smc/af_smc.c:42:
net/smc/smc_clc.h:186:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
  186 |         struct smc_clc_v2_extension             pclc_v2_ext;
      |                                                 ^~~~~~~~~~~
net/smc/smc_clc.h:188:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
  188 |         struct smc_clc_smcd_v2_extension        pclc_smcd_v2_ext;
      |                                                 ^~~~~~~~~~~~~~~~

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 net/smc/smc_clc.c |  5 +++--
 net/smc/smc_clc.h | 24 ++++++++++++++----------
 2 files changed, 17 insertions(+), 12 deletions(-)

Comments

Kees Cook March 2, 2024, 12:09 a.m. UTC | #1 
On Fri, Mar 01, 2024 at 12:40:57PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
> 
> There are currently a couple of objects in `struct smc_clc_msg_proposal_area`
> that contain a couple of flexible structures:
> 
> struct smc_clc_msg_proposal_area {
> 	...
> 	struct smc_clc_v2_extension             pclc_v2_ext;
> 	...
> 	struct smc_clc_smcd_v2_extension        pclc_smcd_v2_ext;
> 	...
> };
> 
> So, in order to avoid ending up with a couple of flexible-array members
> in the middle of a struct, we use the `struct_group_tagged()` helper to
> separate the flexible array from the rest of the members in the flexible
> structure:
> 
> struct smc_clc_smcd_v2_extension {
>         struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
>                             u8 system_eid[SMC_MAX_EID_LEN];
>                             u8 reserved[16];
>         );
>         struct smc_clc_smcd_gid_chid gidchid[];
> };
> 
> With the change described above, we now declare objects of the type of
> the tagged struct without embedding flexible arrays in the middle of
> another struct:
> 
> struct smc_clc_msg_proposal_area {
>         ...
>         struct smc_clc_v2_extension_hdr		pclc_v2_ext;
>         ...
>         struct smc_clc_smcd_v2_extension_hdr	pclc_smcd_v2_ext;
>         ...
> };
> 
> We also use `container_of()` when we need to retrieve a pointer to the
> flexible structures.
> 
> So, with these changes, fix the following warnings:
> 
> In file included from net/smc/af_smc.c:42:
> net/smc/smc_clc.h:186:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>   186 |         struct smc_clc_v2_extension             pclc_v2_ext;
>       |                                                 ^~~~~~~~~~~
> net/smc/smc_clc.h:188:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>   188 |         struct smc_clc_smcd_v2_extension        pclc_smcd_v2_ext;
>       |                                                 ^~~~~~~~~~~~~~~~
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

I think this is a nice way to deal with these flex-array cases. Using
the struct_group() and container_of() means there is very little
collateral impact. Since this is isolated to a single file, I wonder if
it's easy to check that there are no binary differences too? I wouldn't
expect any -- container_of() is all constant expressions, so the
assignment offsets should all be the same, etc.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees
Wen Gu March 4, 2024, 9 a.m. UTC | #2 
On 2024/3/2 02:40, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
> 
> There are currently a couple of objects in `struct smc_clc_msg_proposal_area`
> that contain a couple of flexible structures:
> 
> struct smc_clc_msg_proposal_area {
> 	...
> 	struct smc_clc_v2_extension             pclc_v2_ext;
> 	...
> 	struct smc_clc_smcd_v2_extension        pclc_smcd_v2_ext;
> 	...
> };
> 
> So, in order to avoid ending up with a couple of flexible-array members
> in the middle of a struct, we use the `struct_group_tagged()` helper to
> separate the flexible array from the rest of the members in the flexible
> structure:
> 
> struct smc_clc_smcd_v2_extension {
>          struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
>                              u8 system_eid[SMC_MAX_EID_LEN];
>                              u8 reserved[16];
>          );
>          struct smc_clc_smcd_gid_chid gidchid[];
> };
> 
> With the change described above, we now declare objects of the type of
> the tagged struct without embedding flexible arrays in the middle of
> another struct:
> 
> struct smc_clc_msg_proposal_area {
>          ...
>          struct smc_clc_v2_extension_hdr		pclc_v2_ext;
>          ...
>          struct smc_clc_smcd_v2_extension_hdr	pclc_smcd_v2_ext;
>          ...
> };
> 
> We also use `container_of()` when we need to retrieve a pointer to the
> flexible structures.
> 
> So, with these changes, fix the following warnings:
> 
> In file included from net/smc/af_smc.c:42:
> net/smc/smc_clc.h:186:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>    186 |         struct smc_clc_v2_extension             pclc_v2_ext;
>        |                                                 ^~~~~~~~~~~
> net/smc/smc_clc.h:188:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>    188 |         struct smc_clc_smcd_v2_extension        pclc_smcd_v2_ext;
>        |                                                 ^~~~~~~~~~~~~~~~
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>   net/smc/smc_clc.c |  5 +++--
>   net/smc/smc_clc.h | 24 ++++++++++++++----------
>   2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
> index e55026c7529c..3094cfa1c458 100644
> --- a/net/smc/smc_clc.c
> +++ b/net/smc/smc_clc.c
> @@ -853,8 +853,9 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
>   	pclc_smcd = &pclc->pclc_smcd;
>   	pclc_prfx = &pclc->pclc_prfx;
>   	ipv6_prfx = pclc->pclc_prfx_ipv6;
> -	v2_ext = &pclc->pclc_v2_ext;
> -	smcd_v2_ext = &pclc->pclc_smcd_v2_ext;
> +	v2_ext = container_of(&pclc->pclc_v2_ext, struct smc_clc_v2_extension, _hdr);
> +	smcd_v2_ext = container_of(&pclc->pclc_smcd_v2_ext,
> +				   struct smc_clc_smcd_v2_extension, hdr);
>   	gidchids = pclc->pclc_gidchids;
>   	trl = &pclc->pclc_trl;
>   
> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
> index 7cc7070b9772..5b91a1947078 100644
> --- a/net/smc/smc_clc.h
> +++ b/net/smc/smc_clc.h
> @@ -134,12 +134,14 @@ struct smc_clc_smcd_gid_chid {
>   			 */
>   
>   struct smc_clc_v2_extension {
> -	struct smc_clnt_opts_area_hdr hdr;
> -	u8 roce[16];		/* RoCEv2 GID */
> -	u8 max_conns;
> -	u8 max_links;
> -	__be16 feature_mask;
> -	u8 reserved[12];
> +	struct_group_tagged(smc_clc_v2_extension_hdr, _hdr,
> +		struct smc_clnt_opts_area_hdr hdr;
> +		u8 roce[16];		/* RoCEv2 GID */
> +		u8 max_conns;
> +		u8 max_links;
> +		__be16 feature_mask;
> +		u8 reserved[12];
> +	);
>   	u8 user_eids[][SMC_MAX_EID_LEN];
>   };
>   
> @@ -159,8 +161,10 @@ struct smc_clc_msg_smcd {	/* SMC-D GID information */
>   };
>   
>   struct smc_clc_smcd_v2_extension {
> -	u8 system_eid[SMC_MAX_EID_LEN];
> -	u8 reserved[16];
> +	struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
> +		u8 system_eid[SMC_MAX_EID_LEN];
> +		u8 reserved[16];
> +	);
>   	struct smc_clc_smcd_gid_chid gidchid[];
>   };
>   
> @@ -183,9 +187,9 @@ struct smc_clc_msg_proposal_area {
>   	struct smc_clc_msg_smcd			pclc_smcd;
>   	struct smc_clc_msg_proposal_prefix	pclc_prfx;
>   	struct smc_clc_ipv6_prefix	pclc_prfx_ipv6[SMC_CLC_MAX_V6_PREFIX];
> -	struct smc_clc_v2_extension		pclc_v2_ext;
> +	struct smc_clc_v2_extension_hdr		pclc_v2_ext;
>   	u8			user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
> -	struct smc_clc_smcd_v2_extension	pclc_smcd_v2_ext;
> +	struct smc_clc_smcd_v2_extension_hdr	pclc_smcd_v2_ext;
>   	struct smc_clc_smcd_gid_chid
>   				pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES];
>   	struct smc_clc_msg_trail		pclc_trl;

Thank you! Gustavo. This patch can fix this warning well, just the name
'*_hdr' might not be very accurate, but I don't have a good idea ATM.

Besides, I am wondering if this can be fixed by moving
user_eids of smc_clc_msg_proposal_area into smc_clc_v2_extension,
and
pclc_gidchids of smc_clc_msg_proposal_area into smc_clc_smcd_v2_extension.

so that we can avoid to use the flexible-array in smc_clc_v2_extension
and smc_clc_smcd_v2_extension.


diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index e55026c7529c..971b4677b84d 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -855,7 +855,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
         ipv6_prfx = pclc->pclc_prfx_ipv6;
         v2_ext = &pclc->pclc_v2_ext;
         smcd_v2_ext = &pclc->pclc_smcd_v2_ext;
-       gidchids = pclc->pclc_gidchids;
+       gidchids = pclc->pclc_smcd_v2_ext.gidchid;
         trl = &pclc->pclc_trl;

         pclc_base->hdr.version = SMC_V2;
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 7cc7070b9772..36c8432af73e 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -133,6 +133,14 @@ struct smc_clc_smcd_gid_chid {
                          * (https://www.ibm.com/support/pages/node/6326337)
                          */

+#define SMC_CLC_MAX_V6_PREFIX          8
+#define SMC_CLC_MAX_UEID               8
+#define SMCD_CLC_MAX_V2_GID_ENTRIES    8 /* max # of CHID-GID entries in CLC
+                                          * proposal SMC-Dv2 extension.
+                                          * each ISM device takes one entry and
+                                          * each Emulated-ISM takes two entries
+                                          */
+
  struct smc_clc_v2_extension {
         struct smc_clnt_opts_area_hdr hdr;
         u8 roce[16];            /* RoCEv2 GID */
@@ -140,7 +148,7 @@ struct smc_clc_v2_extension {
         u8 max_links;
         __be16 feature_mask;
         u8 reserved[12];
-       u8 user_eids[][SMC_MAX_EID_LEN];
+       u8 user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
  };

  struct smc_clc_msg_proposal_prefix {   /* prefix part of clc proposal message*/
@@ -161,7 +169,7 @@ struct smc_clc_msg_smcd {   /* SMC-D GID information */
  struct smc_clc_smcd_v2_extension {
         u8 system_eid[SMC_MAX_EID_LEN];
         u8 reserved[16];
-       struct smc_clc_smcd_gid_chid gidchid[];
+       struct smc_clc_smcd_gid_chid gidchid[SMCD_CLC_MAX_V2_GID_ENTRIES];
  };

  struct smc_clc_msg_proposal {  /* clc proposal message sent by Linux */
@@ -170,24 +178,13 @@ struct smc_clc_msg_proposal {     /* clc proposal message sent by Linux */
         __be16 iparea_offset;   /* offset to IP address information area */
  } __aligned(4);

-#define SMC_CLC_MAX_V6_PREFIX          8
-#define SMC_CLC_MAX_UEID               8
-#define SMCD_CLC_MAX_V2_GID_ENTRIES    8 /* max # of CHID-GID entries in CLC
-                                          * proposal SMC-Dv2 extension.
-                                          * each ISM device takes one entry and
-                                          * each Emulated-ISM takes two entries
-                                          */
-
  struct smc_clc_msg_proposal_area {
         struct smc_clc_msg_proposal             pclc_base;
         struct smc_clc_msg_smcd                 pclc_smcd;
         struct smc_clc_msg_proposal_prefix      pclc_prfx;
         struct smc_clc_ipv6_prefix      pclc_prfx_ipv6[SMC_CLC_MAX_V6_PREFIX];
         struct smc_clc_v2_extension             pclc_v2_ext;
-       u8                      user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
         struct smc_clc_smcd_v2_extension        pclc_smcd_v2_ext;
-       struct smc_clc_smcd_gid_chid
-                               pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES];
         struct smc_clc_msg_trail                pclc_trl;
  };


Thanks!
Wen Gu
diff mbox series

Patch

diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index e55026c7529c..3094cfa1c458 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -853,8 +853,9 @@  int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 	pclc_smcd = &pclc->pclc_smcd;
 	pclc_prfx = &pclc->pclc_prfx;
 	ipv6_prfx = pclc->pclc_prfx_ipv6;
-	v2_ext = &pclc->pclc_v2_ext;
-	smcd_v2_ext = &pclc->pclc_smcd_v2_ext;
+	v2_ext = container_of(&pclc->pclc_v2_ext, struct smc_clc_v2_extension, _hdr);
+	smcd_v2_ext = container_of(&pclc->pclc_smcd_v2_ext,
+				   struct smc_clc_smcd_v2_extension, hdr);
 	gidchids = pclc->pclc_gidchids;
 	trl = &pclc->pclc_trl;
 
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 7cc7070b9772..5b91a1947078 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -134,12 +134,14 @@  struct smc_clc_smcd_gid_chid {
 			 */
 
 struct smc_clc_v2_extension {
-	struct smc_clnt_opts_area_hdr hdr;
-	u8 roce[16];		/* RoCEv2 GID */
-	u8 max_conns;
-	u8 max_links;
-	__be16 feature_mask;
-	u8 reserved[12];
+	struct_group_tagged(smc_clc_v2_extension_hdr, _hdr,
+		struct smc_clnt_opts_area_hdr hdr;
+		u8 roce[16];		/* RoCEv2 GID */
+		u8 max_conns;
+		u8 max_links;
+		__be16 feature_mask;
+		u8 reserved[12];
+	);
 	u8 user_eids[][SMC_MAX_EID_LEN];
 };
 
@@ -159,8 +161,10 @@  struct smc_clc_msg_smcd {	/* SMC-D GID information */
 };
 
 struct smc_clc_smcd_v2_extension {
-	u8 system_eid[SMC_MAX_EID_LEN];
-	u8 reserved[16];
+	struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
+		u8 system_eid[SMC_MAX_EID_LEN];
+		u8 reserved[16];
+	);
 	struct smc_clc_smcd_gid_chid gidchid[];
 };
 
@@ -183,9 +187,9 @@  struct smc_clc_msg_proposal_area {
 	struct smc_clc_msg_smcd			pclc_smcd;
 	struct smc_clc_msg_proposal_prefix	pclc_prfx;
 	struct smc_clc_ipv6_prefix	pclc_prfx_ipv6[SMC_CLC_MAX_V6_PREFIX];
-	struct smc_clc_v2_extension		pclc_v2_ext;
+	struct smc_clc_v2_extension_hdr		pclc_v2_ext;
 	u8			user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
-	struct smc_clc_smcd_v2_extension	pclc_smcd_v2_ext;
+	struct smc_clc_smcd_v2_extension_hdr	pclc_smcd_v2_ext;
 	struct smc_clc_smcd_gid_chid
 				pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES];
 	struct smc_clc_msg_trail		pclc_trl;