2023-04-28 11:11:10

by Viacheslav Mitrofanov

Subject: [PATCH 0/1] Limit the number of counter returned from SBI.

Perf relies on reliability of SBI. If sth goes wrong the code trusts it.
It happened due to some debug process that I passed more than
RISCV_MAX_COUNTERS to perf from SBI. At the first glance there were
bloating of kalloced variable pmu_ctr_list and counter mask recycle write.
May be there were some other effects. But anyway it is better to add
extra check.

Viacheslav Mitrofanov (1):
perf: RISC-V: Limit the number of counters returned from SBI.

drivers/perf/riscv_pmu_sbi.c | 5 +++++
1 file changed, 5 insertions(+)

--
2.37.2

2023-04-28 11:11:10

by Viacheslav Mitrofanov

Subject: [PATCH 1/1] perf: RISC-V: Limit the number of counters returned from SBI.

Perf gets the number of supported counters from SBI. If it happens that
the number of returned counters more than RISCV_MAX_COUNTERS the code
trusts it. It does not lead to an immediate problem but can potentially
lead to it. Prevent getting more than RISCV_MAX_COUNTERS from SBI.

Signed-off-by: Viacheslav Mitrofanov <[email protected]>
---
drivers/perf/riscv_pmu_sbi.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 70cb50fd41c2..0183bf911bfb 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -867,6 +867,11 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
pr_err("SBI PMU extension doesn't provide any counters\n");
goto out_free;
}
+ /* It is possible to get from SBI more than max number of counters */
+ if (num_counters > RISCV_MAX_COUNTERS) {
+ pr_warn("SBI returned more than maximum number of counters\n");
+ num_counters = RISCV_MAX_COUNTERS;
+ }

/* cache all the information about counters now */
if (pmu_sbi_get_ctrinfo(num_counters, &cmask))
--
2.37.2

2023-04-28 12:21:27

by Andrew Jones

Subject: Re: [PATCH 1/1] perf: RISC-V: Limit the number of counters returned from SBI.

On Fri, Apr 28, 2023 at 11:02:56AM +0000, Viacheslav Mitrofanov wrote:
> Perf gets the number of supported counters from SBI. If it happens that
> the number of returned counters more than RISCV_MAX_COUNTERS the code
> trusts it. It does not lead to an immediate problem but can potentially
> lead to it. Prevent getting more than RISCV_MAX_COUNTERS from SBI.

I recall suggesting we do this during the KVM PMU review, but I guess we
forgot.

>
> Signed-off-by: Viacheslav Mitrofanov <[email protected]>
> ---
> drivers/perf/riscv_pmu_sbi.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 70cb50fd41c2..0183bf911bfb 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -867,6 +867,11 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
> pr_err("SBI PMU extension doesn't provide any counters\n");
> goto out_free;
> }

Need blank line here

> + /* It is possible to get from SBI more than max number of counters */
> + if (num_counters > RISCV_MAX_COUNTERS) {
> + pr_warn("SBI returned more than maximum number of counters\n");
^ the

This should be a pr_info.


> + num_counters = RISCV_MAX_COUNTERS;
> + }

Otherwise,

Reviewed-by: Andrew Jones <[email protected]>

Thanks,
drew