linux-kernel - Re: [RFC PATCH 06/36] ACPI / PPTT: Stop acpi_count_levels() expecting callers to clear levels

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]

Message-ID: <1422ff0e-6aa3-42de-ba12-4dff32e7505d@arm.com>
Date: Fri, 25 Jul 2025 18:05:50 +0100
From: James Morse <james.morse@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 Rob Herring <robh@...nel.org>, Ben Horgan <ben.horgan@....com>,
 Rohit Mathew <rohit.mathew@....com>,
 Shanker Donthineni <sdonthineni@...dia.com>, Zeng Heng
 <zengheng4@...wei.com>, Lecopzer Chen <lecopzerc@...dia.com>,
 Carl Worth <carl@...amperecomputing.com>,
 shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
 Sudeep Holla <sudeep.holla@....com>
Subject: Re: [RFC PATCH 06/36] ACPI / PPTT: Stop acpi_count_levels() expecting
 callers to clear levels

Hi Jonathan,

On 16/07/2025 16:51, Jonathan Cameron wrote:
> On Fri, 11 Jul 2025 18:36:18 +0000
> James Morse <james.morse@....com> wrote:
> 
>> acpi_count_levels() passes the number of levels back via a pointer argument.
>> It also passes this to acpi_find_cache_level() as the starting_level, and
>> preserves this value as it walks up the cpu_node tree counting the levels.
>>
>> The only caller acpi_get_cache_info() happens to have already initialised
>> levels to zero, which acpi_count_levels() depends on to get the correct
>> result.
>>
>> Explicitly zero the levels variable, so the count always starts at zero.
>> This saves any additional callers having to work out they need to do this.

> This is all a bit fiddly as we now end up with that initialized in various
> different places.

I've debugged this one a few times, (turns out I'm forgetful) ... I figured doing this
was better than adding a comment to warn others.
As its static, I figured it was something the compiler can optimise out if there is a
duplicate. (I couldn't find any initialisation I could remove because of this)


> Perhaps simpler to have acpi_count_levels() return the
> number of levels rather than void. Then return number of levels rather
> than 0 on success from acpi_get_cache_info(). Negative error codes used
> for failure just like now.
> 
> That would leave only a local variable in acpi_count_levels being
> initialized to 0 and passed to acpi_find_cache_level() before being
> returned when the loop terminates.
> 
> I think that sequence then makes it such that we can't fail to
> initialize it at without the compiler noticing and screaming.
> 
> Requires a few changes from if (ret) to if (ret < 0) at callers
> of acpi_get_cache_info() but looks simple (says the person who
> hasn't actually coded it!)

Breaking the symmetry between levels and split_levels is an argument against this.

I think within pptt.c this is fine, because 'level's is used internally as
'starting_level', and this expectation it was initialised to zero is a nasty surprise.

But exposing that from acpi_get_cache_info() looks stranger - and would need to touch
users in cacheinfo, arm64, riscv.

I've updated acpi_count_levels() to look as you describe - that at least makes it harder
to miss this in future. (not sure whether it saves anything)


>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 13619b1b821b..13ca2eee3b98 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -183,7 +183,7 @@ acpi_find_cache_level(struct acpi_table_header *table_hdr,
>>   * @cpu_node: processor node we wish to count caches for
>>   * @levels: Number of levels if success.
>>   * @split_levels:	Number of split cache levels (data/instruction) if
>> - *			success. Can by NULL.
>> + *			success. Can be NULL.
> 
> Grumpy reviewer hat.  Unrelated cleanup up - good to have but not in this patch where
> it's a distraction.

I was hoping diff would keep it as one hunk. Happy to leave it tyopd!


Thanks,

James