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: <CAEf4BzbyctiXq8L5MQmCtVqGSN8uawUmNXJMm-X8jDcp8QQ86g@mail.gmail.com>
Date: Tue, 22 Oct 2024 10:29:13 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: paulmck@...nel.org
Cc: Christoph Hellwig <hch@...radead.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 7:26 AM Paul E. McKenney <paulmck@...nel.org> wrote:
>
> On Tue, Oct 22, 2024 at 12:13:12AM -0700, Christoph Hellwig wrote:
> > On Tue, Oct 22, 2024 at 09:10:18AM +0200, Peter Zijlstra wrote:
> > > Ah, well, the thing that got us here is that we (Andrii and me) wanted
> > > to use -1 as an 'invalid' value to indicate SRCU is not currently in
> > > use.
> > >
> > > So it all being int is really rather convenient :-)
> >
> > Then please document that use.  Maybe even with a symolic name for
> > -1 that clearly describes these uses.
>
> Would this work?
>
> #define SRCU_INVALID_INDEX -1
>

But why? 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.

Why are we complicating something that's simple and straightforward?
It's similar with any fd- and id- returning API.

Marking it as u16 would be fine, but unusual (and probably suboptimal
due to common u16 to int conversion).

Marking it as unsigned int would be bad, because it implies that all
32 bits can be used for valid value, making it impossible to use <0
for something else.

Just documenting that valid int values are always >= is good and
convenient, why not sticking to just that?

> Whatever the name, maybe Peter and Andrii define this under #ifndef
> right now, and we get it into include/linux/srcu.h over time.
>
> Or is there a better way?  Or name, for that matter.
>
>                                                         Thanx, Paul