linux-kernel - Re: [PATCH v2 tip/perf/core 2/2] uprobes: SRCU-protect uretprobe lifetime (with timeout)

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: <20241021104815.GC6791@noisy.programming.kicks-ass.net>
Date: Mon, 21 Oct 2024 12:48:15 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org,
	oleg@...hat.com, rostedt@...dmis.org, mhiramat@...nel.org,
	mingo@...nel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
	jolsa@...nel.org, paulmck@...nel.org
Subject: Re: [PATCH v2 tip/perf/core 2/2] uprobes: SRCU-protect uretprobe
 lifetime (with timeout)

On Fri, Oct 18, 2024 at 11:22:09AM -0700, Andrii Nakryiko wrote:

> > So... after a few readings I think I'm mostly okay with this. But I got
> > annoyed by the whole HPROBE_STABLE with uprobe=NULL weirdness. Also,
> > that data_race() usage is weird, what is that about?
> 
> People keep saying that evil KCSAN will come after me if I don't add
> data_race() for values that can change under me, so I add it to make
> it explicit that it's fine. But I can of course just drop data_race(),
> as it has no bearing on correctness.

AFAICT this was READ_ONCE() vs xchg(), and that should work. Otherwise I
have to yell at KCSAN people again :-)

> > And then there's the case where we end up doing:
> >
> >   try_get_uprobe()
> >   put_uprobe()
> >   try_get_uprobe()
> >
> > in the dup path. Yes, it's unlikely, but gah.
> >
> >
> > So how about something like this?
> 
> Yep, it makes sense to start with HPROBE_GONE if it's already NULL, no
> problem. I'll roll those changes in.
> 
> I'm fine with the `bool get` flag as well. Will incorporate all that
> into the next revision, thanks!
> 
> The only problem I can see is in the assumption that `srcu_idx < 0` is
> never going to be returned by srcu_read_lock(). Paul says that it can
> only be 0 or 1, but it's not codified as part of a contract.

Yeah, [0,1] is the current range. Fundamentally that thing is an array
index, so negative values are out and generally safe to use as 'error'
codes. Paul can't we simply document that the SRCU cookie is always a
positive integer (or zero) and the negative space shall not be used?

> So until we change that, probably safer to pass an extra bool
> specifying whether srcu_idx is valid or not, is that OK?

I think Changeing the SRCU documentation to provide us this guarantee
should be an achievable goal.

> (and I assume you want me to drop verbose comments for various states, right?)

I axed the comments because I made them invalid and didn't care enough
to fix them up. If you like them feel free to amend them to reflect the
new state of things.