linux-kernel - Re: [PATCH rcu] srcu: Guarantee non-negative return value from srcu_read

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] [day] [month] [year] [list]

Message-ID: <CAEf4BzZDWy9=WPV4Xb4CjPuNzEokqR2Md1HjAARQjFmXzMhYJA@mail.gmail.com>
Date: Wed, 23 Oct 2024 09:59:56 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Alan Huang <mmpgouride@...il.com>
Cc: Christoph Hellwig <hch@...radead.org>, "Paul E. McKenney" <paulmck@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, RCU <rcu@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, kernel-team@...a.com, 
	Steven Rostedt <rostedt@...dmis.org>, andrii@...nel.org
Subject: Re: [PATCH rcu] srcu: Guarantee non-negative return value from srcu_read_lock()

On Wed, Oct 23, 2024 at 9:46 AM Alan Huang <mmpgouride@...il.com> wrote:
>
> On Oct 24, 2024, at 00:34, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >
> > On Tue, Oct 22, 2024 at 11:40 PM Christoph Hellwig <hch@...radead.org> wrote:
> >>
> >> On Tue, Oct 22, 2024 at 10:29:13AM -0700, Andrii Nakryiko wrote:
> >>>>
> >>>> Would this work?
> >>>>
> >>>> #define SRCU_INVALID_INDEX -1
> >>>>
> >>>
> >>> But why?
> >>
> >> Becaue it very clearly documents what is going on.
> >
> > So does saying "returned value is going to be non-negative, always".
> > It's not some weird and unusual thing in C by any means.
> >
> >>
> >>> It's a nice property to have an int-returning API where valid
> >>> values are only >= 0, so callers are free to use the entire negative
> >>> range (not just -1) for whatever they need to store in case there is
> >>> no srcu_idx value.
> >>
> >> Well, if you have a concrete use case for that we can probably live
> >> with it, but I'd rather have that use case extremely well documented,
> >> as it will be very puzzling to the reader.
> >>
> >
> > See [0] for what Peter is proposing. Note hprobe_init() and its use of
> > `srcu_idx < 0` as a mark that there is no SRCU "session" associated
> > with the uprobe. Now, if we say that SRCU_INVALID_INDEX is the only
> > invalid value, it leaves a question: what to do with other negative
> > srcu_idx values? Are they valid? Should I now WARN() on "unknown
> > invalid" values? Why all these complications? I'd rather just not have
> > a unified hprobe_init() at that point, TBH.
> >
> > But anyways, taking a step back. Take idr_alloc()/idr_alloc_cyclic()
> > APIs as an example. They return int, but valid IDs are documented to
> > be positive. This leaves users of this API free to use int to store ID
> > in their data structures, knowing that <= 0 is "no ID", and if
> > necessary, they can have multiple possible "no ID" situations.
> >
> > Same principle here. Why prescribing a randomly picked -1 as the only
> > "valid" invalid value, if anything negative is clearly impossible.
> >
>
> A IS_INVALID_SRCU_INDEX macro would suffice, you need the condition anyway.

That condition is `if (srcu_idx < 0)`, no need for ugly macros that
obscure things unnecessarily.

>
> >
> >  [0] https://lore.kernel.org/linux-trace-kernel/20241018101647.GA36494@noisy.programming.kicks-ass.net/
>
>