linux-kernel - Re: [PATCH] revocable: hide the implementation details from users

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: <aYvj5h4tEZI-habX@google.com>
Date: Wed, 11 Feb 2026 02:05:26 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Bartosz Golaszewski <brgl@...nel.org>
Cc: paulmck@...nel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>, Shuah Khan <shuah@...nel.org>,
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
	Johan Hovold <johan@...nel.org>, linux-kernel@...r.kernel.org,
	driver-core@...ts.linux.dev, linux-kselftest@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>
Subject: Re: [PATCH] revocable: hide the implementation details from users

On Mon, Feb 09, 2026 at 05:13:17AM -0600, Bartosz Golaszewski wrote:
> On Fri, 6 Feb 2026 23:02:44 +0100, "Paul E. McKenney" <paulmck@...nel.org> said:
> > On Fri, Feb 06, 2026 at 11:32:06AM +0100, Bartosz Golaszewski wrote:
> >> Revocable stacks two layers of SRCU on top of each other: one to protect
> >> the actual revocable resource and another to synchronize the revoking.
> >> While this design itself is questionable, it also forces the user of
> >> revokable to think about the implementation details and annotate the
> >> pointer holding the address of the revocable_provider struct with __rcu.
> >> Hide the real type of struct revokable_provider behind a typedef to free
> >> the users from this responsability. While adding new typedefs goes
> >> against current guidelines, it's still better than the current
> >> requirement.
> >>
> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>
> >> ---
> >> I realized that one important person was missing from the whole review
> >> process: Paul E. McKenney who wrote and maintains SRCU. I had Paul look
> >> at the SRCU usage in GPIO and I think he should have also signed off on
> >> revocable before it got queued.
> >>
> >> Paul: I'm Cc'ing you on this patch to bring revocable to your attention.
> >> The series that implemented it and made its way into v7.0 is here:
> >>
> >>   https://lore.kernel.org/all/20260116080235.350305-1-tzungbi@kernel.org/
> >>
> >> Could you please take a look and say if the design looks sane to you?
> >> Especially the double SRCU on the revocable_provider.
> >
> > The first patch in the above URL adds SRCU, and the other
> > two add various tests.  I do not see a double SRCU, just an
> > srcu_read_lock() in revocable_try_access() and an srcu_read_unlock()
> > in revocable_withdraw_access().
> >
> > You are allowed to nest srcu_read_lock(), if that is what you are asking.
> > *However*, nesting revocable_try_access() on the same revocable structure
> > is buggy because the second call to revocable_try_access() would overwrite
> > the rp->srcu value written by the first call.  This could result in both
> > SRCU grace-period hangs and too-short SRCU grace periods, more likely
> > the former than the latter.
> >
> > Or do you mean something else by "double SRCU"?
> >
> 
> This series didn't have it yet, it appeared as a fix to a race reported after
> it was queued, sorry for the confusion. I'm talking about this bit[1] here.
> It returns an __rcu-annotated pointer, forcing the user to keep and manage it.
> 
> This is because when revoking the resource[2], the pointer storing the address
> of the revokable provider is also managed by SRCU - in addition to the
> revokable resource itself which seems to me like a weird concept. I understand
> the race condition it fixes but I assumed the whole concept of revokable is to
> free the user from being bothered by the implementation details behind it which
> leak out of the API if you need to keep __rcu around.

Please hold off on reviewing the patch and the "double SRCU" usage for now.
I'll remove the second RCU in the next version, which should serve as a
better starting point for a clean review.

Side note: It was actually one SRCU and one RCU, not a double SRCU.