| 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: <217b00f5-d03d-4624-9ba9-d838199ef7b9@embeddedor.com>
Date: Tue, 15 Apr 2025 18:30:20 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
Kenneth Feng <kenneth.feng@....com>, Alex Deucher
<alexander.deucher@....com>, Christian König
<christian.koenig@....com>, Xinhui Pan <Xinhui.Pan@....com>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>
Cc: amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH][next] drm/amd/pm: Avoid multiple
-Wflex-array-member-not-at-end warnings
Hi all,
Friendly ping (second one): who can take this patch, please? 🙂
Thanks!
-Gustavo
On 11/03/25 02:10, Gustavo A. R. Silva wrote:
> Hi all,
>
> Friendly ping: who can take this, please? :)
>
> Thanks!
> --
> Gustavo
>
> On 14/02/25 18:48, 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 NISLANDS_SMC_SWSTATE_HDR` (and `struct
>> SISLANDS_SMC_SWSTATE_HDR`). This structures group together all the
>> members of the flexible `struct NISLANDS_SMC_SWSTATE` (and `struct
>> SISLANDS_SMC_SWSTATE`) 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 members currently causing
>> trouble from `struct NISLANDS_SMC_SWSTATE` to `struct
>> NISLANDS_SMC_SWSTATE_HDR` (and from `struct SISLANDS_SMC_SWSTATE` to
>> `struct SISLANDS_SMC_SWSTATE_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 NISLANDS_SMC_SWSTATE_HDR`
>> (and `struct SISLANDS_SMC_SWSTATE_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 this changes, fix the following warnings:
>>
>> drivers/gpu/drm/amd/amdgpu/../pm/legacy-dpm/sislands_smc.h:218:49: warning: structure containing a flexible array member is not at the end of another
>> structure [-Wflex-array-member-not-at-end]
>> drivers/gpu/drm/amd/amdgpu/../pm/legacy-dpm/si_dpm.h:819:41: warning: structure containing a flexible array member is not at the end of another structure [-
>> Wflex-array-member-not-at-end]
>> drivers/gpu/drm/amd/amdgpu/../pm/legacy-dpm/si_dpm.h:818:41: warning: structure containing a flexible array member is not at the end of another structure [-
>> Wflex-array-member-not-at-end]
>> drivers/gpu/drm/amd/amdgpu/../pm/legacy-dpm/si_dpm.h:817:41: warning: structure containing a flexible array member is not at the end of another structure [-
>> Wflex-array-member-not-at-end]
>> drivers/gpu/drm/amd/amdgpu/../pm/legacy-dpm/si_dpm.h:816:41: 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/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 7 ++++--
>> drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.h | 23 +++++++++++--------
>> .../gpu/drm/amd/pm/legacy-dpm/sislands_smc.h | 15 ++++++++----
>> 3 files changed, 29 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
>> index a87dcf0974bc..2c9d473d122f 100644
>> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
>> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
>> @@ -5234,7 +5234,8 @@ static int si_init_smc_table(struct amdgpu_device *adev)
>> table->driverState.flags = table->initialState.flags;
>> table->driverState.levelCount = table->initialState.levelCount;
>> - table->driverState.levels[0] = table->initialState.level;
>> + container_of(&table->driverState, SISLANDS_SMC_SWSTATE, __hdr)->levels[0] =
>> + table->initialState.level;
>> ret = si_do_program_memory_timing_parameters(adev, amdgpu_boot_state,
>> SISLANDS_INITIAL_STATE_ARB_INDEX);
>> @@ -5755,7 +5756,9 @@ static int si_upload_sw_state(struct amdgpu_device *adev,
>> int ret;
>> u32 address = si_pi->state_table_start +
>> offsetof(SISLANDS_SMC_STATETABLE, driverState);
>> - SISLANDS_SMC_SWSTATE *smc_state = &si_pi->smc_statetable.driverState;
>> + SISLANDS_SMC_SWSTATE *smc_state =
>> + container_of(&si_pi->smc_statetable.driverState,
>> + SISLANDS_SMC_SWSTATE, __hdr);
>> size_t state_size = struct_size(smc_state, levels,
>> new_state->performance_level_count);
>> memset(smc_state, 0, state_size);
>> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.h b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.h
>> index 11cb7874a6bb..62530f89ebdf 100644
>> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.h
>> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.h
>> @@ -784,12 +784,17 @@ typedef struct NISLANDS_SMC_HW_PERFORMANCE_LEVEL NISLANDS_SMC_HW_PERFORMANCE_LEV
>> struct NISLANDS_SMC_SWSTATE
>> {
>> - uint8_t flags;
>> - uint8_t levelCount;
>> - uint8_t padding2;
>> - uint8_t padding3;
>> - NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
>> + /* New members MUST be added within the struct_group() macro below. */
>> + struct_group_tagged(NISLANDS_SMC_SWSTATE_HDR, __hdr,
>> + uint8_t flags;
>> + uint8_t levelCount;
>> + uint8_t padding2;
>> + uint8_t padding3;
>> + );
>> + NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
>> };
>> +static_assert(offsetof(struct NISLANDS_SMC_SWSTATE, levels) == sizeof(struct NISLANDS_SMC_SWSTATE_HDR),
>> + "struct member likely outside of struct_group_tagged()");
>> typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
>> @@ -813,10 +818,10 @@ struct NISLANDS_SMC_STATETABLE
>> uint32_t lowSMIO[NISLANDS_MAX_NO_VREG_STEPS];
>> NISLANDS_SMC_VOLTAGEMASKTABLE voltageMaskTable;
>> PP_NIslands_DPM2Parameters dpm2Params;
>> - NISLANDS_SMC_SWSTATE initialState;
>> - NISLANDS_SMC_SWSTATE ACPIState;
>> - NISLANDS_SMC_SWSTATE ULVState;
>> - NISLANDS_SMC_SWSTATE driverState;
>> + struct NISLANDS_SMC_SWSTATE_HDR initialState;
>> + struct NISLANDS_SMC_SWSTATE_HDR ACPIState;
>> + struct NISLANDS_SMC_SWSTATE_HDR ULVState;
>> + struct NISLANDS_SMC_SWSTATE_HDR driverState;
>> NISLANDS_SMC_HW_PERFORMANCE_LEVEL dpmLevels[NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1];
>> };
>> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/sislands_smc.h b/drivers/gpu/drm/amd/pm/legacy-dpm/sislands_smc.h
>> index 90ec411c5029..1711e3e35e80 100644
>> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/sislands_smc.h
>> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/sislands_smc.h
>> @@ -172,12 +172,17 @@ struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL {
>> typedef struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL SISLANDS_SMC_HW_PERFORMANCE_LEVEL;
>> struct SISLANDS_SMC_SWSTATE {
>> - uint8_t flags;
>> - uint8_t levelCount;
>> - uint8_t padding2;
>> - uint8_t padding3;
>> + /* New members MUST be added within the struct_group() macro below. */
>> + struct_group_tagged(SISLANDS_SMC_SWSTATE_HDR, __hdr,
>> + uint8_t flags;
>> + uint8_t levelCount;
>> + uint8_t padding2;
>> + uint8_t padding3;
>> + );
>> SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
>> };
>> +static_assert(offsetof(struct SISLANDS_SMC_SWSTATE, levels) == sizeof(struct SISLANDS_SMC_SWSTATE_HDR),
>> + "struct member likely outside of struct_group_tagged()");
>> typedef struct SISLANDS_SMC_SWSTATE SISLANDS_SMC_SWSTATE;
>> @@ -215,7 +220,7 @@ struct SISLANDS_SMC_STATETABLE {
>> struct SISLANDS_SMC_SWSTATE_SINGLE initialState;
>> struct SISLANDS_SMC_SWSTATE_SINGLE ACPIState;
>> struct SISLANDS_SMC_SWSTATE_SINGLE ULVState;
>> - SISLANDS_SMC_SWSTATE driverState;
>> + struct SISLANDS_SMC_SWSTATE_HDR driverState;
>> SISLANDS_SMC_HW_PERFORMANCE_LEVEL dpmLevels[SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE];
>> };
>
Powered by blists - more mailing lists