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: <09DF4DE0-1694-4B06-84A9-A0E1DA1B3109@gmail.com>
Date: Thu, 24 Oct 2024 00:46:32 +0800
From: Alan Huang <mmpgouride@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...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 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.

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