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

Message-ID: <CAEf4BzaqXhq3q5kXSAgH7H3tibC3wFU7R9zr5fcwcddT-r6wUw@mail.gmail.com>
Date: Wed, 23 Oct 2024 09:34:26 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: paulmck@...nel.org, Peter Zijlstra <peterz@...radead.org>, rcu@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kernel-team@...a.com, rostedt@...dmis.org, 
	andrii@...nel.org
Subject: Re: [PATCH rcu] srcu: Guarantee non-negative return value from srcu_read_lock()

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.

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