From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
To: Jan Karcher <jaka@linux.ibm.com>,
Wen Gu <guwen@linux.alibaba.com>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Wenjia Zhang <wenjia@linux.ibm.com>,
"D. Wythe" <alibuda@linux.alibaba.com>,
Tony Lu <tonylu@linux.alibaba.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: linux-s390@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end warnings
Date: Thu, 7 Mar 2024 17:46:39 -0600 [thread overview]
Message-ID: <82c1dc9e-d5b6-40e3-9d81-d18cc270724b@embeddedor.com> (raw)
In-Reply-To: <1cb9a110-c877-4420-9b23-1e7980f1300a@linux.ibm.com>
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@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.
>
> 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@kernel.org>
Build-tested-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
--
Gustavo
>
> [...]
>
>> };
>>
>>
>> Thanks!
>> Wen Gu
>
> Thanks you
> - Jan
next prev parent reply other threads:[~2024-03-07 23:46 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-01 18:40 [PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva
2024-03-02 0:09 ` Kees Cook
2024-03-04 9:00 ` Wen Gu
2024-03-07 8:17 ` Jan Karcher
2024-03-07 23:46 ` Gustavo A. R. Silva [this message]
2024-03-11 10:59 ` Wen Gu
2024-03-12 7:54 ` Jan Karcher
2024-03-12 9:53 ` Wen Gu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=82c1dc9e-d5b6-40e3-9d81-d18cc270724b@embeddedor.com \
--to=gustavo@embeddedor.com \
--cc=alibuda@linux.alibaba.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gustavoars@kernel.org \
--cc=guwen@linux.alibaba.com \
--cc=jaka@linux.ibm.com \
--cc=keescook@chromium.org \
--cc=kuba@kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=tonylu@linux.alibaba.com \
--cc=wenjia@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).