linux-kernel - Re: [PATCH 03/19] perf: Fix perf_pmu_register() vs perf_init

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: <20241105120143.GD10375@noisy.programming.kicks-ass.net>
Date: Tue, 5 Nov 2024 13:01:43 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Uros Bizjak <ubizjak@...il.com>
Cc: mingo@...nel.org, lucas.demarchi@...el.com,
	linux-kernel@...r.kernel.org, willy@...radead.org, acme@...nel.org,
	namhyung@...nel.org, mark.rutland@....com,
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com
Subject: Re: [PATCH 03/19] perf: Fix perf_pmu_register() vs perf_init_event()

On Mon, Nov 04, 2024 at 04:36:26PM +0100, Uros Bizjak wrote:
> 
> 
> On 4. 11. 24 14:39, Peter Zijlstra wrote:
> > There is a fairly obvious race between perf_init_event() doing
> > idr_find() and perf_pmu_register() doing idr_alloc() with an
> > incompletely initialized pmu pointer.
> > 
> > Avoid by doing idr_alloc() on a NULL pointer to register the id, and
> > swizzling the real pmu pointer at the end using idr_replace().
> > 
> > Also making sure to not set pmu members after publishing the pmu, duh.
> > 
> > [ introduce idr_cmpxchg() in order to better handle the idr_replace()
> >    error case -- if it were to return an unexpected pointer, it will
> >    already have replaced the value and there is no going back. ]
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> > ---
> >   kernel/events/core.c |   28 ++++++++++++++++++++++++++--
> >   1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -11739,6 +11739,21 @@ static int pmu_dev_alloc(struct pmu *pmu
> >   static struct lock_class_key cpuctx_mutex;
> >   static struct lock_class_key cpuctx_lock;
> > +static bool idr_cmpxchg(struct idr *idr, unsigned long id, void *old, void *new)
> > +{
> > +	void *tmp, *val = idr_find(idr, id);
> > +
> > +	if (val != old)
> > +		return false;
> > +
> > +	tmp = idr_replace(idr, new, id);
> > +	if (IS_ERR(tmp))
> > +		return false;
> > +
> > +	WARN_ON_ONCE(tmp != val);
> > +	return true;
> > +}
> 
> Can the above function be named idr_try_cmpxchg?
> 
> cmpxchg family of functions return an old value from the location and one
> would expect that idr_cmpxchg() returns an old value from *idr, too.
> idr_cmpxchg() function however returns success/failure status, and this is
> also what functions from try_cmpxchg family return.

Fair enough -- OTOH, this function is very much not atomic. I considered
calling it idr_cas() as to distance itself from cmpxchg family.

Also, it is local to perf, and not placed in idr.h or similar.

While the usage here is somewhat spurious, it gets used later on in the
series to better effect.