Message-ID: <2025081408-fracture-happening-dda6@gregkh>
Date: Thu, 14 Aug 2025 12:03:11 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Tzung-Bi Shih <tzungbi@...nel.org>
Cc: bleung@...omium.org, dawidn@...gle.com, chrome-platform@...ts.linux.dev,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] lib: Add ref_proxy module

On Thu, Aug 14, 2025 at 09:10:18AM +0000, Tzung-Bi Shih wrote:
> Some resources can be removed asynchronously, for example, resources
> provided by a hot-pluggable device like USB.  When holding a reference
> to such a resource, it's possible for the resource to be removed and
> its memory freed, leading to use-after-free errors on subsequent access.
> 
> Introduce the ref_proxy library to establish weak references to such
> resources.  It allows a resource consumer to safely attempt to access a
> resource that might be freed at any time by the resource provider.
> 
> The implementation uses a provider/consumer model built on Sleepable
> RCU (SRCU) to guarantee safe memory access:
> 
>  - A resource provider allocates a struct ref_proxy_provider and
>    initializes it with a pointer to the resource.
> 
>  - A resource consumer that wants to access the resource allocates a
>    struct ref_proxy handle which holds a reference to the provider.
> 
>  - To access the resource, the consumer uses ref_proxy_get().  This
>    function enters an SRCU read-side critical section and returns the
>    pointer to the resource.  If the provider has already freed the
>    resource, it returns NULL.  After use, the consumer calls
>    ref_proxy_put() to exit the SRCU critical section.  The
>    REF_PROXY_GET() is a convenient helper for doing that.
> 
>  - When the provider needs to remove the resource, it calls
>    ref_proxy_provider_free().  This function sets the internal resource
>    pointer to NULL and then calls synchronize_srcu() to wait for all
>    current readers to finish before the resource can be completely torn
>    down.

Shouldn't this documentation go into the code files as well?  Ideally in
kernel doc format so that everyone can read it in the kernel
documentation output?


> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@...nel.org>
> ---
>  include/linux/ref_proxy.h |  37 ++++++++
>  lib/Kconfig               |   3 +
>  lib/Makefile              |   1 +
>  lib/ref_proxy.c           | 184 ++++++++++++++++++++++++++++++++++++++

Who is going to be the maintainer of these new files?

>  4 files changed, 225 insertions(+)
>  create mode 100644 include/linux/ref_proxy.h
>  create mode 100644 lib/ref_proxy.c
> 
> diff --git a/include/linux/ref_proxy.h b/include/linux/ref_proxy.h
> new file mode 100644
> index 000000000000..16ff29169272
> --- /dev/null
> +++ b/include/linux/ref_proxy.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

No copyright line?  That's bold, given your employer's normal rules :)

> +
> +#ifndef __LINUX_REF_PROXY_H
> +#define __LINUX_REF_PROXY_H
> +
> +#include <linux/cleanup.h>
> +
> +struct device;
> +struct ref_proxy;
> +struct ref_proxy_provider;
> +
> +struct ref_proxy_provider *ref_proxy_provider_alloc(void *ref);
> +void ref_proxy_provider_free(struct ref_proxy_provider *rpp);
> +struct ref_proxy_provider *devm_ref_proxy_provider_alloc(struct device *dev,
> +							 void *ref);
> +
> +struct ref_proxy *ref_proxy_alloc(struct ref_proxy_provider *rpp);
> +void ref_proxy_free(struct ref_proxy *proxy);
> +void __rcu *ref_proxy_get(struct ref_proxy *proxy);
> +void ref_proxy_put(struct ref_proxy *proxy);
> +
> +DEFINE_FREE(ref_proxy, struct ref_proxy *, if (_T) ref_proxy_put(_T))
> +
> +#define _REF_PROXY_GET(_proxy, _name, _label, _ref) \
> +	for (struct ref_proxy *_name __free(ref_proxy) = _proxy;	\
> +	     (_ref = ref_proxy_get(_name)) || true; ({ goto _label; }))	\
> +		if (0) {						\
> +_label:									\
> +			break;						\
> +		} else
> +
> +#define REF_PROXY_GET(_proxy, _ref)					\
> +	_REF_PROXY_GET(_proxy, __UNIQUE_ID(proxy_name),			\
> +		       __UNIQUE_ID(label), _ref)
> +
> +#endif /* __LINUX_REF_PROXY_H */
> +
> diff --git a/lib/Kconfig b/lib/Kconfig
> index c483951b624f..18237a766606 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -583,6 +583,9 @@ config STACKDEPOT_MAX_FRAMES
>  	default 64
>  	depends on STACKDEPOT
>  
> +config REF_PROXY
> +	bool
> +
>  config REF_TRACKER
>  	bool
>  	depends on STACKTRACE_SUPPORT
> diff --git a/lib/Makefile b/lib/Makefile
> index 392ff808c9b9..e8ad6f67cee9 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -258,6 +258,7 @@ KASAN_SANITIZE_stackdepot.o := n
>  KMSAN_SANITIZE_stackdepot.o := n
>  KCOV_INSTRUMENT_stackdepot.o := n
>  
> +obj-$(CONFIG_REF_PROXY) += ref_proxy.o
>  obj-$(CONFIG_REF_TRACKER) += ref_tracker.o
>  
>  libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
> diff --git a/lib/ref_proxy.c b/lib/ref_proxy.c
> new file mode 100644
> index 000000000000..49940bea651c
> --- /dev/null
> +++ b/lib/ref_proxy.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/device.h>

As this deals with struct device, shouldn't it go into drivers/base/ ?

> +#include <linux/kref.h>
> +#include <linux/ref_proxy.h>
> +#include <linux/slab.h>
> +#include <linux/srcu.h>
> +
> +/**
> + * struct ref_proxy_provider - A handle for resource provider.
> + * @srcu: The SRCU to protect the resource.
> + * @ref:  The pointer of resource.  It can point to anything.
> + * @kref: The refcount for this handle.
> + */
> +struct ref_proxy_provider {
> +	struct srcu_struct srcu;
> +	void __rcu *ref;
> +	struct kref kref;
> +};
> +
> +/**
> + * struct ref_proxy - A handle for resource consumer.
> + * @rpp: The pointer of resource provider.
> + * @idx: The index for the RCU critical section.
> + */
> +struct ref_proxy {
> +	struct ref_proxy_provider *rpp;
> +	int idx;
> +};
> +
> +/**
> + * ref_proxy_provider_alloc() - Allocate struct ref_proxy_provider.
> + * @ref: The pointer of resource.
> + *
> + * 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)
> +{
> +	struct ref_proxy_provider *rpp;
> +
> +	rpp = kzalloc(sizeof(*rpp), GFP_KERNEL);
> +	if (!rpp)
> +		return NULL;
> +
> +	init_srcu_struct(&rpp->srcu);
> +	rcu_assign_pointer(rpp->ref, ref);
> +	synchronize_srcu(&rpp->srcu);
> +	kref_init(&rpp->kref);
> +
> +	return rpp;
> +}
> +EXPORT_SYMBOL(ref_proxy_provider_alloc);

EXPORT_SYMBOL_GPL()?  I have to ask.

> +
> +static void ref_proxy_provider_release(struct kref *kref)
> +{
> +	struct ref_proxy_provider *rpp = container_of(kref,
> +			struct ref_proxy_provider, kref);
> +
> +	cleanup_srcu_struct(&rpp->srcu);
> +	kfree(rpp);
> +}
> +
> +/**
> + * ref_proxy_provider_free() - Free struct ref_proxy_provider.
> + * @rpp: The pointer of resource provider.
> + *
> + * This sets the resource `(struct ref_proxy_provider *)->ref` to NULL to
> + * indicate the resource has gone.
> + *
> + * This drops the refcount to the resource provider.  If it is the final
> + * reference, ref_proxy_provider_release() will be called to free the struct.
> + */
> +void ref_proxy_provider_free(struct ref_proxy_provider *rpp)
> +{
> +	rcu_assign_pointer(rpp->ref, NULL);
> +	synchronize_srcu(&rpp->srcu);
> +	kref_put(&rpp->kref, ref_proxy_provider_release);
> +}
> +EXPORT_SYMBOL(ref_proxy_provider_free);
> +
> +static void devm_ref_proxy_provider_free(void *data)
> +{
> +	struct ref_proxy_provider *rpp = data;
> +
> +	ref_proxy_provider_free(rpp);
> +}
> +
> +/**
> + * devm_ref_proxy_provider_alloc() - Dev-managed ref_proxy_provider_alloc().
> + * @dev: The device.
> + * @ref: The pointer of resource.
> + *
> + * This holds an initial refcount to the struct.
> + *
> + * Return: The pointer of struct ref_proxy_provider.  NULL on errors.
> + */
> +struct ref_proxy_provider *devm_ref_proxy_provider_alloc(struct device *dev,
> +							 void *ref)
> +{
> +	struct ref_proxy_provider *rpp;
> +
> +	rpp = ref_proxy_provider_alloc(ref);
> +	if (rpp)
> +		if (devm_add_action_or_reset(dev, devm_ref_proxy_provider_free,
> +					     rpp))
> +			return NULL;
> +
> +	return rpp;
> +}
> +EXPORT_SYMBOL(devm_ref_proxy_provider_alloc);

Do we really need a devm version?  That feels odd as this should be
doing almost the same thing that devm does, right?  How do they interact
properly?

And do you have a selftest for this thing anywhere?

thanks,

greg k-h

Powered by blists - more mailing lists