| 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
| ||
|
Message-ID: <fd7e4c2f-0d8f-4b1c-86af-9bf472cb7d0f@linux.ibm.com>
Date: Tue, 12 Mar 2024 08:54:46 +0100
From: Jan Karcher <jaka@...ux.ibm.com>
To: Wen Gu <guwen@...ux.alibaba.com>,
"Gustavo A. R. Silva" <gustavo@...eddedor.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 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@...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.
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
Powered by blists - more mailing lists