| Message ID | ZfCXBykRw5XqBvf0@neat (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,next] net/smc: Avoid -Wflex-array-member-not-at-end warnings | expand |
On Tue, 12 Mar 2024 11:55:19 -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. ## Form letter - net-next-closed The merge window for v6.9 has begun and we have already posted our pull request. Therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after March 25th. RFC patches sent for review only are obviously welcome at any time. See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
On 2024/3/13 01:55, 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_fixed, fixed, > 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_fixed pclc_v2_ext; > ... > struct smc_clc_smcd_v2_extension_fixed 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; > | ^~~~~~~~~~~~~~~~ > > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Hi Gustavo, Thank you for the v2. Some places may need improvement, see below. > --- > Changes in v2: > - Name the tagged struct *_fixed instead of *_hdr. > - Add Kees' RB tag. > > 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..63bb5745ab54 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, fixed); checkpatch complained 'WARNING: line length of 86 exceeds 80 columns' here. It can be reproduced by: ./scripts/checkpatch.pl --strict --max-line-length=80 --ignore=COMMIT_LOG_LONG_LINE,MACRO_ARG_REUSE,ALLOC_SIZEOF_STRUCT,NO_AUTHOR_SIGN_OFF,GIT_COMMIT_ID,CAMELCASE xxx.patch > + smcd_v2_ext = container_of(&pclc->pclc_smcd_v2_ext, > + struct smc_clc_smcd_v2_extension, fixed); > gidchids = pclc->pclc_gidchids; > trl = &pclc->pclc_trl; > > diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h > index 7cc7070b9772..2bfb51daf468 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_fixed, fixed, > + struct smc_clnt_opts_area_hdr hdr; checkpatch: 'CHECK: Alignment should match open parenthesis' > + 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_fixed, fixed, > + u8 system_eid[SMC_MAX_EID_LEN]; checkpatch: 'CHECK: Alignment should match open parenthesis' Thanks! Wen Gu > + 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_fixed 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_fixed 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;
>> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c >> index e55026c7529c..63bb5745ab54 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, fixed); > checkpatch complained 'WARNING: line length of 86 exceeds 80 columns' here. I could probably change this. > > It can be reproduced by: > > ./scripts/checkpatch.pl --strict --max-line-length=80 > --ignore=COMMIT_LOG_LONG_LINE,MACRO_ARG_REUSE,ALLOC_SIZEOF_STRUCT,NO_AUTHOR_SIGN_OFF,GIT_COMMIT_ID,CAMELCASE xxx.patch > >> + smcd_v2_ext = container_of(&pclc->pclc_smcd_v2_ext, >> + struct smc_clc_smcd_v2_extension, fixed); >> gidchids = pclc->pclc_gidchids; >> trl = &pclc->pclc_trl; >> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h >> index 7cc7070b9772..2bfb51daf468 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_fixed, fixed, >> + struct smc_clnt_opts_area_hdr hdr; > > checkpatch: 'CHECK: Alignment should match open parenthesis' I think the code looks better as is --without aligning to match the parenthesis. -- Gustavo
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c index e55026c7529c..63bb5745ab54 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, fixed); + smcd_v2_ext = container_of(&pclc->pclc_smcd_v2_ext, + struct smc_clc_smcd_v2_extension, fixed); gidchids = pclc->pclc_gidchids; trl = &pclc->pclc_trl; diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h index 7cc7070b9772..2bfb51daf468 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_fixed, fixed, + 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_fixed, fixed, + 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_fixed 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_fixed 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;
-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_fixed, fixed, 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_fixed pclc_v2_ext; ... struct smc_clc_smcd_v2_extension_fixed 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; | ^~~~~~~~~~~~~~~~ Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- Changes in v2: - Name the tagged struct *_fixed instead of *_hdr. - Add Kees' RB tag. net/smc/smc_clc.c | 5 +++-- net/smc/smc_clc.h | 24 ++++++++++++++---------- 2 files changed, 17 insertions(+), 12 deletions(-)