| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 11 Mar 2024 18:59:12 +0800
From: Wen Gu <guwen@...ux.alibaba.com>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
Jan Karcher <jaka@...ux.ibm.com>, "Gustavo A. R. Silva"
<gustavoars@...nel.org>, Wenjia Zhang <wenjia@...ux.ibm.com>,
"D. Wythe" <alibuda@...ux.alibaba.com>, Tony Lu <tonylu@...ux.alibaba.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: linux-s390@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end
warnings
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@...nel.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@...nel.org>
> Build-tested-by: Gustavo A. R. Silva <gustavoars@...nel.org>
>
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.
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' ?
Thanks!
> Thanks!
> --
> Gustavo
>
>>
>> [...]
>>
>>> };
>>>
>>>
>>> Thanks!
>>> Wen Gu
>>
>> Thanks you
>> - Jan
Powered by blists - more mailing lists