Received-SPF: pass (google.com: domain of linux-kernel+bounces-95119-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <1cb9a110-c877-4420-9b23-1e7980f1300a@linux.ibm.com>
Date: Thu, 7 Mar 2024 09:17:11 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end
 warnings
To: 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>
References: <ZeIhOT44ON5rjPiP@neat>
 <71aa847b-2edc-44a2-beb7-3610bf744937@linux.alibaba.com>
From: Jan Karcher <jaka@linux.ibm.com>
Organization: IBM - Network Linux on Z
In-Reply-To: <71aa847b-2edc-44a2-beb7-3610bf744937@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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?

[...]

>   };
> 
> 
> Thanks!
> Wen Gu

Thanks you
- Jan