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

Jan Karcher <jaka@xxxxxxxxxxxxx> · Tue, 12 Mar 2024 08:54:46 +0100

On 11/03/2024 11:59, Wen Gu wrote:


On 2024/3/8 07:46, Gustavo A. R. Silva wrote:


On 3/7/24 02:17, Jan Karcher wrote:


On 04/03/2024 10:00, Wen Gu wrote:


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:


Thank you Gustavo for the proposal.
I had to do some reading to better understand what's happening and 
how your patch solves this.

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

I agree. Should we chose this option we should come up for a better 
name.


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.

I like the idea and put some thought into it. The only thing that is 
not perfectly clean IMO is the following:
By the current definition it is easily visible that we are dealing 
with a variable sized array. If we move them into the structs one 
could think they are always at their MAX size which they are not.
E.g.: An incoming proposal can have 0 UEIDs indicated by the eid_cnt.
That said nothing a comment can't fix.

 From what i have seen the offset and length calculations regarding 
the "real" size of those structs is fine with your proposal.

Can you verify that your changes also resolve the warnings?

I can confirm that the changes Wen Gu is proposing also resolve the 
warnings.

Wen,

If you send a proper patch, you can include the following tags:

Reviewed-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
Build-tested-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>


Hi Gustavo, thank you for the confirmation that my proposal can fix the 
warning.

But I found that I may have something missed in my proposal when I think 
further.
My proposal changed the sizes of struct smc_clc_v2_extension and 
smc_clc_smcd_v2_extension,
and some places in SMC need them, such as the fill of kvec in 
smc_clc_send_proposal().

So my proposal may involve more changes to current SMC code, and I think 
it is
not as clean as your solution. So I perfer yours now.

Hi Wen Gu,

you're right. I missed that the offset calculation is broken with your 
proposal since the full size of the array is already included in this 
case which means we would have to subtract the empty slots instead of 
adding the full ones.
My bad. Thinking about adding a testcase to sxplicit check the size of 
the CLC Messages send in the future.


And as for the name, I think maybe we can use '*_elems' as a suffix, at 
least it
is unambiguous. So it will be smc_clc_v2_extension_elems and 
smc_clc_smcd_v2_extension_elems.


Jan, what do you think of the name '*_elems' ?

Hmm... I think it is way better than priv. One more proposal from my 
side would be *_fixed since this is the fixed content and not variable. 
I'm open for both.

Which one would you prefer more?


Thanks!

Thanks!
--
Gustavo


[...]

  };


Thanks!
Wen Gu

Thanks you
- Jan







