linux-kernel - Re: [PATCH V2] arm64/cpufeature: Add get_arm64_ftr_reg

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 May 2020 19:33:29 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Suzuki K Poulose <suzuki.poulose@....com>,
        linux-arm-kernel@...ts.infradead.org
Cc:     mark.rutland@....com, catalin.marinas@....com, will@...nel.org,
        broonie@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()



On 05/26/2020 07:34 PM, Suzuki K Poulose wrote:
> On 05/26/2020 02:39 PM, Anshuman Khandual wrote:
>> There is no way to proceed when requested register could not be searched in
>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
>> well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg()
>> rather than checking for return value and doing a BUG_ON() instead in some
>> individual callers. But there are also caller instances that dont error out
>> when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for
>> such cases.
>>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@....com>
>> Cc: Mark Brown <broonie@...nel.org>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>> Changes in V2:
>>
>> - Added get_arm64_ftr_reg_nowarn() per Will
>> - read_sanitised_ftr_reg() returns 0 when register search fails per Catalin
>>
>> Changes in V1: (https://patchwork.kernel.org/patch/11559083/)
>>
>>   arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++-----------
>>   1 file changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index bc5048f152c1..f4555b9d145c 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
>>   }
>>   
> 
> ...
> 
>>   static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
>>                      s64 ftr_val)
>>   {
>> @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
>>       const struct arm64_ftr_bits *ftrp;
>>       struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>>   -    BUG_ON(!reg);
>> -
>>       for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>>           u64 ftr_mask = arm64_ftr_mask(ftrp);
>>           s64 ftr_new = arm64_ftr_value(ftrp, new);
>> @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
>>   {
>>       struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>>   -    BUG_ON(!regp);
>>       update_cpu_ftr_reg(regp, val);
>>       if ((boot & regp->strict_mask) == (val & regp->strict_mask))
>>           return 0;
>> @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field)
>>       const struct arm64_ftr_bits *ftrp;
>>       struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>>   -    if (WARN_ON(!regp))
>> -        return;
>> -
> 
> You need to return here, on !regp. Rest looks fine to me.

Catalin had suggested and agreed on for this change in behavior here.
If the register is not found, there is already some problem.