linux-kernel - Re: [PATCH 02/19] perf/hisilicon: Fix group validation

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: <aK20GP5g1iu9DGrQ@J2N7QTR9R3>
Date: Tue, 26 Aug 2025 14:18:16 +0100
From: Mark Rutland <mark.rutland@....com>
To: Robin Murphy <robin.murphy@....com>
Cc: peterz@...radead.org, mingo@...hat.com, will@...nel.org,
	acme@...nel.org, namhyung@...nel.org,
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, linux-perf-users@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-alpha@...r.kernel.org,
	linux-snps-arc@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev,
	linux-csky@...r.kernel.org, loongarch@...ts.linux.dev,
	linux-mips@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	linux-s390@...r.kernel.org, linux-sh@...r.kernel.org,
	sparclinux@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-rockchip@...ts.infradead.org, dmaengine@...r.kernel.org,
	linux-fpga@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
	dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org,
	intel-xe@...ts.freedesktop.org, coresight@...ts.linaro.org,
	iommu@...ts.linux.dev, linux-amlogic@...ts.infradead.org,
	linux-cxl@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	linux-riscv@...ts.infradead.org
Subject: Re: [PATCH 02/19] perf/hisilicon: Fix group validation

On Tue, Aug 26, 2025 at 12:15:23PM +0100, Mark Rutland wrote:
> On Wed, Aug 13, 2025 at 06:00:54PM +0100, Robin Murphy wrote:
> > The group validation logic shared by the HiSilicon HNS3/PCIe drivers is
> > a bit off, in that given a software group leader, it will consider that
> > event *in place of* the actual new event being opened. At worst this
> > could theoretically allow an unschedulable group if the software event
> > config happens to look like one of the hardware siblings.
> > 
> > The uncore framework avoids that particular issue,
> 
> What is "the uncore framework"? I'm not sure exactly what you're
> referring to, nor how that composes with the problem described above.
> 
> > but all 3 also share the common issue of not preventing racy access to
> > the sibling list,
> 
> Can you please elaborate on this racy access to the silbing list? I'm
> not sure exactly what you're referring to.

Ah, I think you're referring to the issue in:

  https://lore.kernel.org/linux-arm-kernel/Zg0l642PgQ7T3a8Z@FVFF77S0Q05N/

... where when creatign a new event which is its own group leader,
lockdep_assert_event_ctx(event) fires in for_each_sibling_event(),
because the new event's context isn't locked...

> > diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> > index a449651f79c9..3c531b36cf25 100644
> > --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
> > +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> > @@ -101,26 +101,17 @@ static bool hisi_validate_event_group(struct perf_event *event)
> >  	/* Include count for the event */
> >  	int counters = 1;
> >  
> > -	if (!is_software_event(leader)) {
> > -		/*
> > -		 * We must NOT create groups containing mixed PMUs, although
> > -		 * software events are acceptable
> > -		 */
> > -		if (leader->pmu != event->pmu)
> > -			return false;
> > +	if (leader == event)
> > +		return true;

... and hence bailing out here avoids that?

It's not strictly "racy access to the sibling list", becuase there's
nothing else accessing the list; it's just that this is the simplest way
to appease lockdep while avoiding false negatives.

It'd probably be better to say something like "the common issue of
calling for_each_sibling_event() when initialising a new group leader",
and maybe to spell that out a bit.

Mark.