Re: [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array

From: Sergey Matyukevich <geomatsi@gmail.com>
To: Conor.Dooley@microchip.com
Cc: linux-riscv@lists.infradead.org, atishp@atishpatra.org,
	mark.rutland@arm.com, will@kernel.org, anup@brainfault.org,
	aou@eecs.berkeley.edu, palmer@dabbelt.com,
	paul.walmsley@sifive.com, sergey.matyukevich@syntacore.com,
	atishp@rivosinc.com
Subject: Re: [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array
Date: Mon, 29 Aug 2022 00:31:10 +0300	[thread overview]
Message-ID: <YwvenvGgTiJAy69W@curiosity> (raw)
In-Reply-To: <70656978-546f-c14d-96a9-8dc7d8dcf9aa@microchip.com>

> > [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array
>                                ^^^ (see below)
> > 
> > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> > 
> > The root cause could be related to the interpretation of the number of
> > counters reported by SBI firmware. For instance, if we assume that unused
> > timer counter with index 1 is not reported, then the range is correct
> > and larger array needs to be allocated.
> 
> I found this to be confusingly worded, had to read it a few times before
> I understood what you meant. Maybe it's just late on a Friday, I think the
> theorycrafting about why the code looks how it does got me lol

Well, it was not exactly theorycrafting, see accompanying opensbi changes:
http://lists.infradead.org/pipermail/opensbi/2022-June/002926.html
I tried to provide some context for this one-liner, but failed to do so.

> > This is not the case though since SBI firmware is supposed to report the
> > total number of firmware and hardware counters including special or
> > unused ones like the timer counter. So just fix the range in for-loop.
>                                               ^^^
> I see "fix" mentioned twice here, what commmit does it fix?

That is the initial commit adding the whole SBI PMU driver, so it is a
bit confusing to add 'Fixes' in this case. I think the following commit
message should be sufficient:

SBI firmware should report total number of firmware and hardware counters
including unused ones or special ones. In this case the kernel doesn't need
to make any assumptions about gaps in reported counters, e.g. excluded timer
counter. That was fixed in OpenSBI v1.1 by commit 3f66465fb6bf ("lib: pmu:
allow to use the highest available counter"). This kernel patch has no effect
if SBI firmware behaves correctly. However it eliminates access beyond the
allocated pmu_ctr_list if the kernel is used with OpenSBI older than v1.1.

Regards,
Sergey

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv