This is a follow-up series of [1]. It tries to fix a possible UAF in the fops of cros_ec_chardev after the underlying protocol device has gone by using revocable.
The 1st patch introduces the revocable which is an implementation of ideas from the talk [2].
The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
The 4th patch converts existing protocol devices to resource providers of cros_ec_device.
The 5th patch converts cros_ec_chardev to a resource consumer of cros_ec_device to fix the UAF.
[1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@ker... [2] https://lpc.events/event/17/contributions/1627/
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
v3: - Rebase onto https://lore.kernel.org/chrome-platform/20250828083601.856083-1-tzungbi@kern... and next-20250912. - Change the 4th patch accordingly.
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-1-tzungbi@kern... - Rename "ref_proxy" -> "revocable". - Add test cases in Kunit and selftest.
v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-1-tzungbi@ker...
Tzung-Bi Shih (5): revocable: Revocable resource management revocable: Add Kunit test cases selftests: revocable: Add kselftest cases platform/chrome: Protect cros_ec_device lifecycle with revocable platform/chrome: cros_ec_chardev: Consume cros_ec_device via revocable
.../driver-api/driver-model/index.rst | 1 + .../driver-api/driver-model/revocable.rst | 151 ++++++++++++ MAINTAINERS | 9 + drivers/base/Kconfig | 8 + drivers/base/Makefile | 5 +- drivers/base/revocable.c | 229 ++++++++++++++++++ drivers/base/revocable_test.c | 110 +++++++++ drivers/platform/chrome/cros_ec.c | 5 + drivers/platform/chrome/cros_ec_chardev.c | 124 +++++++--- include/linux/platform_data/cros_ec_proto.h | 4 + include/linux/revocable.h | 37 +++ tools/testing/selftests/Makefile | 1 + .../selftests/drivers/base/revocable/Makefile | 7 + .../drivers/base/revocable/revocable_test.c | 116 +++++++++ .../drivers/base/revocable/test-revocable.sh | 39 +++ .../base/revocable/test_modules/Makefile | 10 + .../revocable/test_modules/revocable_test.c | 188 ++++++++++++++ 17 files changed, 1003 insertions(+), 41 deletions(-) create mode 100644 Documentation/driver-api/driver-model/revocable.rst create mode 100644 drivers/base/revocable.c create mode 100644 drivers/base/revocable_test.c create mode 100644 include/linux/revocable.h create mode 100644 tools/testing/selftests/drivers/base/revocable/Makefile create mode 100644 tools/testing/selftests/drivers/base/revocable/revocable_test.c create mode 100755 tools/testing/selftests/drivers/base/revocable/test-revocable.sh create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/Makefile create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
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 revocable 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 revocable_provider and initializes it with a pointer to the resource.
- A resource consumer that wants to access the resource allocates a struct revocable which holds a reference to the provider.
- To access the resource, the consumer uses revocable_try_access(). 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 revocable_release() to exit the SRCU critical section. The REVOCABLE() is a convenient helper for doing that.
- When the provider needs to remove the resource, it calls revocable_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.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- v3: - No changes.
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-2-tzungbi@kern... - Rename "ref_proxy" -> "revocable". - Add introduction in kernel-doc format in revocable.c. - Add MAINTAINERS entry. - Add copyright. - Move from lib/ to drivers/base/. - EXPORT_SYMBOL() -> EXPORT_SYMBOL_GPL(). - Add Documentation/. - Rename _get() -> try_access(); _put() -> release(). - Fix a sparse warning by removing the redundant __rcu annotations. - Fix a sparse warning by adding __acquires() and __releases() annotations.
v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-2-tzungbi@ker...
.../driver-api/driver-model/index.rst | 1 + .../driver-api/driver-model/revocable.rst | 151 ++++++++++++ MAINTAINERS | 7 + drivers/base/Makefile | 2 +- drivers/base/revocable.c | 229 ++++++++++++++++++ include/linux/revocable.h | 37 +++ 6 files changed, 426 insertions(+), 1 deletion(-) create mode 100644 Documentation/driver-api/driver-model/revocable.rst create mode 100644 drivers/base/revocable.c create mode 100644 include/linux/revocable.h
diff --git a/Documentation/driver-api/driver-model/index.rst b/Documentation/driver-api/driver-model/index.rst index 4831bdd92e5c..8e1ee21185df 100644 --- a/Documentation/driver-api/driver-model/index.rst +++ b/Documentation/driver-api/driver-model/index.rst @@ -14,6 +14,7 @@ Driver Model overview platform porting + revocable
.. only:: subproject and html
diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst new file mode 100644 index 000000000000..b9e2968ba9c1 --- /dev/null +++ b/Documentation/driver-api/driver-model/revocable.rst @@ -0,0 +1,151 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Revocable Resource Management +============================== + +Overview +======== + +In a system with hot-pluggable devices, such as USB, resources provided by +these devices can be removed asynchronously. If a consumer holds a reference +to such a resource, the resource might be deallocated while the reference is +still held, leading to use-after-free errors upon subsequent access. + +The "revocable" mechanism addresses this by establishing a weak reference to a +resource that might be freed at any time. It allows a resource consumer to +safely attempt to access the resource, guaranteeing that the access is valid +for the duration of its use, or it fails safely if the resource has already +been revoked. + +The implementation is based on a provider/consumer model that uses Sleepable +RCU (SRCU) to ensure safe memory access without traditional locking. + +How It Works +============ + +1. **Provider**: A resource provider, such as a driver for a hot-pluggable + device, allocates a ``struct revocable_provider``. This structure is + initialized with a pointer to the actual resource it manages. + +2. **Consumer**: A consumer that needs to access the resource is given a + ``struct revocable``, which acts as a handle containing a reference to + the provider. + +3. **Accessing the Resource**: To access the resource, the consumer uses + ``revocable_try_access()``. This function enters an SRCU read-side + critical section and returns a pointer to the resource. If the provider + has already revoked the resource, this function returns ``NULL``. The + consumer must check for this ``NULL`` return. + +4. **Releasing the Resource**: After the consumer has finished using the + resource, it must call ``revocable_release()`` to exit the SRCU critical + section. This signals that the consumer no longer requires access. The + ``REVOCABLE()`` macro is provided as a convenient and safe way to manage + the access-release cycle. + +5. **Revoking the Resource**: When the provider needs to remove the resource + (e.g., the device is unplugged), it calls ``revocable_provider_free()``. + This function first sets the internal resource pointer to ``NULL``, + preventing any new consumers from accessing it. It then calls + ``synchronize_srcu()``, which waits for all existing consumers currently + in the SRCU critical section to finish their work. Once all consumers + have released their access, the resource can be safely deallocated. + +Revocable vs. Device-Managed (devm) Resources +============================================= + +It's important to understand the distinction between a standard +device-managed (devm) resource and a resource managed by a +``revocable_provider``. + +The key difference is their lifetime: + +* A **devm resource** is tied to the lifetime of the device. It is + automatically freed when the device is unbound. +* A **revocable_provider** persists as long as there are active references + to it from ``revocable`` consumer handles. + +This means that a ``revocable_provider`` can outlive the device that created +it. This is a deliberate design feature, allowing consumers to hold a +reference to a resource even after the underlying device has been removed, +without causing a fault. When the consumer attempts to access the resource, +it will simply be informed that the resource is no longer available. + +API and Usage +============= + +For Resource Providers +---------------------- + +``struct revocable_provider *revocable_provider_alloc(void *res);`` + Allocates a provider handle for the given resource ``res``. It returns a + pointer to the ``revocable_provider`` on success, or ``NULL`` on failure. + +``struct revocable_provider *devm_revocable_provider_alloc(struct device *dev, void *res);`` + A device-managed version of ``revocable_provider_alloc``. It is + convenient to allocate providers via this function if the ``res`` is also + tied to the lifetime of the ``dev``. ``revocable_provider_free`` will be + called automatically when the device is unbound. + +``void revocable_provider_free(struct revocable_provider *rp);`` + Revokes the resource. This function marks the resource as unavailable and + waits for all current consumers to finish before the underlying memory + can be freed. + +For Resource Consumers +---------------------- + +``struct revocable *revocable_alloc(struct revocable_provider *rp);`` + Allocates a consumer handle for a given provider ``rp``. + +``void revocable_free(struct revocable *rev);`` + Frees a consumer handle. + +``void *revocable_try_access(struct revocable *rev);`` + Attempts to gain access to the resource. Returns a pointer to the + resource on success or ``NULL`` if it has been revoked. + +``void revocable_release(struct revocable *rev);`` + Releases access to the resource, exiting the SRCU critical section. + +The ``REVOCABLE()`` Macro +========================= + +The ``REVOCABLE()`` macro simplifies the access-release cycle for consumers, +ensuring that ``revocable_release()`` is always called, even in the case of +an early exit. + +``REVOCABLE(rev, res)`` + * ``rev``: The consumer's ``struct revocable *`` handle. + * ``res``: A pointer variable that will be assigned the resource. + +The macro creates a ``for`` loop that executes exactly once. Inside the loop, +``res`` is populated with the result of ``revocable_try_access()``. The +consumer code **must** check if ``res`` is ``NULL`` before using it. The +``revocable_release()`` function is automatically called when the scope of +the loop is exited. + +Example Usage +------------- + +.. code-block:: c + + void consumer_use_resource(struct revocable *rev) + { + struct foo_resource *res; + + REVOCABLE(rev, res) { + // Always check if the resource is valid. + if (!res) { + pr_warn("Resource is not available\n"); + return; + } + + // At this point, 'res' is guaranteed to be valid until + // this block exits. + do_something_with(res); + } + + // revocable_release() is automatically called here. + } diff --git a/MAINTAINERS b/MAINTAINERS index fa7f80bd7b2f..5d11aeeb546e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21877,6 +21877,13 @@ F: include/uapi/linux/rseq.h F: kernel/rseq.c F: tools/testing/selftests/rseq/
+REVOCABLE RESOURCE MANAGEMENT +M: Tzung-Bi Shih tzungbi@kernel.org +L: linux-kernel@vger.kernel.org +S: Maintained +F: drivers/base/revocable.c +F: include/linux/revocable.h + RFKILL M: Johannes Berg johannes@sipsolutions.net L: linux-wireless@vger.kernel.org diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 8074a10183dc..bdf854694e39 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -6,7 +6,7 @@ obj-y := component.o core.o bus.o dd.o syscore.o \ cpu.o firmware.o init.o map.o devres.o \ attribute_container.o transport_class.o \ topology.o container.o property.o cacheinfo.o \ - swnode.o faux.o + swnode.o faux.o revocable.o obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-y += power/ diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c new file mode 100644 index 000000000000..80a48896b241 --- /dev/null +++ b/drivers/base/revocable.c @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2025 Google LLC + * + * Revocable resource management + */ + +#include <linux/device.h> +#include <linux/kref.h> +#include <linux/revocable.h> +#include <linux/slab.h> +#include <linux/srcu.h> + +/** + * DOC: Overview + * + * 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 revocable 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 revocable_provider and + * initializes it with a pointer to the resource. + * + * - A resource consumer that wants to access the resource allocates a + * struct revocable which holds a reference to the provider. + * + * - To access the resource, the consumer uses revocable_try_access(). + * 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 + * revocable_release() to exit the SRCU critical section. The + * REVOCABLE() is a convenient helper for doing that. + * + * - When the provider needs to remove the resource, it calls + * revocable_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. + */ + +/** + * struct revocable_provider - A handle for resource provider. + * @srcu: The SRCU to protect the resource. + * @res: The pointer of resource. It can point to anything. + * @kref: The refcount for this handle. + */ +struct revocable_provider { + struct srcu_struct srcu; + void __rcu *res; + struct kref kref; +}; + +/** + * struct revocable - A handle for resource consumer. + * @rp: The pointer of resource provider. + * @idx: The index for the RCU critical section. + */ +struct revocable { + struct revocable_provider *rp; + int idx; +}; + +/** + * revocable_provider_alloc() - Allocate struct revocable_provider. + * @res: The pointer of resource. + * + * This holds an initial refcount to the struct. + * + * Return: The pointer of struct revocable_provider. NULL on errors. + */ +struct revocable_provider *revocable_provider_alloc(void *res) +{ + struct revocable_provider *rp; + + rp = kzalloc(sizeof(*rp), GFP_KERNEL); + if (!rp) + return NULL; + + init_srcu_struct(&rp->srcu); + rcu_assign_pointer(rp->res, res); + synchronize_srcu(&rp->srcu); + kref_init(&rp->kref); + + return rp; +} +EXPORT_SYMBOL_GPL(revocable_provider_alloc); + +static void revocable_provider_release(struct kref *kref) +{ + struct revocable_provider *rp = container_of(kref, + struct revocable_provider, kref); + + cleanup_srcu_struct(&rp->srcu); + kfree(rp); +} + +/** + * revocable_provider_free() - Free struct revocable_provider. + * @rp: The pointer of resource provider. + * + * This sets the resource `(struct revocable_provider *)->res` to NULL to + * indicate the resource has gone. + * + * This drops the refcount to the resource provider. If it is the final + * reference, revocable_provider_release() will be called to free the struct. + */ +void revocable_provider_free(struct revocable_provider *rp) +{ + rcu_assign_pointer(rp->res, NULL); + synchronize_srcu(&rp->srcu); + kref_put(&rp->kref, revocable_provider_release); +} +EXPORT_SYMBOL_GPL(revocable_provider_free); + +static void devm_revocable_provider_free(void *data) +{ + struct revocable_provider *rp = data; + + revocable_provider_free(rp); +} + +/** + * devm_revocable_provider_alloc() - Dev-managed revocable_provider_alloc(). + * @dev: The device. + * @res: The pointer of resource. + * + * It is convenient to allocate providers via this function if the @res is + * also tied to the lifetime of the @dev. revocable_provider_free() will + * be called automatically when the device is unbound. + * + * This holds an initial refcount to the struct. + * + * Return: The pointer of struct revocable_provider. NULL on errors. + */ +struct revocable_provider *devm_revocable_provider_alloc(struct device *dev, + void *res) +{ + struct revocable_provider *rp; + + rp = revocable_provider_alloc(res); + if (!rp) + return NULL; + + if (devm_add_action_or_reset(dev, devm_revocable_provider_free, rp)) + return NULL; + + return rp; +} +EXPORT_SYMBOL_GPL(devm_revocable_provider_alloc); + +/** + * revocable_alloc() - Allocate struct revocable_provider. + * @rp: The pointer of resource provider. + * + * This holds a refcount to the resource provider. + * + * Return: The pointer of struct revocable_provider. NULL on errors. + */ +struct revocable *revocable_alloc(struct revocable_provider *rp) +{ + struct revocable *rev; + + rev = kzalloc(sizeof(*rev), GFP_KERNEL); + if (!rev) + return NULL; + + rev->rp = rp; + kref_get(&rp->kref); + + return rev; +} +EXPORT_SYMBOL_GPL(revocable_alloc); + +/** + * revocable_free() - Free struct revocable. + * @rev: The pointer of struct revocable. + * + * This drops a refcount to the resource provider. If it is the final + * reference, revocable_provider_release() will be called to free the struct. + */ +void revocable_free(struct revocable *rev) +{ + struct revocable_provider *rp = rev->rp; + + kref_put(&rp->kref, revocable_provider_release); + kfree(rev); +} +EXPORT_SYMBOL_GPL(revocable_free); + +/** + * revocable_try_access() - Try to access the resource. + * @rev: The pointer of struct revocable. + * + * This tries to de-reference to the resource and enters a RCU critical + * section. + * + * Return: The pointer to the resource. NULL if the resource has gone. + */ +void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu) +{ + struct revocable_provider *rp = rev->rp; + + rev->idx = srcu_read_lock(&rp->srcu); + return rcu_dereference(rp->res); +} +EXPORT_SYMBOL_GPL(revocable_try_access); + +/** + * revocable_release() - Stop accessing to the resource. + * @rev: The pointer of struct revocable. + * + * Call this function to indicate the resource is no longer used. It exits + * the RCU critical section. + */ +void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu) +{ + struct revocable_provider *rp = rev->rp; + + srcu_read_unlock(&rp->srcu, rev->idx); +} +EXPORT_SYMBOL_GPL(revocable_release); diff --git a/include/linux/revocable.h b/include/linux/revocable.h new file mode 100644 index 000000000000..17d9b7ce633d --- /dev/null +++ b/include/linux/revocable.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright 2025 Google LLC + */ + +#ifndef __LINUX_REVOCABLE_H +#define __LINUX_REVOCABLE_H + +#include <linux/cleanup.h> + +struct device; +struct revocable; +struct revocable_provider; + +struct revocable_provider *revocable_provider_alloc(void *res); +void revocable_provider_free(struct revocable_provider *rp); +struct revocable_provider *devm_revocable_provider_alloc(struct device *dev, + void *res); + +struct revocable *revocable_alloc(struct revocable_provider *rp); +void revocable_free(struct revocable *rev); +void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu); +void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu); + +DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T)) + +#define _REVOCABLE(_rev, _label, _res) \ + for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev; \ + (_res = revocable_try_access(_rev)) || true; ({ goto _label; })) \ + if (0) { \ +_label: \ + break; \ + } else + +#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res) + +#endif /* __LINUX_REVOCABLE_H */
On Fri Sep 12, 2025 at 10:17 AM CEST, Tzung-Bi Shih wrote:
+/**
- struct revocable_provider - A handle for resource provider.
- @srcu: The SRCU to protect the resource.
- @res: The pointer of resource. It can point to anything.
- @kref: The refcount for this handle.
- */
+struct revocable_provider {
- struct srcu_struct srcu;
- void __rcu *res;
- struct kref kref;
+};
I think a revocable provider should provide an optional revoke() callback where users of the revocable provider can release the revoked resource.
But this can also be done as a follow-up.
+/**
- struct revocable - A handle for resource consumer.
- @rp: The pointer of resource provider.
- @idx: The index for the RCU critical section.
- */
+struct revocable {
- struct revocable_provider *rp;
- int idx;
+};
I think I asked about this in the previous version (but I don't remember if there was an answer):
In Rust we get away with a single Revocable<T> structure, but we're using RCU instead of SRCU. It seems to me that the split between struct revocable and struct revocable_provider is only for the SRCU index? Or is there any other reason?
+/**
- revocable_provider_free() - Free struct revocable_provider.
- @rp: The pointer of resource provider.
- This sets the resource `(struct revocable_provider *)->res` to NULL to
- indicate the resource has gone.
- This drops the refcount to the resource provider. If it is the final
- reference, revocable_provider_release() will be called to free the struct.
- */
+void revocable_provider_free(struct revocable_provider *rp) +{
- rcu_assign_pointer(rp->res, NULL);
- synchronize_srcu(&rp->srcu);
- kref_put(&rp->kref, revocable_provider_release);
+} +EXPORT_SYMBOL_GPL(revocable_provider_free);
I think naming this "free" is a bit misleading, since what it basically does is
(1) Revoke access to the resource.
(2) Drop a reference count of struct revocable_provider.
In a typical application there may still be struct revocable instances that have a reference to the provider, so we can't claim that it's freed here.
So, given that, I'd rather call this revocable_provider_revoke().
+static void devm_revocable_provider_free(void *data) +{
- struct revocable_provider *rp = data;
- revocable_provider_free(rp);
+}
Same here, I'd call this devm_revocable_provider_revoke().
+DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
+#define _REVOCABLE(_rev, _label, _res) \
- for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev; \
(_res = revocable_try_access(_rev)) || true; ({ goto _label; })) \if (0) { \+_label: \
break; \} else+#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)
This is basically the same as Revocable::try_access_with() [1] in Rust, i.e. try to access and run a closure.
Admittedly, REVOCABLE_TRY_ACCESS_WITH() is pretty verbose and I also do not have a great idea to shorten it.
Maybe you have a good idea, otherwise I'm also fine with the current name.
Otherwise, maybe it's worth to link to the Rust Revocable API for reference?
With *_free() renamed to *_revoke(), feel free to add:
Acked-by: Danilo Krummrich dakr@kernel.org
[1] https://rust.docs.kernel.org/kernel/revocable/struct.Revocable.html#method.t...
On Fri, Sep 12, 2025 at 11:05:20AM +0200, Danilo Krummrich wrote:
On Fri Sep 12, 2025 at 10:17 AM CEST, Tzung-Bi Shih wrote:
+/**
- struct revocable_provider - A handle for resource provider.
- @srcu: The SRCU to protect the resource.
- @res: The pointer of resource. It can point to anything.
- @kref: The refcount for this handle.
- */
+struct revocable_provider {
- struct srcu_struct srcu;
- void __rcu *res;
- struct kref kref;
+};
I think a revocable provider should provide an optional revoke() callback where users of the revocable provider can release the revoked resource.
But this can also be done as a follow-up.
Understood. Since this effectively delegates the memory of `res` to the struct revocable provider, I propose we name the callback .release().
+/**
- struct revocable - A handle for resource consumer.
- @rp: The pointer of resource provider.
- @idx: The index for the RCU critical section.
- */
+struct revocable {
- struct revocable_provider *rp;
- int idx;
+};
I think I asked about this in the previous version (but I don't remember if there was an answer):
Yes, in v1 https://lore.kernel.org/chrome-platform/aJ7HUJ0boqYndbtD@google.com/.
In Rust we get away with a single Revocable<T> structure, but we're using RCU instead of SRCU. It seems to me that the split between struct revocable and struct revocable_provider is only for the SRCU index? Or is there any other reason?
Yes, for the SRCU index.
+/**
- revocable_provider_free() - Free struct revocable_provider.
- @rp: The pointer of resource provider.
- This sets the resource `(struct revocable_provider *)->res` to NULL to
- indicate the resource has gone.
- This drops the refcount to the resource provider. If it is the final
- reference, revocable_provider_release() will be called to free the struct.
- */
+void revocable_provider_free(struct revocable_provider *rp) +{
- rcu_assign_pointer(rp->res, NULL);
- synchronize_srcu(&rp->srcu);
- kref_put(&rp->kref, revocable_provider_release);
+} +EXPORT_SYMBOL_GPL(revocable_provider_free);
I think naming this "free" is a bit misleading, since what it basically does is
(1) Revoke access to the resource.
(2) Drop a reference count of struct revocable_provider.
In a typical application there may still be struct revocable instances that have a reference to the provider, so we can't claim that it's freed here.
So, given that, I'd rather call this revocable_provider_revoke().
Ack, will fix it in the next version.
+static void devm_revocable_provider_free(void *data) +{
- struct revocable_provider *rp = data;
- revocable_provider_free(rp);
+}
Same here, I'd call this devm_revocable_provider_revoke().
Ack, will fix it in the next version.
+DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
+#define _REVOCABLE(_rev, _label, _res) \
- for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev; \
(_res = revocable_try_access(_rev)) || true; ({ goto _label; })) \if (0) { \+_label: \
break; \} else+#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)
This is basically the same as Revocable::try_access_with() [1] in Rust, i.e. try to access and run a closure.
Admittedly, REVOCABLE_TRY_ACCESS_WITH() is pretty verbose and I also do not have a great idea to shorten it.
Maybe you have a good idea, otherwise I'm also fine with the current name.
Otherwise, maybe it's worth to link to the Rust Revocable API for reference?
No, I don't have a good idea either. Will use REVOCABLE_TRY_ACCESS_WITH() to align with Rust Revocable API in the next version.
Tzung-Bi Shih tzungbi@kernel.org writes:
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.
Far be it from me to complain about a new feature that comes with nice documentation! I will make one small observation, though, for consideration.
We have the document itself:
diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst new file mode 100644 index 000000000000..b9e2968ba9c1 --- /dev/null +++ b/Documentation/driver-api/driver-model/revocable.rst @@ -0,0 +1,151 @@ +.. SPDX-License-Identifier: GPL-2.0
+============================== +Revocable Resource Management +==============================
+Overview +========
+In a system with hot-pluggable devices, such as USB, resources provided by +these devices can be removed asynchronously. If a consumer holds a reference +to such a resource, the resource might be deallocated while the reference is +still held, leading to use-after-free errors upon subsequent access.
+The "revocable" mechanism addresses this by establishing a weak reference to a +resource that might be freed at any time. It allows a resource consumer to +safely attempt to access the resource, guaranteeing that the access is valid +for the duration of its use, or it fails safely if the resource has already +been revoked.
[...]
Then there is the in-code documentation:
diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c new file mode 100644 index 000000000000..80a48896b241 --- /dev/null +++ b/drivers/base/revocable.c @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright 2025 Google LLC
- Revocable resource management
- */
+#include <linux/device.h> +#include <linux/kref.h> +#include <linux/revocable.h> +#include <linux/slab.h> +#include <linux/srcu.h>
+/**
- DOC: Overview
- 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 revocable 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 revocable_provider and
- initializes it with a pointer to the resource.
There is a certain amount of duplication here, stuff that might go out of sync at some point. I would consider pushing the bulk of the information into the kerneldoc comments, then actually *using* those comments in the .rst file (with kernel-doc directives) to create the rendered version.
Thanks,
jon
On Fri, Sep 12, 2025 at 07:27:26AM -0600, Jonathan Corbet wrote:
There is a certain amount of duplication here, stuff that might go out of sync at some point. I would consider pushing the bulk of the information into the kerneldoc comments, then actually *using* those comments in the .rst file (with kernel-doc directives) to create the rendered version.
Ack, will fix it in the next version.
On Fri, Sep 12, 2025 at 08:17:13AM +0000, Tzung-Bi Shih wrote:
+void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu) +{
- struct revocable_provider *rp = rev->rp;
- rev->idx = srcu_read_lock(&rp->srcu);
- return rcu_dereference(rp->res);
Got a warning from lock debugging. This should be srcu_dereference(). Will fix it in the next version.
On Fri, Sep 12, 2025 at 08:17:13AM +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 revocable 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 revocable_provider and initializes it with a pointer to the resource.
A resource consumer that wants to access the resource allocates a struct revocable which holds a reference to the provider.
To access the resource, the consumer uses revocable_try_access(). 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 revocable_release() to exit the SRCU critical section. The REVOCABLE() is a convenient helper for doing that.
When the provider needs to remove the resource, it calls revocable_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.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org
Want. Want. Want.
Acked-by: Simona Vetter simona.vetter@ffwll.ch
SRCU isn't the greatest choice in theory, which is why Rust uses plain RCU. But with C's real-world track record at getting error paths right, it's imo the pragmatic choice since it allows you to cheat a bit and cut some corners. Rust is flat out just massively better at this, despite linux/cleanup.h and all the other improvements we've landed over the years.
Since DRM uses the same concept in drm_dev_enter() and drm_dev_exit() it would be really neat to have a patch that switches these over internally to use revocable resources. That way drivers could use REVOCEABLE() and we could slowly convert away from that DRM-ism.
Cheers, Sima
v3:
- No changes.
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-2-tzungbi@kern...
- Rename "ref_proxy" -> "revocable".
- Add introduction in kernel-doc format in revocable.c.
- Add MAINTAINERS entry.
- Add copyright.
- Move from lib/ to drivers/base/.
- EXPORT_SYMBOL() -> EXPORT_SYMBOL_GPL().
- Add Documentation/.
- Rename _get() -> try_access(); _put() -> release().
- Fix a sparse warning by removing the redundant __rcu annotations.
- Fix a sparse warning by adding __acquires() and __releases() annotations.
v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-2-tzungbi@ker...
.../driver-api/driver-model/index.rst | 1 + .../driver-api/driver-model/revocable.rst | 151 ++++++++++++ MAINTAINERS | 7 + drivers/base/Makefile | 2 +- drivers/base/revocable.c | 229 ++++++++++++++++++ include/linux/revocable.h | 37 +++ 6 files changed, 426 insertions(+), 1 deletion(-) create mode 100644 Documentation/driver-api/driver-model/revocable.rst create mode 100644 drivers/base/revocable.c create mode 100644 include/linux/revocable.h
diff --git a/Documentation/driver-api/driver-model/index.rst b/Documentation/driver-api/driver-model/index.rst index 4831bdd92e5c..8e1ee21185df 100644 --- a/Documentation/driver-api/driver-model/index.rst +++ b/Documentation/driver-api/driver-model/index.rst @@ -14,6 +14,7 @@ Driver Model overview platform porting
- revocable
.. only:: subproject and html diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst new file mode 100644 index 000000000000..b9e2968ba9c1 --- /dev/null +++ b/Documentation/driver-api/driver-model/revocable.rst @@ -0,0 +1,151 @@ +.. SPDX-License-Identifier: GPL-2.0
+============================== +Revocable Resource Management +==============================
+Overview +========
+In a system with hot-pluggable devices, such as USB, resources provided by +these devices can be removed asynchronously. If a consumer holds a reference +to such a resource, the resource might be deallocated while the reference is +still held, leading to use-after-free errors upon subsequent access.
+The "revocable" mechanism addresses this by establishing a weak reference to a +resource that might be freed at any time. It allows a resource consumer to +safely attempt to access the resource, guaranteeing that the access is valid +for the duration of its use, or it fails safely if the resource has already +been revoked.
+The implementation is based on a provider/consumer model that uses Sleepable +RCU (SRCU) to ensure safe memory access without traditional locking.
+How It Works +============
+1. **Provider**: A resource provider, such as a driver for a hot-pluggable
- device, allocates a ``struct revocable_provider``. This structure is
- initialized with a pointer to the actual resource it manages.
+2. **Consumer**: A consumer that needs to access the resource is given a
- ``struct revocable``, which acts as a handle containing a reference to
- the provider.
+3. **Accessing the Resource**: To access the resource, the consumer uses
- ``revocable_try_access()``. This function enters an SRCU read-side
- critical section and returns a pointer to the resource. If the provider
- has already revoked the resource, this function returns ``NULL``. The
- consumer must check for this ``NULL`` return.
+4. **Releasing the Resource**: After the consumer has finished using the
- resource, it must call ``revocable_release()`` to exit the SRCU critical
- section. This signals that the consumer no longer requires access. The
- ``REVOCABLE()`` macro is provided as a convenient and safe way to manage
- the access-release cycle.
+5. **Revoking the Resource**: When the provider needs to remove the resource
- (e.g., the device is unplugged), it calls ``revocable_provider_free()``.
- This function first sets the internal resource pointer to ``NULL``,
- preventing any new consumers from accessing it. It then calls
- ``synchronize_srcu()``, which waits for all existing consumers currently
- in the SRCU critical section to finish their work. Once all consumers
- have released their access, the resource can be safely deallocated.
+Revocable vs. Device-Managed (devm) Resources +=============================================
+It's important to understand the distinction between a standard +device-managed (devm) resource and a resource managed by a +``revocable_provider``.
+The key difference is their lifetime:
+* A **devm resource** is tied to the lifetime of the device. It is
- automatically freed when the device is unbound.
+* A **revocable_provider** persists as long as there are active references
- to it from ``revocable`` consumer handles.
+This means that a ``revocable_provider`` can outlive the device that created +it. This is a deliberate design feature, allowing consumers to hold a +reference to a resource even after the underlying device has been removed, +without causing a fault. When the consumer attempts to access the resource, +it will simply be informed that the resource is no longer available.
+API and Usage +=============
+For Resource Providers +----------------------
+``struct revocable_provider *revocable_provider_alloc(void *res);``
- Allocates a provider handle for the given resource ``res``. It returns a
- pointer to the ``revocable_provider`` on success, or ``NULL`` on failure.
+``struct revocable_provider *devm_revocable_provider_alloc(struct device *dev, void *res);``
- A device-managed version of ``revocable_provider_alloc``. It is
- convenient to allocate providers via this function if the ``res`` is also
- tied to the lifetime of the ``dev``. ``revocable_provider_free`` will be
- called automatically when the device is unbound.
+``void revocable_provider_free(struct revocable_provider *rp);``
- Revokes the resource. This function marks the resource as unavailable and
- waits for all current consumers to finish before the underlying memory
- can be freed.
+For Resource Consumers +----------------------
+``struct revocable *revocable_alloc(struct revocable_provider *rp);``
- Allocates a consumer handle for a given provider ``rp``.
+``void revocable_free(struct revocable *rev);``
- Frees a consumer handle.
+``void *revocable_try_access(struct revocable *rev);``
- Attempts to gain access to the resource. Returns a pointer to the
- resource on success or ``NULL`` if it has been revoked.
+``void revocable_release(struct revocable *rev);``
- Releases access to the resource, exiting the SRCU critical section.
+The ``REVOCABLE()`` Macro +=========================
+The ``REVOCABLE()`` macro simplifies the access-release cycle for consumers, +ensuring that ``revocable_release()`` is always called, even in the case of +an early exit.
+``REVOCABLE(rev, res)``
- ``rev``: The consumer's ``struct revocable *`` handle.
- ``res``: A pointer variable that will be assigned the resource.
+The macro creates a ``for`` loop that executes exactly once. Inside the loop, +``res`` is populated with the result of ``revocable_try_access()``. The +consumer code **must** check if ``res`` is ``NULL`` before using it. The +``revocable_release()`` function is automatically called when the scope of +the loop is exited.
+Example Usage +-------------
+.. code-block:: c
- void consumer_use_resource(struct revocable *rev)
- {
struct foo_resource *res;REVOCABLE(rev, res) {// Always check if the resource is valid.if (!res) {pr_warn("Resource is not available\n");return;}// At this point, 'res' is guaranteed to be valid until// this block exits.do_something_with(res);}// revocable_release() is automatically called here.- }
diff --git a/MAINTAINERS b/MAINTAINERS index fa7f80bd7b2f..5d11aeeb546e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21877,6 +21877,13 @@ F: include/uapi/linux/rseq.h F: kernel/rseq.c F: tools/testing/selftests/rseq/ +REVOCABLE RESOURCE MANAGEMENT +M: Tzung-Bi Shih tzungbi@kernel.org +L: linux-kernel@vger.kernel.org +S: Maintained +F: drivers/base/revocable.c +F: include/linux/revocable.h
RFKILL M: Johannes Berg johannes@sipsolutions.net L: linux-wireless@vger.kernel.org diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 8074a10183dc..bdf854694e39 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -6,7 +6,7 @@ obj-y := component.o core.o bus.o dd.o syscore.o \ cpu.o firmware.o init.o map.o devres.o \ attribute_container.o transport_class.o \ topology.o container.o property.o cacheinfo.o \
swnode.o faux.o
swnode.o faux.o revocable.oobj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-y += power/ diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c new file mode 100644 index 000000000000..80a48896b241 --- /dev/null +++ b/drivers/base/revocable.c @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright 2025 Google LLC
- Revocable resource management
- */
+#include <linux/device.h> +#include <linux/kref.h> +#include <linux/revocable.h> +#include <linux/slab.h> +#include <linux/srcu.h>
+/**
- DOC: Overview
- 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 revocable 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 revocable_provider and
- initializes it with a pointer to the resource.
- A resource consumer that wants to access the resource allocates a
- struct revocable which holds a reference to the provider.
- To access the resource, the consumer uses revocable_try_access().
- 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
- revocable_release() to exit the SRCU critical section. The
- REVOCABLE() is a convenient helper for doing that.
- When the provider needs to remove the resource, it calls
- revocable_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.
- */
+/**
- struct revocable_provider - A handle for resource provider.
- @srcu: The SRCU to protect the resource.
- @res: The pointer of resource. It can point to anything.
- @kref: The refcount for this handle.
- */
+struct revocable_provider {
- struct srcu_struct srcu;
- void __rcu *res;
- struct kref kref;
+};
+/**
- struct revocable - A handle for resource consumer.
- @rp: The pointer of resource provider.
- @idx: The index for the RCU critical section.
- */
+struct revocable {
- struct revocable_provider *rp;
- int idx;
+};
+/**
- revocable_provider_alloc() - Allocate struct revocable_provider.
- @res: The pointer of resource.
- This holds an initial refcount to the struct.
- Return: The pointer of struct revocable_provider. NULL on errors.
- */
+struct revocable_provider *revocable_provider_alloc(void *res) +{
- struct revocable_provider *rp;
- rp = kzalloc(sizeof(*rp), GFP_KERNEL);
- if (!rp)
return NULL;- init_srcu_struct(&rp->srcu);
- rcu_assign_pointer(rp->res, res);
- synchronize_srcu(&rp->srcu);
- kref_init(&rp->kref);
- return rp;
+} +EXPORT_SYMBOL_GPL(revocable_provider_alloc);
+static void revocable_provider_release(struct kref *kref) +{
- struct revocable_provider *rp = container_of(kref,
struct revocable_provider, kref);- cleanup_srcu_struct(&rp->srcu);
- kfree(rp);
+}
+/**
- revocable_provider_free() - Free struct revocable_provider.
- @rp: The pointer of resource provider.
- This sets the resource `(struct revocable_provider *)->res` to NULL to
- indicate the resource has gone.
- This drops the refcount to the resource provider. If it is the final
- reference, revocable_provider_release() will be called to free the struct.
- */
+void revocable_provider_free(struct revocable_provider *rp) +{
- rcu_assign_pointer(rp->res, NULL);
- synchronize_srcu(&rp->srcu);
- kref_put(&rp->kref, revocable_provider_release);
+} +EXPORT_SYMBOL_GPL(revocable_provider_free);
+static void devm_revocable_provider_free(void *data) +{
- struct revocable_provider *rp = data;
- revocable_provider_free(rp);
+}
+/**
- devm_revocable_provider_alloc() - Dev-managed revocable_provider_alloc().
- @dev: The device.
- @res: The pointer of resource.
- It is convenient to allocate providers via this function if the @res is
- also tied to the lifetime of the @dev. revocable_provider_free() will
- be called automatically when the device is unbound.
- This holds an initial refcount to the struct.
- Return: The pointer of struct revocable_provider. NULL on errors.
- */
+struct revocable_provider *devm_revocable_provider_alloc(struct device *dev,
void *res)+{
- struct revocable_provider *rp;
- rp = revocable_provider_alloc(res);
- if (!rp)
return NULL;- if (devm_add_action_or_reset(dev, devm_revocable_provider_free, rp))
return NULL;- return rp;
+} +EXPORT_SYMBOL_GPL(devm_revocable_provider_alloc);
+/**
- revocable_alloc() - Allocate struct revocable_provider.
- @rp: The pointer of resource provider.
- This holds a refcount to the resource provider.
- Return: The pointer of struct revocable_provider. NULL on errors.
- */
+struct revocable *revocable_alloc(struct revocable_provider *rp) +{
- struct revocable *rev;
- rev = kzalloc(sizeof(*rev), GFP_KERNEL);
- if (!rev)
return NULL;- rev->rp = rp;
- kref_get(&rp->kref);
- return rev;
+} +EXPORT_SYMBOL_GPL(revocable_alloc);
+/**
- revocable_free() - Free struct revocable.
- @rev: The pointer of struct revocable.
- This drops a refcount to the resource provider. If it is the final
- reference, revocable_provider_release() will be called to free the struct.
- */
+void revocable_free(struct revocable *rev) +{
- struct revocable_provider *rp = rev->rp;
- kref_put(&rp->kref, revocable_provider_release);
- kfree(rev);
+} +EXPORT_SYMBOL_GPL(revocable_free);
+/**
- revocable_try_access() - Try to access the resource.
- @rev: The pointer of struct revocable.
- This tries to de-reference to the resource and enters a RCU critical
- section.
- Return: The pointer to the resource. NULL if the resource has gone.
- */
+void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu) +{
- struct revocable_provider *rp = rev->rp;
- rev->idx = srcu_read_lock(&rp->srcu);
- return rcu_dereference(rp->res);
+} +EXPORT_SYMBOL_GPL(revocable_try_access);
+/**
- revocable_release() - Stop accessing to the resource.
- @rev: The pointer of struct revocable.
- Call this function to indicate the resource is no longer used. It exits
- the RCU critical section.
- */
+void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu) +{
- struct revocable_provider *rp = rev->rp;
- srcu_read_unlock(&rp->srcu, rev->idx);
+} +EXPORT_SYMBOL_GPL(revocable_release); diff --git a/include/linux/revocable.h b/include/linux/revocable.h new file mode 100644 index 000000000000..17d9b7ce633d --- /dev/null +++ b/include/linux/revocable.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright 2025 Google LLC
- */
+#ifndef __LINUX_REVOCABLE_H +#define __LINUX_REVOCABLE_H
+#include <linux/cleanup.h>
+struct device; +struct revocable; +struct revocable_provider;
+struct revocable_provider *revocable_provider_alloc(void *res); +void revocable_provider_free(struct revocable_provider *rp); +struct revocable_provider *devm_revocable_provider_alloc(struct device *dev,
void *res);+struct revocable *revocable_alloc(struct revocable_provider *rp); +void revocable_free(struct revocable *rev); +void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu); +void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu);
+DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
+#define _REVOCABLE(_rev, _label, _res) \
- for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev; \
(_res = revocable_try_access(_rev)) || true; ({ goto _label; })) \if (0) { \+_label: \
break; \} else+#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)
+#endif /* __LINUX_REVOCABLE_H */
2.51.0.384.g4c02a37b29-goog
Add Kunit test cases for the revocable API.
The test cases cover the following scenarios: - Basic: Verifies that a consumer can successfully access the resource provided via the provider. - Revocation: Verifies that after the provider revokes the resource, the consumer correctly receives a NULL pointer on a subsequent access. - Macro: Same as "Revocation" but uses the REVOCABLE().
A way to run the test: $ ./tools/testing/kunit/kunit.py run \ --kconfig_add CONFIG_REVOCABLE_KUNIT_TEST=y \ revocable_test
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- v3: - No changes.
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-3-tzungbi@kern... - New in the series.
MAINTAINERS | 1 + drivers/base/Kconfig | 8 +++ drivers/base/Makefile | 3 + drivers/base/revocable_test.c | 110 ++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+) create mode 100644 drivers/base/revocable_test.c
diff --git a/MAINTAINERS b/MAINTAINERS index 5d11aeeb546e..74930474f1bd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21882,6 +21882,7 @@ M: Tzung-Bi Shih tzungbi@kernel.org L: linux-kernel@vger.kernel.org S: Maintained F: drivers/base/revocable.c +F: drivers/base/revocable_test.c F: include/linux/revocable.h
RFKILL diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 064eb52ff7e2..a6488035f63d 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -244,3 +244,11 @@ config FW_DEVLINK_SYNC_STATE_TIMEOUT work on.
endmenu + +# Kunit test cases +config REVOCABLE_KUNIT_TEST + tristate "Kunit tests for revocable" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + Kunit tests for the revocable API. diff --git a/drivers/base/Makefile b/drivers/base/Makefile index bdf854694e39..4185aaa9bbb9 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -35,3 +35,6 @@ ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG # define_trace.h needs to know how to find our header CFLAGS_trace.o := -I$(src) obj-$(CONFIG_TRACING) += trace.o + +# Kunit test cases +obj-$(CONFIG_REVOCABLE_KUNIT_TEST) += revocable_test.o diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c new file mode 100644 index 000000000000..d1ec7e47cd1d --- /dev/null +++ b/drivers/base/revocable_test.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2025 Google LLC + * + * Kunit tests for the revocable API. + * + * The test cases cover the following scenarios: + * + * - Basic: Verifies that a consumer can successfully access the resource + * provided via the provider. + * + * - Revocation: Verifies that after the provider revokes the resource, + * the consumer correctly receives a NULL pointer on a subsequent access. + * + * - Macro: Same as "Revocation" but uses the REVOCABLE(). + */ + +#include <kunit/test.h> +#include <linux/revocable.h> + +static void revocable_test_basic(struct kunit *test) +{ + struct revocable_provider *rp; + struct revocable *rev; + void *real_res = (void *)0x12345678, *res; + + rp = revocable_provider_alloc(real_res); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp); + + rev = revocable_alloc(rp); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev); + + res = revocable_try_access(rev); + KUNIT_EXPECT_PTR_EQ(test, res, real_res); + revocable_release(rev); + + revocable_free(rev); + revocable_provider_free(rp); +} + +static void revocable_test_revocation(struct kunit *test) +{ + struct revocable_provider *rp; + struct revocable *rev; + void *real_res = (void *)0x12345678, *res; + + rp = revocable_provider_alloc(real_res); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp); + + rev = revocable_alloc(rp); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev); + + res = revocable_try_access(rev); + KUNIT_EXPECT_PTR_EQ(test, res, real_res); + revocable_release(rev); + + revocable_provider_free(rp); + + res = revocable_try_access(rev); + KUNIT_EXPECT_PTR_EQ(test, res, NULL); + revocable_release(rev); + + revocable_free(rev); +} + +static void revocable_test_macro(struct kunit *test) +{ + struct revocable_provider *rp; + struct revocable *rev; + void *real_res = (void *)0x12345678, *res; + bool accessed; + + rp = revocable_provider_alloc(real_res); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp); + + rev = revocable_alloc(rp); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev); + + accessed = false; + REVOCABLE(rev, res) { + KUNIT_EXPECT_PTR_EQ(test, res, real_res); + accessed = true; + } + KUNIT_EXPECT_TRUE(test, accessed); + + revocable_provider_free(rp); + + accessed = false; + REVOCABLE(rev, res) { + KUNIT_EXPECT_PTR_EQ(test, res, NULL); + accessed = true; + } + KUNIT_EXPECT_TRUE(test, accessed); + + revocable_free(rev); +} + +static struct kunit_case revocable_test_cases[] = { + KUNIT_CASE(revocable_test_basic), + KUNIT_CASE(revocable_test_revocation), + KUNIT_CASE(revocable_test_macro), + {} +}; + +static struct kunit_suite revocable_test_suite = { + .name = "revocable_test", + .test_cases = revocable_test_cases, +}; + +kunit_test_suite(revocable_test_suite);
Add kselftest cases for the revocable API.
The test consists of three parts: - A kernel module (revocable_test.ko) that creates a debugfs interface with `/provider` and `/consumer` files. - A user-space C program (revocable_test) that uses the kselftest harness to interact with the debugfs files. - An orchestrating shell script (test-revocable.sh) that loads the module, runs the C program, and unloads the module.
The test cases cover the following scenarios: - Basic: Verifies that a consumer can successfully access the resource provided via the provider. - Revocation: Verifies that after the provider revokes the resource, the consumer correctly receives a NULL pointer on a subsequent access. - Macro: Same as "Revocation" but uses the REVOCABLE().
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- v3: - No changes.
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-4-tzungbi@kern... - New in the series.
MAINTAINERS | 1 + tools/testing/selftests/Makefile | 1 + .../selftests/drivers/base/revocable/Makefile | 7 + .../drivers/base/revocable/revocable_test.c | 116 +++++++++++ .../drivers/base/revocable/test-revocable.sh | 39 ++++ .../base/revocable/test_modules/Makefile | 10 + .../revocable/test_modules/revocable_test.c | 188 ++++++++++++++++++ 7 files changed, 362 insertions(+) create mode 100644 tools/testing/selftests/drivers/base/revocable/Makefile create mode 100644 tools/testing/selftests/drivers/base/revocable/revocable_test.c create mode 100755 tools/testing/selftests/drivers/base/revocable/test-revocable.sh create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/Makefile create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
diff --git a/MAINTAINERS b/MAINTAINERS index 74930474f1bd..583d818262c4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21884,6 +21884,7 @@ S: Maintained F: drivers/base/revocable.c F: drivers/base/revocable_test.c F: include/linux/revocable.h +F: tools/testing/selftests/drivers/base/revocable/
RFKILL M: Johannes Berg johannes@sipsolutions.net diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 850dacb09929..d9700ce9eb64 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -17,6 +17,7 @@ TARGETS += damon TARGETS += devices/error_logs TARGETS += devices/probe TARGETS += dmabuf-heaps +TARGETS += drivers/base/revocable TARGETS += drivers/dma-buf TARGETS += drivers/ntsync TARGETS += drivers/s390x/uvdevice diff --git a/tools/testing/selftests/drivers/base/revocable/Makefile b/tools/testing/selftests/drivers/base/revocable/Makefile new file mode 100644 index 000000000000..afa5ca0fa452 --- /dev/null +++ b/tools/testing/selftests/drivers/base/revocable/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0 + +TEST_GEN_MODS_DIR := test_modules +TEST_GEN_PROGS_EXTENDED := revocable_test +TEST_PROGS := test-revocable.sh + +include ../../../lib.mk diff --git a/tools/testing/selftests/drivers/base/revocable/revocable_test.c b/tools/testing/selftests/drivers/base/revocable/revocable_test.c new file mode 100644 index 000000000000..cdef49dc4095 --- /dev/null +++ b/tools/testing/selftests/drivers/base/revocable/revocable_test.c @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2025 Google LLC + * + * A selftest for the revocable API. + * + * The test cases cover the following scenarios: + * + * - Basic: Verifies that a consumer can successfully access the resource + * provided via the provider. + * + * - Revocation: Verifies that after the provider revokes the resource, + * the consumer correctly receives a NULL pointer on a subsequent access. + * + * - Macro: Same as "Revocation" but uses the REVOCABLE(). + */ + +#include <fcntl.h> +#include <unistd.h> + +#include "../../../kselftest_harness.h" + +#define DEBUGFS_PATH "/sys/kernel/debug/revocable_test" +#define TEST_CMD_RESOURCE_GONE "resource_gone" +#define TEST_DATA "12345678" +#define TEST_MAGIC_OFFSET 0x1234 + +FIXTURE(revocable_fixture) { + int pfd; + int cfd; +}; + +FIXTURE_SETUP(revocable_fixture) { + int ret; + + self->pfd = open(DEBUGFS_PATH "/provider", O_WRONLY); + ASSERT_NE(-1, self->pfd) + TH_LOG("failed to open provider fd"); + + ret = write(self->pfd, TEST_DATA, strlen(TEST_DATA)); + ASSERT_NE(-1, ret) { + close(self->pfd); + TH_LOG("failed to write test data"); + } + + self->cfd = open(DEBUGFS_PATH "/consumer", O_RDONLY); + ASSERT_NE(-1, self->cfd) + TH_LOG("failed to open consumer fd"); +} + +FIXTURE_TEARDOWN(revocable_fixture) { + close(self->cfd); + close(self->pfd); +} + +/* + * ASSERT_* is only available in TEST or TEST_F block. Use + * macro for the helper. + */ +#define READ_TEST_DATA(_fd, _offset, _data, _msg) \ + do { \ + int ret; \ + \ + ret = lseek(_fd, _offset, SEEK_SET); \ + ASSERT_NE(-1, ret) \ + TH_LOG("failed to lseek"); \ + \ + ret = read(_fd, _data, sizeof(_data) - 1); \ + ASSERT_NE(-1, ret) \ + TH_LOG(_msg); \ + data[ret] = '\0'; \ + } while (0) + +TEST_F(revocable_fixture, basic) { + char data[16]; + + READ_TEST_DATA(self->cfd, 0, data, "failed to read test data"); + EXPECT_STREQ(TEST_DATA, data); +} + +TEST_F(revocable_fixture, revocation) { + char data[16]; + int ret; + + READ_TEST_DATA(self->cfd, 0, data, "failed to read test data"); + EXPECT_STREQ(TEST_DATA, data); + + ret = write(self->pfd, TEST_CMD_RESOURCE_GONE, + strlen(TEST_CMD_RESOURCE_GONE)); + ASSERT_NE(-1, ret) + TH_LOG("failed to write resource gone cmd"); + + READ_TEST_DATA(self->cfd, 0, data, + "failed to read test data after resource gone"); + EXPECT_STREQ("(null)", data); +} + +TEST_F(revocable_fixture, macro) { + char data[16]; + int ret; + + READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET, data, + "failed to read test data"); + EXPECT_STREQ(TEST_DATA, data); + + ret = write(self->pfd, TEST_CMD_RESOURCE_GONE, + strlen(TEST_CMD_RESOURCE_GONE)); + ASSERT_NE(-1, ret) + TH_LOG("failed to write resource gone cmd"); + + READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET, data, + "failed to read test data after resource gone"); + EXPECT_STREQ("(null)", data); +} + +TEST_HARNESS_MAIN diff --git a/tools/testing/selftests/drivers/base/revocable/test-revocable.sh b/tools/testing/selftests/drivers/base/revocable/test-revocable.sh new file mode 100755 index 000000000000..3a34be28001a --- /dev/null +++ b/tools/testing/selftests/drivers/base/revocable/test-revocable.sh @@ -0,0 +1,39 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +mod_name="revocable_test" +ksft_fail=1 +ksft_skip=4 + +if [ "$(id -u)" -ne 0 ]; then + echo "$0: Must be run as root" + exit "$ksft_skip" +fi + +if ! which insmod > /dev/null 2>&1; then + echo "$0: Need insmod" + exit "$ksft_skip" +fi + +if ! which rmmod > /dev/null 2>&1; then + echo "$0: Need rmmod" + exit "$ksft_skip" +fi + +insmod test_modules/"${mod_name}".ko + +if [ ! -d /sys/kernel/debug/revocable_test/ ]; then + mount -t debugfs none /sys/kernel/debug/ + + if [ ! -d /sys/kernel/debug/revocable_test/ ]; then + echo "$0: Error mounting debugfs" + exit "$ksft_fail" + fi +fi + +./revocable_test +ret=$? + +rmmod "${mod_name}" + +exit "${ret}" diff --git a/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile b/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile new file mode 100644 index 000000000000..f29e4f909402 --- /dev/null +++ b/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile @@ -0,0 +1,10 @@ +TESTMODS_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST))))) +KDIR ?= /lib/modules/$(shell uname -r)/build + +obj-m += revocable_test.o + +all: + $(Q)$(MAKE) -C $(KDIR) M=$(TESTMODS_DIR) + +clean: + $(Q)$(MAKE) -C $(KDIR) M=$(TESTMODS_DIR) clean diff --git a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c new file mode 100644 index 000000000000..9166b860a55a --- /dev/null +++ b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2025 Google LLC + * + * A kernel module for testing the revocable API. + */ + +#include <linux/debugfs.h> +#include <linux/module.h> +#include <linux/revocable.h> +#include <linux/slab.h> + +#define TEST_CMD_RESOURCE_GONE "resource_gone" +#define TEST_MAGIC_OFFSET 0x1234 + +static struct dentry *debugfs_dir; + +struct revocable_test_provider_priv { + struct revocable_provider *rp; + struct dentry *dentry; + char res[16]; +}; + +static int revocable_test_consumer_open(struct inode *inode, struct file *filp) +{ + struct revocable *rev; + struct revocable_provider *rp = inode->i_private; + + rev = revocable_alloc(rp); + if (!rev) + return -ENOMEM; + filp->private_data = rev; + + return 0; +} + +static int revocable_test_consumer_release(struct inode *inode, + struct file *filp) +{ + struct revocable *rev = filp->private_data; + + revocable_free(rev); + return 0; +} + +static ssize_t revocable_test_consumer_read(struct file *filp, + char __user *buf, + size_t count, loff_t *offset) +{ + char *res; + char data[16]; + size_t len; + struct revocable *rev = filp->private_data; + + switch (*offset) { + case 0: + res = revocable_try_access(rev); + snprintf(data, sizeof(data), "%s", res ?: "(null)"); + revocable_release(rev); + break; + case TEST_MAGIC_OFFSET: + REVOCABLE(rev, res) + snprintf(data, sizeof(data), "%s", res ?: "(null)"); + break; + default: + return 0; + } + + len = min_t(size_t, strlen(data), count); + if (copy_to_user(buf, data, len)) + return -EFAULT; + + *offset = len; + return len; +} + +static const struct file_operations revocable_test_consumer_fops = { + .open = revocable_test_consumer_open, + .release = revocable_test_consumer_release, + .read = revocable_test_consumer_read, + .llseek = default_llseek, +}; + +static int revocable_test_provider_open(struct inode *inode, struct file *filp) +{ + struct revocable_test_provider_priv *priv; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + filp->private_data = priv; + + return 0; +} + +static int revocable_test_provider_release(struct inode *inode, + struct file *filp) +{ + struct revocable_test_provider_priv *priv = filp->private_data; + + debugfs_remove(priv->dentry); + if (priv->rp) + revocable_provider_free(priv->rp); + kfree(priv); + + return 0; +} + +static ssize_t revocable_test_provider_write(struct file *filp, + const char __user *buf, + size_t count, loff_t *offset) +{ + size_t copied; + char data[64]; + struct revocable_test_provider_priv *priv = filp->private_data; + + copied = strncpy_from_user(data, buf, sizeof(data)); + if (copied < 0) + return copied; + if (copied == sizeof(data)) + data[sizeof(data) - 1] = '\0'; + + /* + * Note: The test can't just close the FD for signaling the + * resource gone. Subsequent file operations on the opening + * FD of debugfs return -EIO after calling debugfs_remove(). + * See also debugfs_file_get(). + * + * Here is a side command channel for signaling the resource + * gone. + */ + if (!strcmp(data, TEST_CMD_RESOURCE_GONE)) { + revocable_provider_free(priv->rp); + priv->rp = NULL; + } else { + if (priv->res[0] != '\0') + return 0; + + strscpy(priv->res, data); + + priv->rp = revocable_provider_alloc(&priv->res); + if (!priv->rp) + return -ENOMEM; + + priv->dentry = debugfs_create_file("consumer", 0400, + debugfs_dir, priv->rp, + &revocable_test_consumer_fops); + if (!priv->dentry) { + revocable_provider_free(priv->rp); + return -ENOMEM; + } + } + + return copied; +} + +static const struct file_operations revocable_test_provider_fops = { + .open = revocable_test_provider_open, + .release = revocable_test_provider_release, + .write = revocable_test_provider_write, +}; + +static int __init revocable_test_init(void) +{ + debugfs_dir = debugfs_create_dir("revocable_test", NULL); + if (!debugfs_dir) + return -ENOMEM; + + if (!debugfs_create_file("provider", 0200, debugfs_dir, NULL, + &revocable_test_provider_fops)) { + debugfs_remove_recursive(debugfs_dir); + return -ENOMEM; + } + + return 0; +} + +static void __exit revocable_test_exit(void) +{ + debugfs_remove_recursive(debugfs_dir); +} + +module_init(revocable_test_init); +module_exit(revocable_test_exit); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Tzung-Bi Shih tzungbi@kernel.org"); +MODULE_DESCRIPTION("Revocable Kselftest");
The cros_ec_device can be unregistered when the underlying device is removed. Other kernel drivers that interact with the EC may hold a pointer to the cros_ec_device, creating a risk of a use-after-free error if the EC device is removed while still being referenced.
To prevent this, leverage the revocable and convert the underlying device drivers to resource providers of cros_ec_device.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- v3: - Initialize the revocable provider in cros_ec_device_alloc() instead of spreading in protocol device drivers.
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-5-tzungbi@kern... - Rename "ref_proxy" -> "revocable".
v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-3-tzungbi@ker...
drivers/platform/chrome/cros_ec.c | 5 +++++ include/linux/platform_data/cros_ec_proto.h | 4 ++++ 2 files changed, 9 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c index 1da79e3d215b..95e3e898e3da 100644 --- a/drivers/platform/chrome/cros_ec.c +++ b/drivers/platform/chrome/cros_ec.c @@ -16,6 +16,7 @@ #include <linux/platform_device.h> #include <linux/platform_data/cros_ec_commands.h> #include <linux/platform_data/cros_ec_proto.h> +#include <linux/revocable.h> #include <linux/slab.h> #include <linux/suspend.h>
@@ -47,6 +48,10 @@ struct cros_ec_device *cros_ec_device_alloc(struct device *dev) if (!ec_dev) return NULL;
+ ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev); + if (!ec_dev->revocable_provider) + return NULL; + ec_dev->din_size = sizeof(struct ec_host_response) + sizeof(struct ec_response_get_protocol_info) + EC_MAX_RESPONSE_OVERHEAD; diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h index de14923720a5..fbb6ca34a40f 100644 --- a/include/linux/platform_data/cros_ec_proto.h +++ b/include/linux/platform_data/cros_ec_proto.h @@ -12,6 +12,7 @@ #include <linux/lockdep_types.h> #include <linux/mutex.h> #include <linux/notifier.h> +#include <linux/revocable.h>
#include <linux/platform_data/cros_ec_commands.h>
@@ -165,6 +166,7 @@ struct cros_ec_command { * @pd: The platform_device used by the mfd driver to interface with the * PD behind an EC. * @panic_notifier: EC panic notifier. + * @revocable_provider: The revocable_provider to this device. */ struct cros_ec_device { /* These are used by other drivers that want to talk to the EC */ @@ -211,6 +213,8 @@ struct cros_ec_device { struct platform_device *pd;
struct blocking_notifier_head panic_notifier; + + struct revocable_provider *revocable_provider; };
/**
The cros_ec_chardev driver provides a character device interface to the ChromeOS EC. A file handle to this device can remain open in userspace even if the underlying EC device is removed.
This creates a classic use-after-free vulnerability. Any file operation (ioctl, release, etc.) on the open handle after the EC device has gone would access a stale pointer, leading to a system crash.
To prevent this, leverage the revocable and convert cros_ec_chardev to a resource consumer of cros_ec_device.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- v3: - Use specific labels for different cleanup in cros_ec_chardev_open().
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-6-tzungbi@kern... - Rename "ref_proxy" -> "revocable". - Fix a sparse warning by removing the redundant __rcu annotation.
v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-4-tzungbi@ker...
drivers/platform/chrome/cros_ec_chardev.c | 124 +++++++++++++++------- 1 file changed, 84 insertions(+), 40 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c index c9d80ad5b57e..2677b756e31c 100644 --- a/drivers/platform/chrome/cros_ec_chardev.c +++ b/drivers/platform/chrome/cros_ec_chardev.c @@ -22,6 +22,7 @@ #include <linux/platform_data/cros_ec_proto.h> #include <linux/platform_device.h> #include <linux/poll.h> +#include <linux/revocable.h> #include <linux/slab.h> #include <linux/types.h> #include <linux/uaccess.h> @@ -32,7 +33,7 @@ #define CROS_MAX_EVENT_LEN PAGE_SIZE
struct chardev_priv { - struct cros_ec_device *ec_dev; + struct revocable *ec_dev_rev; struct notifier_block notifier; wait_queue_head_t wait_event; unsigned long event_mask; @@ -55,6 +56,7 @@ static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen) }; struct ec_response_get_version *resp; struct cros_ec_command *msg; + struct cros_ec_device *ec_dev; int ret;
msg = kzalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL); @@ -64,12 +66,19 @@ static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen) msg->command = EC_CMD_GET_VERSION + priv->cmd_offset; msg->insize = sizeof(*resp);
- ret = cros_ec_cmd_xfer_status(priv->ec_dev, msg); - if (ret < 0) { - snprintf(str, maxlen, - "Unknown EC version, returned error: %d\n", - msg->result); - goto exit; + REVOCABLE(priv->ec_dev_rev, ec_dev) { + if (!ec_dev) { + ret = -ENODEV; + goto exit; + } + + ret = cros_ec_cmd_xfer_status(ec_dev, msg); + if (ret < 0) { + snprintf(str, maxlen, + "Unknown EC version, returned error: %d\n", + msg->result); + goto exit; + } }
resp = (struct ec_response_get_version *)msg->data; @@ -92,22 +101,30 @@ static int cros_ec_chardev_mkbp_event(struct notifier_block *nb, { struct chardev_priv *priv = container_of(nb, struct chardev_priv, notifier); - struct cros_ec_device *ec_dev = priv->ec_dev; + struct cros_ec_device *ec_dev; struct ec_event *event; - unsigned long event_bit = 1 << ec_dev->event_data.event_type; - int total_size = sizeof(*event) + ec_dev->event_size; + unsigned long event_bit; + int total_size; + + REVOCABLE(priv->ec_dev_rev, ec_dev) { + if (!ec_dev) + return NOTIFY_DONE; + + event_bit = 1 << ec_dev->event_data.event_type; + total_size = sizeof(*event) + ec_dev->event_size;
- if (!(event_bit & priv->event_mask) || - (priv->event_len + total_size) > CROS_MAX_EVENT_LEN) - return NOTIFY_DONE; + if (!(event_bit & priv->event_mask) || + (priv->event_len + total_size) > CROS_MAX_EVENT_LEN) + return NOTIFY_DONE;
- event = kzalloc(total_size, GFP_KERNEL); - if (!event) - return NOTIFY_DONE; + event = kzalloc(total_size, GFP_KERNEL); + if (!event) + return NOTIFY_DONE;
- event->size = ec_dev->event_size; - event->event_type = ec_dev->event_data.event_type; - memcpy(event->data, &ec_dev->event_data.data, ec_dev->event_size); + event->size = ec_dev->event_size; + event->event_type = ec_dev->event_data.event_type; + memcpy(event->data, &ec_dev->event_data.data, ec_dev->event_size); + }
spin_lock(&priv->wait_event.lock); list_add_tail(&event->node, &priv->events); @@ -166,7 +183,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp) if (!priv) return -ENOMEM;
- priv->ec_dev = ec_dev; + priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider); + if (!priv->ec_dev_rev) { + ret = -ENOMEM; + goto free_priv; + } + priv->cmd_offset = ec->cmd_offset; filp->private_data = priv; INIT_LIST_HEAD(&priv->events); @@ -178,9 +200,14 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp) &priv->notifier); if (ret) { dev_err(ec_dev->dev, "failed to register event notifier\n"); - kfree(priv); + goto free_revocable; }
+ return 0; +free_revocable: + revocable_free(priv->ec_dev_rev); +free_priv: + kfree(priv); return ret; }
@@ -251,11 +278,15 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer, static int cros_ec_chardev_release(struct inode *inode, struct file *filp) { struct chardev_priv *priv = filp->private_data; - struct cros_ec_device *ec_dev = priv->ec_dev; + struct cros_ec_device *ec_dev; struct ec_event *event, *e;
- blocking_notifier_chain_unregister(&ec_dev->event_notifier, - &priv->notifier); + REVOCABLE(priv->ec_dev_rev, ec_dev) { + if (ec_dev) + blocking_notifier_chain_unregister(&ec_dev->event_notifier, + &priv->notifier); + } + revocable_free(priv->ec_dev_rev);
list_for_each_entry_safe(event, e, &priv->events, node) { list_del(&event->node); @@ -273,6 +304,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *a { struct cros_ec_command *s_cmd; struct cros_ec_command u_cmd; + struct cros_ec_device *ec_dev; long ret;
if (copy_from_user(&u_cmd, arg, sizeof(u_cmd))) @@ -299,10 +331,17 @@ static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *a }
s_cmd->command += priv->cmd_offset; - ret = cros_ec_cmd_xfer(priv->ec_dev, s_cmd); - /* Only copy data to userland if data was received. */ - if (ret < 0) - goto exit; + REVOCABLE(priv->ec_dev_rev, ec_dev) { + if (!ec_dev) { + ret = -ENODEV; + goto exit; + } + + ret = cros_ec_cmd_xfer(ec_dev, s_cmd); + /* Only copy data to userland if data was received. */ + if (ret < 0) + goto exit; + }
if (copy_to_user(arg, s_cmd, sizeof(*s_cmd) + s_cmd->insize)) ret = -EFAULT; @@ -313,24 +352,29 @@ static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *a
static long cros_ec_chardev_ioctl_readmem(struct chardev_priv *priv, void __user *arg) { - struct cros_ec_device *ec_dev = priv->ec_dev; + struct cros_ec_device *ec_dev; struct cros_ec_readmem s_mem = { }; long num;
- /* Not every platform supports direct reads */ - if (!ec_dev->cmd_readmem) - return -ENOTTY; + REVOCABLE(priv->ec_dev_rev, ec_dev) { + if (!ec_dev) + return -ENODEV;
- if (copy_from_user(&s_mem, arg, sizeof(s_mem))) - return -EFAULT; + /* Not every platform supports direct reads */ + if (!ec_dev->cmd_readmem) + return -ENOTTY;
- if (s_mem.bytes > sizeof(s_mem.buffer)) - return -EINVAL; + if (copy_from_user(&s_mem, arg, sizeof(s_mem))) + return -EFAULT;
- num = ec_dev->cmd_readmem(ec_dev, s_mem.offset, s_mem.bytes, - s_mem.buffer); - if (num <= 0) - return num; + if (s_mem.bytes > sizeof(s_mem.buffer)) + return -EINVAL; + + num = ec_dev->cmd_readmem(ec_dev, s_mem.offset, s_mem.bytes, + s_mem.buffer); + if (num <= 0) + return num; + }
if (copy_to_user((void __user *)arg, &s_mem, sizeof(s_mem))) return -EFAULT;
On Fri, Sep 12, 2025 at 08:17:12AM +0000, Tzung-Bi Shih wrote:
This is a follow-up series of [1]. It tries to fix a possible UAF in the fops of cros_ec_chardev after the underlying protocol device has gone by using revocable.
The 1st patch introduces the revocable which is an implementation of ideas from the talk [2].
The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
The 4th patch converts existing protocol devices to resource providers of cros_ec_device.
The 5th patch converts cros_ec_chardev to a resource consumer of cros_ec_device to fix the UAF.
[1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@ker... [2] https://lpc.events/event/17/contributions/1627/
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
This is, frankly, wonderful work. Thanks so much for doing this, it's what many of us have been wanting to see for a very long time but none of us got around to actually doing it.
And it has tests! And documentation! Couldn't ask for more.
We can bikeshed about the REVOCABLE() macro name, but frankly, you wrote it, you get to pick it :)
Laurent, Bartosz, Wolfram, any objection to this series? I think this addresses the issues that all of you have been raising for years with our access of pointers that have different lifecycles from other structures (i.e. struct cdev from struct device).
Also, Danilo, if you get the chance, can you give this a review as well? At first glance it looks good to me, but as you wrote the Rust implementation of this feature, a second pair of eyes would be great to have if you have the time.
thanks,
greg k-h
On 9/12/25 10:30 AM, Greg Kroah-Hartman wrote:
Also, Danilo, if you get the chance, can you give this a review as well? At first glance it looks good to me, but as you wrote the Rust implementation of this feature, a second pair of eyes would be great to have if you have the time.
Sure, I will follow up on this later today.
On Fri, Sep 12, 2025 at 10:30:45AM +0200, Greg KH wrote:
On Fri, Sep 12, 2025 at 08:17:12AM +0000, Tzung-Bi Shih wrote:
This is a follow-up series of [1]. It tries to fix a possible UAF in the fops of cros_ec_chardev after the underlying protocol device has gone by using revocable.
The 1st patch introduces the revocable which is an implementation of ideas from the talk [2].
The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
The 4th patch converts existing protocol devices to resource providers of cros_ec_device.
The 5th patch converts cros_ec_chardev to a resource consumer of cros_ec_device to fix the UAF.
[1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@ker... [2] https://lpc.events/event/17/contributions/1627/
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
This is, frankly, wonderful work. Thanks so much for doing this, it's what many of us have been wanting to see for a very long time but none of us got around to actually doing it.
And it has tests! And documentation! Couldn't ask for more.
We can bikeshed about the REVOCABLE() macro name, but frankly, you wrote it, you get to pick it :)
Laurent, Bartosz, Wolfram, any objection to this series? I think this addresses the issues that all of you have been raising for years with our access of pointers that have different lifecycles from other structures (i.e. struct cdev from struct device).
I'll check this either later today or over the weekend.
Also, Danilo, if you get the chance, can you give this a review as well? At first glance it looks good to me, but as you wrote the Rust implementation of this feature, a second pair of eyes would be great to have if you have the time.
On 12/09/2025 10:17, Tzung-Bi Shih wrote:
This is a follow-up series of [1]. It tries to fix a possible UAF in the fops of cros_ec_chardev after the underlying protocol device has gone by using revocable.
The 1st patch introduces the revocable which is an implementation of ideas from the talk [2].
The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
The 4th patch converts existing protocol devices to resource providers of cros_ec_device.
The 5th patch converts cros_ec_chardev to a resource consumer of cros_ec_device to fix the UAF.
[1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@ker... [2] https://lpc.events/event/17/contributions/1627/
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
Thanks for the work. Just a note, please start using b4, so above Cc will be propagated to all patches. Folks above received only the cover letter...
Best regards, Krzysztof
On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski krzk@kernel.org wrote:
On 12/09/2025 10:17, Tzung-Bi Shih wrote:
This is a follow-up series of [1]. It tries to fix a possible UAF in the fops of cros_ec_chardev after the underlying protocol device has gone by using revocable.
The 1st patch introduces the revocable which is an implementation of ideas from the talk [2].
The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
The 4th patch converts existing protocol devices to resource providers of cros_ec_device.
The 5th patch converts cros_ec_chardev to a resource consumer of cros_ec_device to fix the UAF.
[1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@ker... [2] https://lpc.events/event/17/contributions/1627/
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
Thanks for the work. Just a note, please start using b4, so above Cc will be propagated to all patches. Folks above received only the cover letter...
Thanks to Krzysztof for making me aware of this. Could you please Cc my brgl@bgdev.pl address on the next iteration.
I haven't looked into the details yet but the small size of the first patch strikes me as odd. The similar changes I did for GPIO were quite big and they were designed just for a single sub-system.
During the talk you reference, after I suggested a library like this, Greg KH can be heard saying: do this for two big subsystems so that you're sure it's a generic solution. Here you're only using it in a single driver which makes me wonder if we can actually use it to improve bigger offenders, like for example I2C, or even replace the custom, SRCU-based solution in GPIO we have now. Have you considered at least doing a PoC in a wider kernel framework?
Just my two cents.
Bartosz
On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski krzk@kernel.org wrote:
On 12/09/2025 10:17, Tzung-Bi Shih wrote:
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
Thanks for the work. Just a note, please start using b4, so above Cc will be propagated to all patches. Folks above received only the cover letter...
Thank you for bringing this to my attention. I wasn't aware of that and will ensure this is handled correctly in the future.
Thanks to Krzysztof for making me aware of this. Could you please Cc my brgl@bgdev.pl address on the next iteration.
Sure, will do.
I haven't looked into the details yet but the small size of the first patch strikes me as odd. The similar changes I did for GPIO were quite big and they were designed just for a single sub-system.
During the talk you reference, after I suggested a library like this, Greg KH can be heard saying: do this for two big subsystems so that you're sure it's a generic solution. Here you're only using it in a single driver which makes me wonder if we can actually use it to improve bigger offenders, like for example I2C, or even replace the custom, SRCU-based solution in GPIO we have now. Have you considered at least doing a PoC in a wider kernel framework?
Yes, I'm happy to take this on.
To help me get started, could you please point me to some relevant code locations? Also, could you let me know if any specific physical devices will be needed for testing?
On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
On 12/09/2025 10:17, Tzung-Bi Shih wrote:
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
Thanks for the work. Just a note, please start using b4, so above Cc will be propagated to all patches. Folks above received only the cover letter...
Thank you for bringing this to my attention. I wasn't aware of that and will ensure this is handled correctly in the future.
Thanks to Krzysztof for making me aware of this. Could you please Cc my brgl@bgdev.pl address on the next iteration.
Sure, will do.
I haven't looked into the details yet but the small size of the first patch strikes me as odd. The similar changes I did for GPIO were quite big and they were designed just for a single sub-system.
During the talk you reference, after I suggested a library like this, Greg KH can be heard saying: do this for two big subsystems so that you're sure it's a generic solution. Here you're only using it in a single driver which makes me wonder if we can actually use it to improve bigger offenders, like for example I2C, or even replace the custom, SRCU-based solution in GPIO we have now. Have you considered at least doing a PoC in a wider kernel framework?
Yes, I'm happy to take this on.
To help me get started, could you please point me to some relevant code locations? Also, could you let me know if any specific physical devices will be needed for testing?
One interesting test would be to move the logic to the cdev layer. The use-after-free problem isn't specific to one type of character device, and so shouldn't require a fix in every driver instantiating a cdev directly (or indirectly). See [1] for a previous attempt to handle this at the V4L2 level and [2] for an attempt to handle it at the cdev level.
In [1], two new functions named video_device_enter() and video_device_exit() flag the beginning and end of protected code sections. The equivalent in [2] is the manual get/put of cdev->qactive, and if I understand things correctly, your series creates a REVOCABLE() macro to do the same. I'm sure we'll bikesheed about names at some point, but for the time being, what I'd like to see if this being done in fs/char_dev.c to cover all entry points from userspace at the cdev level.
We then have video_device_unplug() in [1], which I think is more or less the equivalent of revocable_provider_free(). I don't think we'll be able to hide this completely from drivers, at least not in all cases. We should however design the API to make it easy for drivers, likely with subsystem-specific wrappers.
What I have in mind is roughly the following:
1. Protect all access to the cdev from userspace with enter/exit calls that flag if a call is in progress. This can be done with explicit function calls, or with a scope guard as in your series.
2. At .remove() time, start by flagging that the device is being removed. That has to be an explicit call from drivers I believe, likely using subsystem-specific wrappers to simplify things.
3. Once the device is marked as being removed, all enter() calls should fail at the cdev level.
4. In .remove(), proceed to perform driver-specific operations that will stop the device and wake up any userspace task blocked on a syscall protected by enter()/remove(). This isn't needed for drivers/subsystems that don't provide any blocking API, but is required otherwise.
5. Unregister, still in .remove(), the cdev (likely through subsystem-specific APIs in most cases). This should block until all protected sections have exited.
6. The cdev is now unregistered, can't be opened anymore, and any new syscall on any opened file handle will return an error. The driver's .remove() function can proceed to free data, there won't be any UAF caused by userspace.
[1] implemented this fairly naively with flags and spinlocks. An RCU-based implementation is probably more efficient, even if I don't know how performance-sensitive all this is.
Does this align with your design, and do you think you could give a try at pushing revocable resource handling to the cdev level ?
On a separate note, I'm not sure "revocable" is the right name here. I believe a revocable resource API is needed, and well-named, for in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the userspace syscalls racing with .remove(), I don't think we're dealing with "revocable resources". Now, if a "revocable resources" API were to support the in-kernel users, and be usable as a building block to fix the cdev issue, I would have nothing against it, but the "revocable" name should be internal in that case, used in the cdev layer only, and not exposed to drivers (or even subsystem helpers that should wrap cdev functions instead).
[1] https://lore.kernel.org/r/20171116003349.19235-1-laurent.pinchart+renesas@id... [2] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwi...
On Fri, Sep 12, 2025 at 04:26:56PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
On 12/09/2025 10:17, Tzung-Bi Shih wrote:
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
Thanks for the work. Just a note, please start using b4, so above Cc will be propagated to all patches. Folks above received only the cover letter...
Thank you for bringing this to my attention. I wasn't aware of that and will ensure this is handled correctly in the future.
Thanks to Krzysztof for making me aware of this. Could you please Cc my brgl@bgdev.pl address on the next iteration.
Sure, will do.
I haven't looked into the details yet but the small size of the first patch strikes me as odd. The similar changes I did for GPIO were quite big and they were designed just for a single sub-system.
During the talk you reference, after I suggested a library like this, Greg KH can be heard saying: do this for two big subsystems so that you're sure it's a generic solution. Here you're only using it in a single driver which makes me wonder if we can actually use it to improve bigger offenders, like for example I2C, or even replace the custom, SRCU-based solution in GPIO we have now. Have you considered at least doing a PoC in a wider kernel framework?
Yes, I'm happy to take this on.
To help me get started, could you please point me to some relevant code locations? Also, could you let me know if any specific physical devices will be needed for testing?
One interesting test would be to move the logic to the cdev layer. The use-after-free problem isn't specific to one type of character device, and so shouldn't require a fix in every driver instantiating a cdev directly (or indirectly). See [1] for a previous attempt to handle this at the V4L2 level and [2] for an attempt to handle it at the cdev level.
In [1], two new functions named video_device_enter() and video_device_exit() flag the beginning and end of protected code sections. The equivalent in [2] is the manual get/put of cdev->qactive, and if I understand things correctly, your series creates a REVOCABLE() macro to do the same. I'm sure we'll bikesheed about names at some point, but for the time being, what I'd like to see if this being done in fs/char_dev.c to cover all entry points from userspace at the cdev level.
We then have video_device_unplug() in [1], which I think is more or less the equivalent of revocable_provider_free(). I don't think we'll be able to hide this completely from drivers, at least not in all cases. We should however design the API to make it easy for drivers, likely with subsystem-specific wrappers.
What I have in mind is roughly the following:
Protect all access to the cdev from userspace with enter/exit calls that flag if a call is in progress. This can be done with explicit function calls, or with a scope guard as in your series.
At .remove() time, start by flagging that the device is being removed. That has to be an explicit call from drivers I believe, likely using subsystem-specific wrappers to simplify things.
Once the device is marked as being removed, all enter() calls should fail at the cdev level.
In .remove(), proceed to perform driver-specific operations that will stop the device and wake up any userspace task blocked on a syscall protected by enter()/remove(). This isn't needed for drivers/subsystems that don't provide any blocking API, but is required otherwise.
Unregister, still in .remove(), the cdev (likely through subsystem-specific APIs in most cases). This should block until all protected sections have exited.
The cdev is now unregistered, can't be opened anymore, and any new syscall on any opened file handle will return an error. The driver's .remove() function can proceed to free data, there won't be any UAF caused by userspace.
[1] implemented this fairly naively with flags and spinlocks. An RCU-based implementation is probably more efficient, even if I don't know how performance-sensitive all this is.
Does this align with your design, and do you think you could give a try at pushing revocable resource handling to the cdev level ?
On a separate note, I'm not sure "revocable" is the right name here. I believe a revocable resource API is needed, and well-named, for in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the userspace syscalls racing with .remove(), I don't think we're dealing with "revocable resources". Now, if a "revocable resources" API were to support the in-kernel users, and be usable as a building block to fix the cdev issue, I would have nothing against it, but the "revocable" name should be internal in that case, used in the cdev layer only, and not exposed to drivers (or even subsystem helpers that should wrap cdev functions instead).
I think the name makes sense as it matches up with how things are working (the backend structure is "revoked"), but naming is tough :)
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
And that's fine, we have "interns" that we can set loose on this type of code conversions, I think we just need to wrap the access to the cdev with this api, which will take a bit of rewriting in many drivers.
Anyway, just my thought, if someone else can see how this could drop into the core cdev code without any changes needed, that would be great, but I don't see it at the moment. cdev is just too "raw" for that.
thanks,
greg k-h
On Fri, Sep 12, 2025 at 03:39:45PM +0200, Greg KH wrote:
On Fri, Sep 12, 2025 at 04:26:56PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
On 12/09/2025 10:17, Tzung-Bi Shih wrote:
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
Thanks for the work. Just a note, please start using b4, so above Cc will be propagated to all patches. Folks above received only the cover letter...
Thank you for bringing this to my attention. I wasn't aware of that and will ensure this is handled correctly in the future.
Thanks to Krzysztof for making me aware of this. Could you please Cc my brgl@bgdev.pl address on the next iteration.
Sure, will do.
I haven't looked into the details yet but the small size of the first patch strikes me as odd. The similar changes I did for GPIO were quite big and they were designed just for a single sub-system.
During the talk you reference, after I suggested a library like this, Greg KH can be heard saying: do this for two big subsystems so that you're sure it's a generic solution. Here you're only using it in a single driver which makes me wonder if we can actually use it to improve bigger offenders, like for example I2C, or even replace the custom, SRCU-based solution in GPIO we have now. Have you considered at least doing a PoC in a wider kernel framework?
Yes, I'm happy to take this on.
To help me get started, could you please point me to some relevant code locations? Also, could you let me know if any specific physical devices will be needed for testing?
One interesting test would be to move the logic to the cdev layer. The use-after-free problem isn't specific to one type of character device, and so shouldn't require a fix in every driver instantiating a cdev directly (or indirectly). See [1] for a previous attempt to handle this at the V4L2 level and [2] for an attempt to handle it at the cdev level.
In [1], two new functions named video_device_enter() and video_device_exit() flag the beginning and end of protected code sections. The equivalent in [2] is the manual get/put of cdev->qactive, and if I understand things correctly, your series creates a REVOCABLE() macro to do the same. I'm sure we'll bikesheed about names at some point, but for the time being, what I'd like to see if this being done in fs/char_dev.c to cover all entry points from userspace at the cdev level.
We then have video_device_unplug() in [1], which I think is more or less the equivalent of revocable_provider_free(). I don't think we'll be able to hide this completely from drivers, at least not in all cases. We should however design the API to make it easy for drivers, likely with subsystem-specific wrappers.
What I have in mind is roughly the following:
Protect all access to the cdev from userspace with enter/exit calls that flag if a call is in progress. This can be done with explicit function calls, or with a scope guard as in your series.
At .remove() time, start by flagging that the device is being removed. That has to be an explicit call from drivers I believe, likely using subsystem-specific wrappers to simplify things.
Once the device is marked as being removed, all enter() calls should fail at the cdev level.
In .remove(), proceed to perform driver-specific operations that will stop the device and wake up any userspace task blocked on a syscall protected by enter()/remove(). This isn't needed for drivers/subsystems that don't provide any blocking API, but is required otherwise.
Unregister, still in .remove(), the cdev (likely through subsystem-specific APIs in most cases). This should block until all protected sections have exited.
The cdev is now unregistered, can't be opened anymore, and any new syscall on any opened file handle will return an error. The driver's .remove() function can proceed to free data, there won't be any UAF caused by userspace.
[1] implemented this fairly naively with flags and spinlocks. An RCU-based implementation is probably more efficient, even if I don't know how performance-sensitive all this is.
Does this align with your design, and do you think you could give a try at pushing revocable resource handling to the cdev level ?
On a separate note, I'm not sure "revocable" is the right name here. I believe a revocable resource API is needed, and well-named, for in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the userspace syscalls racing with .remove(), I don't think we're dealing with "revocable resources". Now, if a "revocable resources" API were to support the in-kernel users, and be usable as a building block to fix the cdev issue, I would have nothing against it, but the "revocable" name should be internal in that case, used in the cdev layer only, and not exposed to drivers (or even subsystem helpers that should wrap cdev functions instead).
I think the name makes sense as it matches up with how things are working (the backend structure is "revoked"), but naming is tough :)
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
For the .remove() code paths, yes, I expect driver changes. We need subsystem-level helpers that will make those easy and hide the complexity. For the code paths from userspace into the drivers through cdev file operations, there should be no driver change required.
And that's fine, we have "interns" that we can set loose on this type of code conversions, I think we just need to wrap the access to the cdev with this api, which will take a bit of rewriting in many drivers.
Anyway, just my thought, if someone else can see how this could drop into the core cdev code without any changes needed, that would be great, but I don't see it at the moment. cdev is just too "raw" for that.
On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
I don't think cdev is the right place for this as user-space keeping a reference to a file-descriptor whose "backend" disappeared is not the only possible problem. We can easily create a use-case of a USB I2C expander being used by some in-kernel consumer and then unplugged. This has nothing to do with the character device. I believe the sub-system level is the right place for this and every driver subsystem would have to integrate it separately, taking its various quirks into account.
Bartosz
On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
I don't think cdev is the right place for this as user-space keeping a reference to a file-descriptor whose "backend" disappeared is not the only possible problem. We can easily create a use-case of a USB I2C expander being used by some in-kernel consumer and then unplugged. This has nothing to do with the character device. I believe the sub-system level is the right place for this and every driver subsystem would have to integrate it separately, taking its various quirks into account.
That's why I mentioned in-kernel users previously. Drivers routinely acquire resources provided by other drivers, and having a way to revoke those is needed.
It is a different but related problem compared to userspace racing with .remove(). Could we solve both using the same backend concepts ? Perhaps, time will tell, and if that works nicely, great. But we still have lots of drivers exposing character devices to userspace (usually through a subsystem-specific API, drivers that create a cdev manually are the minority). That problem is in my opinion more urgent than handling the removal of in-kernel resources, because it's more common, and is easily triggerable by userspace. The good news is that it should also be simpler to solve, we should be able to address the enter/exit part entirely in cdev, and limit the changes to drivers in .remove() to the strict minimum.
What I'd like to see is if the proposed implementation of revocable resources can be used as a building block to fix the cdev issue. If it ca, great, let's solve it then. If it can't, that's still fine, it will still be useful for in-kernel resources, even if we need a different implementation for cdev.
On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
I don't think cdev is the right place for this as user-space keeping a reference to a file-descriptor whose "backend" disappeared is not the only possible problem. We can easily create a use-case of a USB I2C expander being used by some in-kernel consumer and then unplugged. This has nothing to do with the character device. I believe the sub-system level is the right place for this and every driver subsystem would have to integrate it separately, taking its various quirks into account.
That's why I mentioned in-kernel users previously. Drivers routinely acquire resources provided by other drivers, and having a way to revoke those is needed.
It is a different but related problem compared to userspace racing with .remove(). Could we solve both using the same backend concepts ? Perhaps, time will tell, and if that works nicely, great. But we still have lots of drivers exposing character devices to userspace (usually through a subsystem-specific API, drivers that create a cdev manually are the minority). That problem is in my opinion more urgent than handling the removal of in-kernel resources, because it's more common, and is easily triggerable by userspace. The good news is that it should also be simpler to solve, we should be able to address the enter/exit part entirely in cdev, and limit the changes to drivers in .remove() to the strict minimum.
What I'd like to see is if the proposed implementation of revocable resources can be used as a building block to fix the cdev issue. If it ca, great, let's solve it then. If it can't, that's still fine, it will still be useful for in-kernel resources, even if we need a different implementation for cdev.
Patch 5/5 in this series does just this for a specific use of a cdev in the driver. Is that what you are looking for?
thanks,
greg k-h
(CC'ing Dan Williams)
On Fri, Sep 12, 2025 at 04:19:53PM +0200, Greg KH wrote:
On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
I don't think cdev is the right place for this as user-space keeping a reference to a file-descriptor whose "backend" disappeared is not the only possible problem. We can easily create a use-case of a USB I2C expander being used by some in-kernel consumer and then unplugged. This has nothing to do with the character device. I believe the sub-system level is the right place for this and every driver subsystem would have to integrate it separately, taking its various quirks into account.
That's why I mentioned in-kernel users previously. Drivers routinely acquire resources provided by other drivers, and having a way to revoke those is needed.
It is a different but related problem compared to userspace racing with .remove(). Could we solve both using the same backend concepts ? Perhaps, time will tell, and if that works nicely, great. But we still have lots of drivers exposing character devices to userspace (usually through a subsystem-specific API, drivers that create a cdev manually are the minority). That problem is in my opinion more urgent than handling the removal of in-kernel resources, because it's more common, and is easily triggerable by userspace. The good news is that it should also be simpler to solve, we should be able to address the enter/exit part entirely in cdev, and limit the changes to drivers in .remove() to the strict minimum.
What I'd like to see is if the proposed implementation of revocable resources can be used as a building block to fix the cdev issue. If it ca, great, let's solve it then. If it can't, that's still fine, it will still be useful for in-kernel resources, even if we need a different implementation for cdev.
Patch 5/5 in this series does just this for a specific use of a cdev in the driver. Is that what you are looking for?
Not quite, I would like to see the enter/exit (aka revocable scope if my understanding is correct) being pushed to char_dev.c, as Dan did in [1]. I'm fine having to add an extra function call in the .remove() path of drivers, but I'm not fine with having to mark revocable sections manually in drivers. That part belongs to cdev.
[1] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwi...
On Fri, Sep 12, 2025 at 05:26:46PM +0300, Laurent Pinchart wrote:
(CC'ing Dan Williams)
On Fri, Sep 12, 2025 at 04:19:53PM +0200, Greg KH wrote:
On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
I don't think cdev is the right place for this as user-space keeping a reference to a file-descriptor whose "backend" disappeared is not the only possible problem. We can easily create a use-case of a USB I2C expander being used by some in-kernel consumer and then unplugged. This has nothing to do with the character device. I believe the sub-system level is the right place for this and every driver subsystem would have to integrate it separately, taking its various quirks into account.
That's why I mentioned in-kernel users previously. Drivers routinely acquire resources provided by other drivers, and having a way to revoke those is needed.
It is a different but related problem compared to userspace racing with .remove(). Could we solve both using the same backend concepts ? Perhaps, time will tell, and if that works nicely, great. But we still have lots of drivers exposing character devices to userspace (usually through a subsystem-specific API, drivers that create a cdev manually are the minority). That problem is in my opinion more urgent than handling the removal of in-kernel resources, because it's more common, and is easily triggerable by userspace. The good news is that it should also be simpler to solve, we should be able to address the enter/exit part entirely in cdev, and limit the changes to drivers in .remove() to the strict minimum.
What I'd like to see is if the proposed implementation of revocable resources can be used as a building block to fix the cdev issue. If it ca, great, let's solve it then. If it can't, that's still fine, it will still be useful for in-kernel resources, even if we need a different implementation for cdev.
Patch 5/5 in this series does just this for a specific use of a cdev in the driver. Is that what you are looking for?
Not quite, I would like to see the enter/exit (aka revocable scope if my understanding is correct) being pushed to char_dev.c, as Dan did in [1]. I'm fine having to add an extra function call in the .remove() path of drivers, but I'm not fine with having to mark revocable sections manually in drivers. That part belongs to cdev.
[1] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwi...
Dan's proposal here is a good start, but the "sleep in cdev_del() until the device drains all existing opens" is going to not really work well for what we want.
So sure, make a new cdev api to use this, that's fine, then we will have what, 5 different ways to use a cdev? :)
Seriously, that would be good, then we can work to convert things over, but I think overall it will look much the same as what patch 5/5 does here. But details matter, I don't really known for sure...
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
thanks,
greg k-h
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
Dan's proposal here is a good start, but the "sleep in cdev_del() until the device drains all existing opens" is going to not really work well for what we want.
So sure, make a new cdev api to use this, that's fine, then we will have what, 5 different ways to use a cdev? :)
Seriously, that would be good, then we can work to convert things over, but I think overall it will look much the same as what patch 5/5 does here. But details matter, I don't really known for sure...
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is needed for this to be usable in other parts of the kernel, that can be done gradually. Literally no subsystem ever was perfect on day 1.
Tzung-Bi: I'm not sure if you did submit anything but I'd love to see this discussed during Linux Plumbers in Tokyo, it's the perfect fit for the kernel summit.
Bartosz
On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
Dan's proposal here is a good start, but the "sleep in cdev_del() until the device drains all existing opens" is going to not really work well for what we want.
So sure, make a new cdev api to use this, that's fine, then we will have what, 5 different ways to use a cdev? :)
Seriously, that would be good, then we can work to convert things over, but I think overall it will look much the same as what patch 5/5 does here. But details matter, I don't really known for sure...
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is needed for this to be usable in other parts of the kernel, that can be done gradually. Literally no subsystem ever was perfect on day 1.
To be clear, I'm not against the API being merged for the use cases that would benefit from it, but I don't want to see drivers using it to protect from the cdev/unregistration race.
Tzung-Bi: I'm not sure if you did submit anything but I'd love to see this discussed during Linux Plumbers in Tokyo, it's the perfect fit for the kernel summit.
On 9/12/25 4:54 PM, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is needed for this to be usable in other parts of the kernel, that can be done gradually. Literally no subsystem ever was perfect on day 1.
To be clear, I'm not against the API being merged for the use cases that would benefit from it, but I don't want to see drivers using it to protect from the cdev/unregistration race.
I mean, revocable is really a synchronization primitive in the end that "revokes" access to some resource in a race free way.
So, technically, it probably belongs into lib/.
I think the reason it ended up in drivers/base/ is that one common use case is to revoke a device resource from a driver when the device is unbound from this driver; or in other words devres is an obvious user.
So, I think that any other API (cdev, devres, etc.) should be built on top of it.
This is also what we do in Rust, Revocable is just a common synchronization primitive and the (only) user it has is currently Devres building on top of it.
On Fri, Sep 12, 2025 at 06:22:48PM +0200, Danilo Krummrich wrote:
On 9/12/25 4:54 PM, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is needed for this to be usable in other parts of the kernel, that can be done gradually. Literally no subsystem ever was perfect on day 1.
To be clear, I'm not against the API being merged for the use cases that would benefit from it, but I don't want to see drivers using it to protect from the cdev/unregistration race.
I mean, revocable is really a synchronization primitive in the end that "revokes" access to some resource in a race free way.
So, technically, it probably belongs into lib/.
I think the reason it ended up in drivers/base/ is that one common use case is to revoke a device resource from a driver when the device is unbound from this driver; or in other words devres is an obvious user.
So, I think that any other API (cdev, devres, etc.) should be built on top of it.
No issue with that. I'm sure there are people who have better knowledge than me when it comes to implementing the low-level primitive in the most efficient way. What I have lots of experience with is the impact of API design on drivers, and the API misuse (including through cargo-cult programming) this can generate. Let's design the API towards drivers correctly.
This is also what we do in Rust, Revocable is just a common synchronization primitive and the (only) user it has is currently Devres building on top of it.
Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 06:22:48PM +0200, Danilo Krummrich wrote:
On 9/12/25 4:54 PM, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is needed for this to be usable in other parts of the kernel, that can be done gradually. Literally no subsystem ever was perfect on day 1.
To be clear, I'm not against the API being merged for the use cases that would benefit from it, but I don't want to see drivers using it to protect from the cdev/unregistration race.
I mean, revocable is really a synchronization primitive in the end that "revokes" access to some resource in a race free way.
So, technically, it probably belongs into lib/.
I think the reason it ended up in drivers/base/ is that one common use case is to revoke a device resource from a driver when the device is unbound from this driver; or in other words devres is an obvious user.
So, I think that any other API (cdev, devres, etc.) should be built on top of it.
No issue with that. I'm sure there are people who have better knowledge than me when it comes to implementing the low-level primitive in the most efficient way. What I have lots of experience with is the impact of API design on drivers, and the API misuse (including through cargo-cult programming) this can generate. Let's design the API towards drivers correctly.
Note that I dropped the "managed_fops" [1] effort, targeted for use for CXL, in favor of simply this in the CXL ioctl device shutdown path:
cdev_device_del(&cxlmd->cdev, &cxlmd->dev); scoped_guard(rwsem_write, &cxl_memdev_rwsem) cxlmd->cxlds = NULL; put_device(&cxlmd->dev);
Pair that with:
guard(rwsem_read)(&cxl_memdev_rwsem); cxlds = cxlmd->cxlds; if (cxlds) return __cxl_memdev_ioctl(cxlmd, cmd, arg); return -ENXIO;
...on the ioctl invocation side.
This "revocable" mechanism looks useful for other inter-driver resource sharing, but not for the well known issues with cdev. For cdev and the design pattern of "shutdown the ioctl path on a core-subsystem device object that is also a chardev", just use cdev_device_add() with an rwsem.
[1]: http://lore.kernel.org/all/CAPcyv4h74NjqcuUjv4zFKHAxio_bV0bngLoxP=ACw=JvMfq-...
On Fri, Sep 12, 2025 at 05:54:16PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
Dan's proposal here is a good start, but the "sleep in cdev_del() until the device drains all existing opens" is going to not really work well for what we want.
So sure, make a new cdev api to use this, that's fine, then we will have what, 5 different ways to use a cdev? :)
Seriously, that would be good, then we can work to convert things over, but I think overall it will look much the same as what patch 5/5 does here. But details matter, I don't really known for sure...
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is needed for this to be usable in other parts of the kernel, that can be done gradually. Literally no subsystem ever was perfect on day 1.
To be clear, I'm not against the API being merged for the use cases that would benefit from it, but I don't want to see drivers using it to protect from the cdev/unregistration race.
Based on the discussion thread, my main takeaways are:
- Current `revocable` is considered a low level API. We shouldn't (and likely can't) stop drivers, like the one in patch 5/5 in the series, from using it directly to fix UAFs.
- Subsystems (like cdev) should build on this API to provide an easier interface for their drivers to manage revocable resources.
I'll create a PoC based on this.
Tzung-Bi: I'm not sure if you did submit anything but I'd love to see this discussed during Linux Plumbers in Tokyo, it's the perfect fit for the kernel summit.
Yes, and I just realized that in addition to the website submission, a separate email is also required (or at least encouraged). I've just sent that email and am hoping it's not too late.
On Sat, Sep 13, 2025 at 11:55:45PM +0800, Tzung-Bi Shih wrote:
On Fri, Sep 12, 2025 at 05:54:16PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:
Dan's proposal here is a good start, but the "sleep in cdev_del() until the device drains all existing opens" is going to not really work well for what we want.
So sure, make a new cdev api to use this, that's fine, then we will have what, 5 different ways to use a cdev? :)
Seriously, that would be good, then we can work to convert things over, but I think overall it will look much the same as what patch 5/5 does here. But details matter, I don't really known for sure...
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to. We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is needed for this to be usable in other parts of the kernel, that can be done gradually. Literally no subsystem ever was perfect on day 1.
To be clear, I'm not against the API being merged for the use cases that would benefit from it, but I don't want to see drivers using it to protect from the cdev/unregistration race.
Based on the discussion thread, my main takeaways are:
- Current `revocable` is considered a low level API. We shouldn't (and likely can't) stop drivers, like the one in patch 5/5 in the series, from using it directly to fix UAFs.
Why shouldn't we ? We have enough precedents where driver authors rushed to adopt brand new APIs without understand the implications. devm_kzalloc() is a prime example of a small new API that very quickly got misused everywhere. If we had taken the time to clearly explain when it should be used and when it should *not* be used, we wouldn't be plagued by as many device removal race conditions today. Let's not repeat the same mistake, I'd like this new API to make things better, not worse.
- Subsystems (like cdev) should build on this API to provide an easier interface for their drivers to manage revocable resources.
I'll create a PoC based on this.
I'm looking forward to that. Please let me know if there's anything you would like to discuss. I didn't dive deep in technical details in this thread, and I don't expect anyone to guess what I have in mind if I failed to express it :-) I'm very confident the cdev race condition can be fixed in a neat way, so let's do that.
Tzung-Bi: I'm not sure if you did submit anything but I'd love to see this discussed during Linux Plumbers in Tokyo, it's the perfect fit for the kernel summit.
Yes, and I just realized that in addition to the website submission, a separate email is also required (or at least encouraged). I've just sent that email and am hoping it's not too late.
On Sat, Sep 13, 2025 at 07:14:13PM +0300, Laurent Pinchart wrote:
On Sat, Sep 13, 2025 at 11:55:45PM +0800, Tzung-Bi Shih wrote:
- Subsystems (like cdev) should build on this API to provide an easier interface for their drivers to manage revocable resources.
I'll create a PoC based on this.
I'm looking forward to that. Please let me know if there's anything you would like to discuss. I didn't dive deep in technical details in this thread, and I don't expect anyone to guess what I have in mind if I failed to express it :-) I'm very confident the cdev race condition can be fixed in a neat way, so let's do that.
Even though I think this isn't what you are looking for originally, please take a look on the PoC attempt (5th - 7th patches) in [1]. Unlike [2], the PoC allows fops to be exuected and defers to the driver to decide what to do if the resource is unavailable.
[1] https://lore.kernel.org/chrome-platform/20250923075302.591026-1-tzungbi@kern... [2] https://lore.kernel.org/all/161117153776.2853729.6944617921517514510.stgit@d...
On Fri, Sep 12, 2025 at 04:40:27PM +0200, Greg KH wrote:
On Fri, Sep 12, 2025 at 05:26:46PM +0300, Laurent Pinchart wrote:
(CC'ing Dan Williams)
On Fri, Sep 12, 2025 at 04:19:53PM +0200, Greg KH wrote:
On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
I don't think cdev is the right place for this as user-space keeping a reference to a file-descriptor whose "backend" disappeared is not the only possible problem. We can easily create a use-case of a USB I2C expander being used by some in-kernel consumer and then unplugged. This has nothing to do with the character device. I believe the sub-system level is the right place for this and every driver subsystem would have to integrate it separately, taking its various quirks into account.
That's why I mentioned in-kernel users previously. Drivers routinely acquire resources provided by other drivers, and having a way to revoke those is needed.
It is a different but related problem compared to userspace racing with .remove(). Could we solve both using the same backend concepts ? Perhaps, time will tell, and if that works nicely, great. But we still have lots of drivers exposing character devices to userspace (usually through a subsystem-specific API, drivers that create a cdev manually are the minority). That problem is in my opinion more urgent than handling the removal of in-kernel resources, because it's more common, and is easily triggerable by userspace. The good news is that it should also be simpler to solve, we should be able to address the enter/exit part entirely in cdev, and limit the changes to drivers in .remove() to the strict minimum.
What I'd like to see is if the proposed implementation of revocable resources can be used as a building block to fix the cdev issue. If it ca, great, let's solve it then. If it can't, that's still fine, it will still be useful for in-kernel resources, even if we need a different implementation for cdev.
Patch 5/5 in this series does just this for a specific use of a cdev in the driver. Is that what you are looking for?
Not quite, I would like to see the enter/exit (aka revocable scope if my understanding is correct) being pushed to char_dev.c, as Dan did in [1]. I'm fine having to add an extra function call in the .remove() path of drivers, but I'm not fine with having to mark revocable sections manually in drivers. That part belongs to cdev.
[1] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwi...
Dan's proposal here is a good start, but the "sleep in cdev_del() until the device drains all existing opens" is going to not really work well for what we want.
I think you missed the fact that the code doesn't wait for all open file handles to be closed. It waits for all in-progress syscalls to return from the driver. That's a big difference.
So sure, make a new cdev api to use this, that's fine, then we will have what, 5 different ways to use a cdev? :)
Seriously, that would be good, then we can work to convert things over, but I think overall it will look much the same as what patch 5/5 does here. But details matter, I don't really known for sure...
What I don't want to see is drivers using this new API directly to protect the cdev race. That would be a big step in the wrong direction. Patch 5/5 needs to move driver code to the cdev level. That shouldn't be difficult, so I really not see why it can't be done in v4 to see how it will look like.
Either way, I think this patch series stands on its own, it doesn't require cdev to implement it, drivers can use it to wrap a cdev if they want to.
No, drivers should *not* do that manually. That's a recipe for disaster that we would regret later.
We have other structures that want to do this type of thing today as is proof with the rust implementation for the devm api.
On Fri, Sep 12, 2025 at 05:26:46PM +0300, Laurent Pinchart wrote:
Not quite, I would like to see the enter/exit (aka revocable scope if my understanding is correct) being pushed to char_dev.c, as Dan did in [1]. I'm fine having to add an extra function call in the .remove() path of drivers, but I'm not fine with having to mark revocable sections manually in drivers. That part belongs to cdev.
+1 I've open coded something here many times.
The implementation challenge is performance..
The big ask would be to make file_operations replaceable without races. I can't see a way to do that at the struct file level without slowing down everything.
Dan's idea to provide a wrapper file_operations that then effectively calls another set of file_operations under a synchornization is more reasonable, but the performance cost is now a percpu ref and another indirect function call for every file operation. It also relies on the inode which some cdev users end up replacing.
Open coding the lock in the subsystems avoids the indirect function call, flexible inode, and subsystems can choose their locking preference (rwsem, srcu, percpu).
As was said later in this thread, it would be a real shame to see people implement revocable in drivers instead of rely on subsystems to have sane unregistration semantics where the subsystem guarentees that no driver callbacks are running after unregister. You never need driver revocable in a world like that.
Jason
On Mon Sep 22, 2025 at 5:10 PM CEST, Jason Gunthorpe wrote:
As was said later in this thread, it would be a real shame to see people implement revocable in drivers instead of rely on subsystems to have sane unregistration semantics where the subsystem guarentees that no driver callbacks are running after unregister. You never need driver revocable in a world like that.
I fully agree with that, in C there is indeed no value of a revocable type when subsystems can guarantee "sane unregistration semantics".
I say "in C" because in C there is no way to get a proof by the compiler that we're in a scope (e.g. through the subsystem guarentee) where the device is guaranteed to be bound (which we can in Rust).
So, effectively, we're not getting any value out of the revocable in C in such a case: In the best case, we're just bypassing the revocable by accessing the pointer unchecked (regardless whether that's valid or not); in the worst case we're introducing a useless SRCU read side critical section.
(In Rust the compiler will stop us from accessing the pointer unchecked if we're not in a scope where unchecked access is valid.)
So, I think in C revocable should be restricted to use-cases where scopes are unbound by design. DRM device callbacks are an example for that and it's the reason why things like drm_dev_{enter,exit}() and drm_dev_unplug() exist. In the end, those are exactly the same as revocable implemented in a slightly different way.
- Danilo
On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
I fully agree with that, in C there is indeed no value of a revocable type when subsystems can guarantee "sane unregistration semantics".
Indeed, I agree with your message. If I look at the ec_cros presented here, there is no reason for any revocable. In fact it already seems like an abuse of the idea.
The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev" that is alreay properly lifetime controlled - the platform is already removed during the remove of the cros_ec.c.
So, there is no need for a revocable that spans two drivers like this!
The bug is that cros_ec_chardev.c doesn't implement itself correctly and doesn't have a well designed remove() for something that creates a char dev. This issue should be entirely handled within cros_ec_chardev.c and not leak out to other files.
1) Using a miscdevice means loosing out on any refcount managed memory. When using a file you need some per-device memory that lives for as long as all files are open. So don't use miscdev, the better pattern is:
struct chardev_data { struct device dev; struct cdev cdev;
Now you get to have a struct device linked refcount and a free function. The file can savely hold onto a chardev_data for its lifetime.
2) Add locking so the file can exist after the driver is removed:
struct chardev_data { [..] struct rw_semaphore sem; struct cros_ec_dev *ec_dev; };
Refcount the chardev_data::dev in the file operations open/release, refer to it via the private_data.
3) At the start of every fop take the sem and check the ec_dev:
ACQUIRE(rwsem_read, ecdev_sem)(&data->sem); if (ACQUIRE_ERR(ecdev_sem) || !data->ec_dev) return -ENODEV;
4) After unregistering the cdev, but before destroying the device take the write side of the rwsem and NULL ec_dev.
5) Purge all the devm usage from cros_ec_chardev, the only allocation is refcounted instead.
Simple. No need for any synchronize_srcu() for such a simple non-performance oriented driver.
Yes, this can be made better, there is a bit too much boilerplate, but revocable is not the way for cros_ec.
Jason
On Mon, Sep 22, 2025 at 02:40:10PM -0300, Jason Gunthorpe wrote:
On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
I fully agree with that, in C there is indeed no value of a revocable type when subsystems can guarantee "sane unregistration semantics".
Indeed, I agree with your message. If I look at the ec_cros presented here, there is no reason for any revocable. In fact it already seems like an abuse of the idea.
The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev" that is alreay properly lifetime controlled - the platform is already removed during the remove of the cros_ec.c.
So, there is no need for a revocable that spans two drivers like this!
It's a very common pattern, you have a struct device, and a userspace access, and need to "split" them apart, as you say below. This logic here handles that pattern.
See the many talks about this in the past at Plumbers and other conferences, this isn't a new thing, and it isn't quite as simple as I think you are making it out to be to solve.
The bug is that cros_ec_chardev.c doesn't implement itself correctly and doesn't have a well designed remove() for something that creates a char dev. This issue should be entirely handled within cros_ec_chardev.c and not leak out to other files.
- Using a miscdevice means loosing out on any refcount managed
memory. When using a file you need some per-device memory that lives for as long as all files are open. So don't use miscdev, the better pattern is:
struct chardev_data { struct device dev; struct cdev cdev;
Woah, no, that is totally wrong.
Now you get to have a struct device linked refcount and a free function. The file can savely hold onto a chardev_data for its lifetime.
You have 2 different reference counts for the same structure. A bug that should never be done (yes, it's done a lot in the kernel, it's wrong.)
And really, let's not abuse cdev anymore than it currently is please. There's a reason that miscdevice returns just a pointer. Right now cdev can be used in 3-4 different ways, let's not come up with another way to abuse that api :)
- Add locking so the file can exist after the driver is removed:
struct chardev_data { [..] struct rw_semaphore sem; struct cros_ec_dev *ec_dev; };
Refcount the chardev_data::dev in the file operations open/release, refer to it via the private_data.
- At the start of every fop take the sem and check the ec_dev:
ACQUIRE(rwsem_read, ecdev_sem)(&data->sem); if (ACQUIRE_ERR(ecdev_sem) || !data->ec_dev) return -ENODEV;
- After unregistering the cdev, but before destroying the device take
the write side of the rwsem and NULL ec_dev.
- Purge all the devm usage from cros_ec_chardev, the only allocation
is refcounted instead.
Simple. No need for any synchronize_srcu() for such a simple non-performance oriented driver.
There is no performance issue here, but there is lifetime rule logic here that I really really do not want to have to "open code" for every user. revokable gives this to us in a simple way that we can "know" is being used correctly.
And again, you can't have a single structure with two reference counts, that's not allowed :)
Yes, this can be made better, there is a bit too much boilerplate, but revocable is not the way for cros_ec.
I strongly disagree, sorry.
greg k-h
On Mon, Sep 22, 2025 at 08:42:23PM +0200, Greg Kroah-Hartman wrote:
On Mon, Sep 22, 2025 at 02:40:10PM -0300, Jason Gunthorpe wrote:
On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
I fully agree with that, in C there is indeed no value of a revocable type when subsystems can guarantee "sane unregistration semantics".
Indeed, I agree with your message. If I look at the ec_cros presented here, there is no reason for any revocable. In fact it already seems like an abuse of the idea.
The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev" that is alreay properly lifetime controlled - the platform is already removed during the remove of the cros_ec.c.
So, there is no need for a revocable that spans two drivers like this!
It's a very common pattern, you have a struct device, and a userspace access, and need to "split" them apart, as you say below. This logic here handles that pattern.
This is two struct devices, two device drivers and a fops. There is no reason to trigger revoke from the parent driver to a child driver, that's just mis-layering and obfuscation.
It already subtly relies on the parent not triggering revoke until the child is removed because it added this to the cros-ec-chardev driver:
@@ -166,7 +183,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp) struct cros_ec_dev *ec_dev = dev_get_drvdata(mdev->parent);
if (!priv) return -ENOMEM;
- priv->ec_dev = ec_dev; + priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
It would UAF if the revoke is triggered by the parent driver at the wrong time.
So why is the parent involved at all? Why does it have to violate modularity?
See the many talks about this in the past at Plumbers and other conferences, this isn't a new thing, and it isn't quite as simple as I think you are making it out to be to solve.
There are more complex situations, but this isn't one of them. All this needs is to fence the file operations of a single cdev. I've solved it enough times now to know exactly how simple this really is..
struct chardev_data {
struct device dev; struct cdev cdev;
Woah, no, that is totally wrong.
Sigh. I'm sure we've had this exchange before. Please re-read the documentation for cdev_device_add():
* This function should be used whenever the struct cdev and the * struct device are members of the same structure whose lifetime is * managed by the struct device. ^^^^^^^^^^^^
We created this helper specifically to clean up the refcount bugs around cdev usage. It supports the above pattern which is the natural and easy way to use cdev.
And really, let's not abuse cdev anymore than it currently is please. There's a reason that miscdevice returns just a pointer. Right now cdev can be used in 3-4 different ways, let's not come up with another way to abuse that api :)
It is true there are many abuses, but converting cdev users to use cdev_device_add() is the right and best option IMHO.
miscdev is not a good option in cases like this because you don't get a nice natural kref around the memory, among other limitations.
There is no performance issue here, but there is lifetime rule logic here that I really really do not want to have to "open code" for every user. revokable gives this to us in a simple way that we can "know" is being used correctly.
I strongly prefer Laurent's direction to add a helper file_operations that automatically wraps all ops in a lock.
I think all this series does is create a weirdly named lock that drivers still have to open code.
The fact the very first proposed user is already abusing it to needlessly violate driver modularity is really depressing. Let's not create another devm :\
Jason
linux-kselftest-mirror@lists.linaro.org
-
Bartosz Golaszewski -
Bartosz Golaszewski -
dan.j.williams＠intel.com -
Danilo Krummrich -
Greg Kroah-Hartman -
Jason Gunthorpe -
Jonathan Corbet -
Krzysztof Kozlowski -
Laurent Pinchart -
Simona Vetter -
Tzung-Bi Shih