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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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>

Thanks!
--
Gustavo


[...]

  };


Thanks!
Wen Gu

Thanks you
- Jan




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux