linux-kernel - Re: [PATCH 1/3] lib: Add ref

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: <aJ7HUJ0boqYndbtD@google.com>
Date: Fri, 15 Aug 2025 05:36:16 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: bleung@...omium.org, dawidn@...gle.com, chrome-platform@...ts.linux.dev,
	akpm@...ux-foundation.org, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] lib: Add ref_proxy module

On Thu, Aug 14, 2025 at 12:55:55PM +0200, Danilo Krummrich wrote:
> On Thu Aug 14, 2025 at 11:10 AM CEST, Tzung-Bi Shih wrote:
> As mentioned in a sub-thread [1], this is pretty much what we already do in Rust
> with Revocable [2].
> 
> The Rust struct Devres [3] is built on Revocable, such that a device resource is
> only accessible for drivers as long as the device is actually bound to the
> driver. Once the device is unbound the resource is "revoked" and drivers are
> prevented from subsequent accesses.
> 
> I think it's be good to have a common naming scheme for this, hence I propose to
> name this struct revocable instead.

Sure, will address all review comments and fix in the next version.

> Besides that, I'm not exactly sure I understand why we need two structs for this.
> struct ref_proxy seems unnecessary. I think we should remove it and rename
> struct ref_proxy_provider to struct revocable. Or do I miss anything?

srcu_read_lock() returns an index [4].  The struct ref_proxy is necessary
for tracking the index for leaving the critical section.

[4] https://elixir.bootlin.com/linux/v6.16/source/kernel/rcu/srcutree.c#L750

> > + *
> > + * This holds an initial refcount to the struct.
> > + *
> > + * Return: The pointer of struct ref_proxy_provider.  NULL on errors.
> > + */
> > +struct ref_proxy_provider *ref_proxy_provider_alloc(void *ref)
> 
> The ref_proxy_provider owns the resource now, so when the ref_proxy_provider
> gets revoked (through the devres callback) the resource must be released, right?
> Where is this done? Who is responsible to do so? Shouldn't this function take a
> release callback that is called once the ref_proxy_provider is revoked?

Thank you, that's a valuable suggestion.  While both approaches are valid,
the current implementation is based on a clear separation of ownership.

The design is that struct ref_proxy_provider doesn't own the resource.
Instead, the resource provider (e.g., cros_ec_spi) is responsible for the
full lifecycle:
* It owns and ultimately releases the resource (e.g., [5]).
* It calls ref_proxy_provider_alloc() to expose the resource.
* It calls ref_proxy_provider_free() to revoke access to the resource.

By doing so, the resource provider doesn't need to create a bunch of
void (*release)(void *) callbacks (if multiple resources are exposing).

[5] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_spi.c#L752