| 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: <8d06f07c-5bb4-473d-90af-5f57ce2b068f@gmail.com>
Date: Thu, 6 Feb 2025 09:25:43 +0200
From: Tariq Toukan <ttoukan.linux@...il.com>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>,
Tariq Toukan <tariqt@...dia.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-rdma@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] net/mlx5e: Avoid a hundred
-Wflex-array-member-not-at-end warnings
On 06/02/2025 6:37, Gustavo A. R. Silva wrote:
> Hi,
>
> On 04/02/25 13:27, Gustavo A. R. Silva wrote:
>> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
>> getting ready to enable it, globally.
>>
>> So, in order to avoid ending up with a flexible-array member in the
>> middle of other structs, we use the `struct_group_tagged()` helper
>> to create a new tagged `struct mlx5e_umr_wqe_hdr`. This structure
>> groups together all the members of the flexible `struct mlx5e_umr_wqe`
>> except the flexible array.
>>
>> As a result, the array is effectively separated from the rest of the
>> members without modifying the memory layout of the flexible structure.
>> We then change the type of the middle struct member currently causing
>> trouble from `struct mlx5e_umr_wqe` to `struct mlx5e_umr_wqe_hdr`.
>>
>> We also want to ensure that when new members need to be added to the
>> flexible structure, they are always included within the newly created
>> tagged struct. For this, we use `static_assert()`. This ensures that the
>> memory layout for both the flexible structure and the new tagged struct
>> is the same after any changes.
>>
>> This approach avoids having to implement `struct mlx5e_umr_wqe_hdr` as
>> a completely separate structure, thus preventing having to maintain two
>> independent but basically identical structures, closing the door to
>> potential bugs in the future.
>>
>> We also use `container_of()` whenever we need to retrieve a pointer to
>> the flexible structure, through which we can access the flexible-array
>> member, if necessary.
>>
>> So, with these changes, fix 124 of the following warnings:
>>
>> drivers/net/ethernet/mellanox/mlx5/core/en.h:664:48: warning:
>> structure containing a flexible array member is not at the end of
>> another structure [-Wflex-array-member-not-at-end]
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/en.h | 13 +++++++++----
>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 4 +++-
>> 2 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/
>> net/ethernet/mellanox/mlx5/core/en.h
>> index 979fc56205e1..c30c64eb346f 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
>> @@ -233,15 +233,20 @@ struct mlx5e_rx_wqe_cyc {
>> };
>> struct mlx5e_umr_wqe {
>> - struct mlx5_wqe_ctrl_seg ctrl;
>> - struct mlx5_wqe_umr_ctrl_seg uctrl;
>> - struct mlx5_mkey_seg mkc;
>> + /* New members MUST be added within the struct_group() macro
>> below. */
>> + struct_group_tagged(mlx5e_umr_wqe_hdr, hdr,
>> + struct mlx5_wqe_ctrl_seg ctrl;
>> + struct mlx5_wqe_umr_ctrl_seg uctrl;
>> + struct mlx5_mkey_seg mkc;
>> + );
>> union {
>> DECLARE_FLEX_ARRAY(struct mlx5_mtt, inline_mtts);
>> DECLARE_FLEX_ARRAY(struct mlx5_klm, inline_klms);
>> DECLARE_FLEX_ARRAY(struct mlx5_ksm, inline_ksms);
>> };
>> };
>> +static_assert(offsetof(struct mlx5e_umr_wqe, inline_mtts) ==
>> sizeof(struct mlx5e_umr_wqe_hdr),
>> + "struct member likely outside of struct_group_tagged()");
>> enum mlx5e_priv_flag {
>> MLX5E_PFLAG_RX_CQE_BASED_MODER,
>> @@ -660,7 +665,7 @@ struct mlx5e_rq {
>> } wqe;
>> struct {
>> struct mlx5_wq_ll wq;
>> - struct mlx5e_umr_wqe umr_wqe;
>> + struct mlx5e_umr_wqe_hdr umr_wqe;
>> struct mlx5e_mpw_info *info;
>> mlx5e_fp_skb_from_cqe_mpwrq skb_from_cqe_mpwrq;
>> __be32 umr_mkey_be;
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/
>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index bd41b75d246e..4ff4ff2342cf 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -373,6 +373,8 @@ static void mlx5e_rq_shampo_hd_info_free(struct
>> mlx5e_rq *rq)
>> static int mlx5e_rq_alloc_mpwqe_info(struct mlx5e_rq *rq, int node)
>> {
>> + struct mlx5e_umr_wqe *umr_wqe =
>> + container_of(&rq->mpwqe.umr_wqe, struct mlx5e_umr_wqe, hdr);
>> int wq_sz = mlx5_wq_ll_get_size(&rq->mpwqe.wq);
>> size_t alloc_size;
>> @@ -393,7 +395,7 @@ static int mlx5e_rq_alloc_mpwqe_info(struct
>> mlx5e_rq *rq, int node)
>> bitmap_fill(wi->skip_release_bitmap, rq->mpwqe.pages_per_wqe);
>> }
>> - mlx5e_build_umr_wqe(rq, rq->icosq, &rq->mpwqe.umr_wqe);
>> + mlx5e_build_umr_wqe(rq, rq->icosq, umr_wqe);
>> return 0;
>> }
>
> Here is another alternative for this. And similarly to the
> struct_group_tagged()
> change above, no struct members should be added before or after `struct
> mlx5e_umr_wqe_hdr hdr;` in `struct mlx5e_umr_wqe`:
>
Thanks for your patch.
The change with the struct_group_tagged() uses advanced tag, and keeps
code cleaner.
I prefer it.
Reviewed-by: Tariq Toukan <tariqt@...dia.com>
Regards,
Tariq
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/
> ethernet/mellanox/mlx5/core/en.h
> index 979fc56205e1..912b97eeb4d6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -232,10 +232,13 @@ struct mlx5e_rx_wqe_cyc {
> DECLARE_FLEX_ARRAY(struct mlx5_wqe_data_seg, data);
> };
>
> -struct mlx5e_umr_wqe {
> +struct mlx5e_umr_wqe_hdr {
> struct mlx5_wqe_ctrl_seg ctrl;
> struct mlx5_wqe_umr_ctrl_seg uctrl;
> struct mlx5_mkey_seg mkc;
> +};
> +struct mlx5e_umr_wqe {
> + struct mlx5e_umr_wqe_hdr hdr;
> union {
> DECLARE_FLEX_ARRAY(struct mlx5_mtt, inline_mtts);
> DECLARE_FLEX_ARRAY(struct mlx5_klm, inline_klms);
> @@ -660,7 +663,7 @@ struct mlx5e_rq {
> } wqe;
> struct {
> struct mlx5_wq_ll wq;
> - struct mlx5e_umr_wqe umr_wqe;
> + struct mlx5e_umr_wqe_hdr umr_wqe;
> struct mlx5e_mpw_info *info;
> mlx5e_fp_skb_from_cqe_mpwrq skb_from_cqe_mpwrq;
> __be32 umr_mkey_be;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c b/
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
> index 1b7132fa70de..2b05536d564a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
> @@ -123,7 +123,7 @@ int mlx5e_xsk_alloc_rx_mpwqe(struct mlx5e_rq *rq,
> u16 ix)
> bitmap_zero(wi->skip_release_bitmap, rq->mpwqe.pages_per_wqe);
> wi->consumed_strides = 0;
>
> - umr_wqe->ctrl.opmod_idx_opcode =
> + umr_wqe->hdr.ctrl.opmod_idx_opcode =
> cpu_to_be32((icosq->pc <<
> MLX5_WQE_CTRL_WQE_INDEX_SHIFT) | MLX5_OPCODE_UMR);
>
> /* Optimized for speed: keep in sync with
> mlx5e_mpwrq_umr_entry_size. */
> @@ -134,7 +134,7 @@ int mlx5e_xsk_alloc_rx_mpwqe(struct mlx5e_rq *rq,
> u16 ix)
> offset = offset * sizeof(struct mlx5_klm) * 2 /
> MLX5_OCTWORD;
> else if (unlikely(rq->mpwqe.umr_mode ==
> MLX5E_MPWRQ_UMR_MODE_TRIPLE))
> offset = offset * sizeof(struct mlx5_ksm) * 4 /
> MLX5_OCTWORD;
> - umr_wqe->uctrl.xlt_offset = cpu_to_be16(offset);
> + umr_wqe->hdr.uctrl.xlt_offset = cpu_to_be16(offset);
>
> icosq->db.wqe_info[pi] = (struct mlx5e_icosq_wqe_info) {
> .wqe_type = MLX5E_ICOSQ_WQE_UMR_RX,
> @@ -144,7 +144,7 @@ int mlx5e_xsk_alloc_rx_mpwqe(struct mlx5e_rq *rq,
> u16 ix)
>
> icosq->pc += rq->mpwqe.umr_wqebbs;
>
> - icosq->doorbell_cseg = &umr_wqe->ctrl;
> + icosq->doorbell_cseg = &umr_wqe->hdr.ctrl;
>
> return 0;
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index a814b63ed97e..bbd0b888d237 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -311,8 +311,8 @@ static inline void mlx5e_build_umr_wqe(struct
> mlx5e_rq *rq,
> struct mlx5e_icosq *sq,
> struct mlx5e_umr_wqe *wqe)
> {
> - struct mlx5_wqe_ctrl_seg *cseg = &wqe->ctrl;
> - struct mlx5_wqe_umr_ctrl_seg *ucseg = &wqe->uctrl;
> + struct mlx5_wqe_ctrl_seg *cseg = &wqe->hdr.ctrl;
> + struct mlx5_wqe_umr_ctrl_seg *ucseg = &wqe->hdr.uctrl;
> u16 octowords;
> u8 ds_cnt;
>
> @@ -393,7 +393,9 @@ static int mlx5e_rq_alloc_mpwqe_info(struct mlx5e_rq
> *rq, int node)
> bitmap_fill(wi->skip_release_bitmap, rq-
> >mpwqe.pages_per_wqe);
> }
>
> - mlx5e_build_umr_wqe(rq, rq->icosq, &rq->mpwqe.umr_wqe);
> + mlx5e_build_umr_wqe(rq, rq->icosq,
> + container_of(&rq->mpwqe.umr_wqe,
> + struct mlx5e_umr_wqe, hdr));
>
> return 0;
> }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/
> net/ethernet/mellanox/mlx5/core/en_rx.c
> index 1963bc5adb18..5fd70b4d55be 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -631,16 +631,16 @@ static void build_ksm_umr(struct mlx5e_icosq *sq,
> struct mlx5e_umr_wqe *umr_wqe,
> __be32 key, u16 offset, u16 ksm_len)
> {
> memset(umr_wqe, 0, offsetof(struct mlx5e_umr_wqe, inline_ksms));
> - umr_wqe->ctrl.opmod_idx_opcode =
> + umr_wqe->hdr.ctrl.opmod_idx_opcode =
> cpu_to_be32((sq->pc << MLX5_WQE_CTRL_WQE_INDEX_SHIFT) |
> MLX5_OPCODE_UMR);
> - umr_wqe->ctrl.umr_mkey = key;
> - umr_wqe->ctrl.qpn_ds = cpu_to_be32((sq->sqn <<
> MLX5_WQE_CTRL_QPN_SHIFT)
> + umr_wqe->hdr.ctrl.umr_mkey = key;
> + umr_wqe->hdr.ctrl.qpn_ds = cpu_to_be32((sq->sqn <<
> MLX5_WQE_CTRL_QPN_SHIFT)
> |
> MLX5E_KSM_UMR_DS_CNT(ksm_len));
> - umr_wqe->uctrl.flags = MLX5_UMR_TRANSLATION_OFFSET_EN |
> MLX5_UMR_INLINE;
> - umr_wqe->uctrl.xlt_offset = cpu_to_be16(offset);
> - umr_wqe->uctrl.xlt_octowords = cpu_to_be16(ksm_len);
> - umr_wqe->uctrl.mkey_mask = cpu_to_be64(MLX5_MKEY_MASK_FREE);
> + umr_wqe->hdr.uctrl.flags = MLX5_UMR_TRANSLATION_OFFSET_EN |
> MLX5_UMR_INLINE;
> + umr_wqe->hdr.uctrl.xlt_offset = cpu_to_be16(offset);
> + umr_wqe->hdr.uctrl.xlt_octowords = cpu_to_be16(ksm_len);
> + umr_wqe->hdr.uctrl.mkey_mask =
> cpu_to_be64(MLX5_MKEY_MASK_FREE);
> }
>
> static struct mlx5e_frag_page *mlx5e_shampo_hd_to_frag_page(struct
> mlx5e_rq *rq, int header_index)
> @@ -704,7 +704,7 @@ static int mlx5e_build_shampo_hd_umr(struct mlx5e_rq
> *rq,
>
> shampo->pi = (shampo->pi + ksm_entries) & (shampo->hd_per_wq - 1);
> sq->pc += wqe_bbs;
> - sq->doorbell_cseg = &umr_wqe->ctrl;
> + sq->doorbell_cseg = &umr_wqe->hdr.ctrl;
>
> return 0;
>
> @@ -814,12 +814,12 @@ static int mlx5e_alloc_rx_mpwqe(struct mlx5e_rq
> *rq, u16 ix)
> bitmap_zero(wi->skip_release_bitmap, rq->mpwqe.pages_per_wqe);
> wi->consumed_strides = 0;
>
> - umr_wqe->ctrl.opmod_idx_opcode =
> + umr_wqe->hdr.ctrl.opmod_idx_opcode =
> cpu_to_be32((sq->pc << MLX5_WQE_CTRL_WQE_INDEX_SHIFT) |
> MLX5_OPCODE_UMR);
>
> offset = (ix * rq->mpwqe.mtts_per_wqe) * sizeof(struct
> mlx5_mtt) / MLX5_OCTWORD;
> - umr_wqe->uctrl.xlt_offset = cpu_to_be16(offset);
> + umr_wqe->hdr.uctrl.xlt_offset = cpu_to_be16(offset);
>
> sq->db.wqe_info[pi] = (struct mlx5e_icosq_wqe_info) {
> .wqe_type = MLX5E_ICOSQ_WQE_UMR_RX,
> @@ -829,7 +829,7 @@ static int mlx5e_alloc_rx_mpwqe(struct mlx5e_rq *rq,
> u16 ix)
>
> sq->pc += rq->mpwqe.umr_wqebbs;
>
> - sq->doorbell_cseg = &umr_wqe->ctrl;
> + sq->doorbell_cseg = &umr_wqe->hdr.ctrl;
>
> return 0;
>
>
> Thanks
> --
> Gustavo
>
Powered by blists - more mailing lists