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 - 7th are PoC patches for showing the use case of "Replace file operations" below.
---
I came out with 2 possible usages of revocable.
1. Use primitive APIs
Use the primitive APIs of revocable directly.
The file operations make sure the resources are available when using them.
This is what the series original proposed[3][4]. Even though it has the finest grain for accessing the resources, it makes the user code verbose. Per feedback from the community, I'm looking for some subsystem level helpers so that user code can be simlper.
2. Replace file operations
Replace filp->f_op to revocable-aware warppers.
The warppers make sure the resources are available in the file operations.
The user code needs to provide a callback .try_access() to tell the wrappers where/how to *save* the pointers of resources.
Known drawback: - The warppers reserve the resources for all file operations even if they might be unused. - The user code still needs to be revocable-aware. - The whole file operation becomes a SRCU read-side critical section. Are there any functions can't be called in the critical section? If there is, the file operations may not be awared of that.
See 5th - 7th patches for an example usage.
[1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@ker... [2] https://lpc.events/event/17/contributions/1627/ [3] https://lore.kernel.org/chrome-platform/20250912081718.3827390-5-tzungbi@ker... [4] https://lore.kernel.org/chrome-platform/20250912081718.3827390-6-tzungbi@ker...
v5: - Rebase onto next-20251015. - Add more context about the PoC. - Support multiple revocable providers in the PoC.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-1-tzungbi@kern... - Rebase onto next-20250922. - Remove the 5th patch from v3. - Add fops replacement PoC in 5th - 7th patches.
v3: https://lore.kernel.org/chrome-platform/20250912081718.3827390-1-tzungbi@ker... - Rebase onto https://lore.kernel.org/chrome-platform/20250828083601.856083-1-tzungbi@kern... and next-20250912. - The 4th patch changed 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 (7): revocable: Revocable resource management revocable: Add Kunit test cases selftests: revocable: Add kselftest cases platform/chrome: Protect cros_ec_device lifecycle with revocable revocable: Add fops replacement char: misc: Leverage revocable fops replacement platform/chrome: cros_ec_chardev: Secure cros_ec_device via revocable
.../driver-api/driver-model/index.rst | 1 + .../driver-api/driver-model/revocable.rst | 87 +++++++ MAINTAINERS | 9 + drivers/base/Kconfig | 8 + drivers/base/Makefile | 5 +- drivers/base/revocable.c | 233 ++++++++++++++++++ drivers/base/revocable_test.c | 110 +++++++++ drivers/char/misc.c | 8 + drivers/platform/chrome/cros_ec.c | 5 + drivers/platform/chrome/cros_ec_chardev.c | 22 +- fs/Makefile | 2 +- fs/fs_revocable.c | 154 ++++++++++++ include/linux/fs.h | 2 + include/linux/fs_revocable.h | 21 ++ include/linux/miscdevice.h | 4 + include/linux/platform_data/cros_ec_proto.h | 4 + include/linux/revocable.h | 53 ++++ 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 ++++++++++++++ 23 files changed, 1086 insertions(+), 3 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 fs/fs_revocable.c create mode 100644 include/linux/fs_revocable.h 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.
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 uses a provider/consumer model built on Sleepable RCU (SRCU) to guarantee safe memory access:
- A resource provider, such as a driver for a hot-pluggable device, 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 acts as a handle containing 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_withdraw_access() to exit the SRCU critical section. The REVOCABLE_TRY_ACCESS_WITH() is a convenient helper for doing that.
- When the provider needs to remove the resource, it calls revocable_provider_revoke(). 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.
Acked-by: Danilo Krummrich dakr@kernel.org Acked-by: Simona Vetter simona.vetter@ffwll.ch Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- v5: - No changes.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-2-tzungbi@kern... - Rename: - revocable_provider_free() -> revocable_provider_revoke(). - REVOCABLE() -> REVOCABLE_TRY_ACCESS_WITH(). - revocable_release() -> revocable_withdraw_access(). - rcu_dereference() -> srcu_dereference() to fix a warning from lock debugging. - Move most docs to kernel-doc, include them in Documentation/, and modify the commit message accordingly. - Fix some doc errors. - Add Acked-by tags.
v3: https://lore.kernel.org/chrome-platform/20250912081718.3827390-2-tzungbi@ker... - 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...
Note (for my reference): - An optional .release() callback for revocable provider-managed resource hasn't added. - `make O=build SPHINXDIRS=driver-api/driver-model/ htmldocs` a way to verify the Documentation/.
.../driver-api/driver-model/index.rst | 1 + .../driver-api/driver-model/revocable.rst | 87 +++++++ MAINTAINERS | 7 + drivers/base/Makefile | 2 +- drivers/base/revocable.c | 233 ++++++++++++++++++ include/linux/revocable.h | 53 ++++ 6 files changed, 382 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..ce2d0eb1c8cb --- /dev/null +++ b/Documentation/driver-api/driver-model/revocable.rst @@ -0,0 +1,87 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Revocable Resource Management +============================== + +Overview +======== + +.. kernel-doc:: drivers/base/revocable.c + :doc: Overview + +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 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 +---------------------- + +.. kernel-doc:: drivers/base/revocable.c + :identifiers: revocable_provider_alloc + +.. kernel-doc:: drivers/base/revocable.c + :identifiers: devm_revocable_provider_alloc + +.. kernel-doc:: drivers/base/revocable.c + :identifiers: revocable_provider_revoke + +For Resource Consumers +---------------------- + +.. kernel-doc:: drivers/base/revocable.c + :identifiers: revocable_alloc + +.. kernel-doc:: drivers/base/revocable.c + :identifiers: revocable_free + +.. kernel-doc:: drivers/base/revocable.c + :identifiers: revocable_try_access + +.. kernel-doc:: drivers/base/revocable.c + :identifiers: revocable_withdraw_access + +.. kernel-doc:: include/linux/revocable.h + :identifiers: REVOCABLE_TRY_ACCESS_WITH + +Example Usage +~~~~~~~~~~~~~ + +.. code-block:: c + + void consumer_use_resource(struct revocable *rev) + { + struct foo_resource *res; + + REVOCABLE_TRY_ACCESS_WITH(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_withdraw_access() is automatically called here. + } diff --git a/MAINTAINERS b/MAINTAINERS index 25ed1846d970..7d00ff431af9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22020,6 +22020,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..f8dd4363a87b --- /dev/null +++ b/drivers/base/revocable.c @@ -0,0 +1,233 @@ +// 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. + * + * 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 uses a provider/consumer model built on Sleepable + * RCU (SRCU) to guarantee safe memory access: + * + * - A resource provider, such as a driver for a hot-pluggable device, + * 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 acts as a handle containing 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_withdraw_access() to exit the SRCU critical section. The + * REVOCABLE_TRY_ACCESS_WITH() is a convenient helper for doing that. + * + * - When the provider needs to remove the resource, it calls + * revocable_provider_revoke(). 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_revoke() - Revoke the managed resource. + * @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_revoke(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_revoke); + +static void devm_revocable_provider_revoke(void *data) +{ + struct revocable_provider *rp = data; + + revocable_provider_revoke(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_revoke() 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_revoke, rp)) + return NULL; + + return rp; +} +EXPORT_SYMBOL_GPL(devm_revocable_provider_alloc); + +/** + * revocable_alloc() - Allocate struct revocable. + * @rp: The pointer of resource provider. + * + * This holds a refcount to the resource provider. + * + * Return: The pointer of struct revocable. 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 srcu_dereference(rp->res, &rp->srcu); +} +EXPORT_SYMBOL_GPL(revocable_try_access); + +/** + * revocable_withdraw_access() - 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_withdraw_access(struct revocable *rev) __releases(&rev->rp->srcu) +{ + struct revocable_provider *rp = rev->rp; + + srcu_read_unlock(&rp->srcu, rev->idx); +} +EXPORT_SYMBOL_GPL(revocable_withdraw_access); diff --git a/include/linux/revocable.h b/include/linux/revocable.h new file mode 100644 index 000000000000..7177bf045d9c --- /dev/null +++ b/include/linux/revocable.h @@ -0,0 +1,53 @@ +/* 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_revoke(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_withdraw_access(struct revocable *rev) __releases(&rev->rp->srcu); + +DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_withdraw_access(_T)) + +#define _REVOCABLE_TRY_ACCESS_WITH(_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 + +/** + * REVOCABLE_TRY_ACCESS_WITH() - A helper for accessing revocable resource + * @_rev: The consumer's ``struct revocable *`` handle. + * @_res: A pointer variable that will be assigned the resource. + * + * The macro simplifies the access-release cycle for consumers, ensuring that + * revocable_withdraw_access() is always called, even in the case of an early + * exit. + * + * It 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_withdraw_access() function is automatically called when the scope + * of the loop is exited. + */ +#define REVOCABLE_TRY_ACCESS_WITH(_rev, _res) \ + _REVOCABLE_TRY_ACCESS_WITH(_rev, __UNIQUE_ID(label), _res) + +#endif /* __LINUX_REVOCABLE_H */
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 --- v5: - No changes.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-3-tzungbi@kern... - REVOCABLE() -> REVOCABLE_TRY_ACCESS_WITH(). - revocable_release() -> revocable_withdraw_access().
v3: https://lore.kernel.org/chrome-platform/20250912081718.3827390-3-tzungbi@ker... - 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 7d00ff431af9..7207538f31f4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22025,6 +22025,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 1786d87b29e2..8f7d7b9d81ac 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -250,3 +250,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..1c3e08ba0505 --- /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_TRY_ACCESS_WITH(). + */ + +#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_withdraw_access(rev); + + revocable_free(rev); + revocable_provider_revoke(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_withdraw_access(rev); + + revocable_provider_revoke(rp); + + res = revocable_try_access(rev); + KUNIT_EXPECT_PTR_EQ(test, res, NULL); + revocable_withdraw_access(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_TRY_ACCESS_WITH(rev, res) { + KUNIT_EXPECT_PTR_EQ(test, res, real_res); + accessed = true; + } + KUNIT_EXPECT_TRUE(test, accessed); + + revocable_provider_revoke(rp); + + accessed = false; + REVOCABLE_TRY_ACCESS_WITH(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 --- v5: - No changes.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-4-tzungbi@kern... - REVOCABLE() -> REVOCABLE_TRY_ACCESS_WITH(). - revocable_release() -> revocable_withdraw_access().
v3: https://lore.kernel.org/chrome-platform/20250912081718.3827390-4-tzungbi@ker... - No changes.
v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-4-tzungbi@kern... - New in the series.
A way to run the kselftest (for my reference): - Update kernel to the DUT. - `mkdir build` and copy the kernel config to build/. - `make O=build LLVM=1 -j32` (for generated headers and built-in symbols). - `make O=build LLVM=1 KDIR=$(pwd) -C tools/testing/selftests/ TARGETS=drivers/base/revocable gen_tar`. - Copy build/kselftest/kselftest_install/kselftest-packages/kselftest.tar.gz to the DUT, extract, and execute the run_kselftest.sh.
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 7207538f31f4..235a1d6e3718 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22027,6 +22027,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 c46ebdb9b8ef..1136a8f12ef5 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..3c0feb7a8944 --- /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_TRY_ACCESS_WITH(). + */ + +#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..c90ec150a6c8 --- /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_withdraw_access(rev); + break; + case TEST_MAGIC_OFFSET: + REVOCABLE_TRY_ACCESS_WITH(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_revoke(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_revoke(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_revoke(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 --- v5: - No changes.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-5-tzungbi@kern... - No changes.
v3: https://lore.kernel.org/chrome-platform/20250912081718.3827390-5-tzungbi@ker... - 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; };
/**
Introduce fs_revocable_replace() to simplify the use of the revocable API with file_operations.
The function, should be called from a driver's ->open(), replaces the fops with a wrapper that automatically handles the `try_access` and `withdraw_access`.
When the file is closed, the wrapper's ->release() restores the original fops and cleanups. This centralizes the revocable logic, making drivers cleaner and easier to maintain.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- PoC patch.
Known issues: - All file operations call revocable_try_access() for guaranteeing the resource even if the resource may be unused in the fops.
v5: - Rename to "fs_revocable". - Move the replacement context to struct file. - Support multiple revocable providers.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-6-tzungbi@kern... - New in the series.
fs/Makefile | 2 +- fs/fs_revocable.c | 154 +++++++++++++++++++++++++++++++++++ include/linux/fs.h | 2 + include/linux/fs_revocable.h | 21 +++++ 4 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 fs/fs_revocable.c create mode 100644 include/linux/fs_revocable.h
diff --git a/fs/Makefile b/fs/Makefile index e3523ab2e587..ec2b21385deb 100644 --- a/fs/Makefile +++ b/fs/Makefile @@ -16,7 +16,7 @@ obj-y := open.o read_write.o file_table.o super.o \ stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \ fs_types.o fs_context.o fs_parser.o fsopen.o init.o \ kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o \ - file_attr.o + file_attr.o fs_revocable.o
obj-$(CONFIG_BUFFER_HEAD) += buffer.o mpage.o obj-$(CONFIG_PROC_FS) += proc_namespace.o diff --git a/fs/fs_revocable.c b/fs/fs_revocable.c new file mode 100644 index 000000000000..4b6d755abfed --- /dev/null +++ b/fs/fs_revocable.c @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2025 Google LLC + * + * File operation replacement with Revocable + */ + +#include <linux/cleanup.h> +#include <linux/fs_revocable.h> +#include <linux/poll.h> +#include <linux/revocable.h> + +struct fs_revocable_replacement { + const struct fs_revocable_operations *frops; + const struct file_operations *orig_fops; + struct file_operations fops; + struct revocable **revs; + size_t num_revs; +}; + +static int fs_revocable_try_access(struct file *filp) +{ + struct fs_revocable_replacement *rr = filp->f_rr; + + return rr->frops->try_access(rr->revs, rr->num_revs, + filp->private_data); +} + +static void fs_revocable_withdraw_access(struct fs_revocable_replacement *rr) +{ + for (size_t i = 0; i < rr->num_revs; ++i) + revocable_withdraw_access(rr->revs[i]); +} + +DEFINE_FREE(fs_revocable_replacement, struct fs_revocable_replacement *, + if (_T) fs_revocable_withdraw_access(_T)) + +static ssize_t fs_revocable_read(struct file *filp, char __user *buffer, + size_t length, loff_t *offset) +{ + struct fs_revocable_replacement *rr + __free(fs_revocable_replacement) = filp->f_rr; + int ret; + + ret = fs_revocable_try_access(filp); + if (ret) + return ret; + return rr->orig_fops->read(filp, buffer, length, offset); +} + +static __poll_t fs_revocable_poll(struct file *filp, poll_table *wait) +{ + struct fs_revocable_replacement *rr + __free(fs_revocable_replacement) = filp->f_rr; + int ret; + + ret = fs_revocable_try_access(filp); + if (ret) + return ret; + return rr->orig_fops->poll(filp, wait); +} + +static long fs_revocable_unlocked_ioctl(struct file *filp, unsigned int cmd, + unsigned long arg) +{ + struct fs_revocable_replacement *rr + __free(fs_revocable_replacement) = filp->f_rr; + int ret; + + ret = fs_revocable_try_access(filp); + if (ret) + return ret; + return rr->orig_fops->unlocked_ioctl(filp, cmd, arg); +} + +static int fs_revocable_release(struct inode *inode, struct file *filp) +{ + int ret = 0; + struct fs_revocable_replacement *rr = filp->f_rr; + + if (!rr->orig_fops->release) + goto recover; + + ret = fs_revocable_try_access(filp); + if (ret) + goto recover; + + ret = rr->orig_fops->release(inode, filp); + + fs_revocable_withdraw_access(rr); +recover: + filp->f_op = rr->orig_fops; + filp->f_rr = NULL; + + for (size_t i = 0; i < rr->num_revs; ++i) + revocable_free(rr->revs[i]); + kfree(rr->revs); + kfree(rr); + + return ret; +} + +/** + * fs_revocable_replace() - Replace the file operations to be revocable-aware. + * + * Should be used only from ->open() instances. + */ +int fs_revocable_replace(struct file *filp, + const struct fs_revocable_operations *frops, + struct revocable_provider **rps, size_t num_rps) +{ + struct fs_revocable_replacement *rr; + size_t i; + + rr = kzalloc(sizeof(*rr), GFP_KERNEL); + if (!rr) + return -ENOMEM; + filp->f_rr = rr; + + rr->frops = frops; + rr->revs = kcalloc(num_rps, sizeof(*rr->revs), GFP_KERNEL); + if (!rr->revs) + goto free_rr; + for (i = 0; i < num_rps; ++i) { + rr->revs[i] = revocable_alloc(rps[i]); + if (!rr->revs[i]) + goto free_revs; + } + rr->num_revs = num_rps; + rr->orig_fops = filp->f_op; + + memcpy(&rr->fops, filp->f_op, sizeof(rr->fops)); + rr->fops.release = fs_revocable_release; + + if (rr->fops.read) + rr->fops.read = fs_revocable_read; + if (rr->fops.poll) + rr->fops.poll = fs_revocable_poll; + if (rr->fops.unlocked_ioctl) + rr->fops.unlocked_ioctl = fs_revocable_unlocked_ioctl; + + filp->f_op = &rr->fops; + return 0; +free_revs: + if (i) { + while (--i) + revocable_free(rr->revs[i]); + } + kfree(rr->revs); +free_rr: + kfree(rr); + return -ENOMEM; +} +EXPORT_SYMBOL_GPL(fs_revocable_replace); diff --git a/include/linux/fs.h b/include/linux/fs.h index 68c4a59ec8fb..a03dd7f343a9 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -80,6 +80,7 @@ struct fs_context; struct fs_parameter_spec; struct file_kattr; struct iomap_ops; +struct fs_revocable_replacement;
extern void __init inode_init(void); extern void __init inode_init_early(void); @@ -1248,6 +1249,7 @@ struct file { }; file_ref_t f_ref; /* --- cacheline 3 boundary (192 bytes) --- */ + struct fs_revocable_replacement *f_rr; } __randomize_layout __attribute__((aligned(4))); /* lest something weird decides that 2 is OK */
diff --git a/include/linux/fs_revocable.h b/include/linux/fs_revocable.h new file mode 100644 index 000000000000..bb066392d232 --- /dev/null +++ b/include/linux/fs_revocable.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright 2025 Google LLC + */ + +#ifndef __LINUX_FS_REVOCABLE_H +#define __LINUX_FS_REVOCABLE_H + +#include <linux/fs.h> +#include <linux/revocable.h> + +struct fs_revocable_operations { + int (*try_access)(struct revocable **revs, size_t num_revs, void *data); +}; + +int fs_revocable_replace(struct file *filp, + const struct fs_revocable_operations *frops, + struct revocable_provider **rps, size_t num_rps); + +#endif /* __LINUX_FS_REVOCABLE_H */
On Thu, Oct 16, 2025 at 05:42:02AM +0000, Tzung-Bi Shih wrote:
Introduce fs_revocable_replace() to simplify the use of the revocable API with file_operations.
The function, should be called from a driver's ->open(), replaces the fops with a wrapper that automatically handles the `try_access` and `withdraw_access`.
When the file is closed, the wrapper's ->release() restores the original fops and cleanups. This centralizes the revocable logic, making drivers cleaner and easier to maintain.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org
PoC patch.
Known issues:
- All file operations call revocable_try_access() for guaranteeing the resource even if the resource may be unused in the fops.
Why is this so complicated??
You already added a per-flip struct:
+struct fs_revocable_replacement {
- const struct fs_revocable_operations *frops;
- const struct file_operations *orig_fops;
- struct file_operations fops;
- struct revocable **revs;
- size_t num_revs;
+};
Why does it need so much junk in it?
struct fs_revocable_replacement { struct srcu_struct srcu; bool *alive; };
That's it. When the caller sets this up it provides a bool * pointer from it's own private struct that is kept krefcounted to life cycle of the struct file.
Then the ops wrapers are a simple thing - generate them with a macro:
srcu_read_lock(&f_rr->srcu); if (*f_rr_>alive) ret = f_rr->orig_fops->XX(...) else ret = -ENODEV; srcu_read_unlock(&f_rr->srcu); return ret;
No need for all this revokable maze to do somethinig so simple.
Also, I don't think srcu is a good idea for this use case, maybe as an option, but the default should be to use rwsem.
Jason
On Thu, Oct 16, 2025 at 09:31:49AM -0300, Jason Gunthorpe wrote:
On Thu, Oct 16, 2025 at 05:42:02AM +0000, Tzung-Bi Shih wrote:
Introduce fs_revocable_replace() to simplify the use of the revocable API with file_operations.
The function, should be called from a driver's ->open(), replaces the fops with a wrapper that automatically handles the `try_access` and `withdraw_access`.
When the file is closed, the wrapper's ->release() restores the original fops and cleanups. This centralizes the revocable logic, making drivers cleaner and easier to maintain.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org
PoC patch.
Known issues:
- All file operations call revocable_try_access() for guaranteeing the resource even if the resource may be unused in the fops.
Why is this so complicated??
You already added a per-flip struct:
+struct fs_revocable_replacement {
- const struct fs_revocable_operations *frops;
- const struct file_operations *orig_fops;
- struct file_operations fops;
- struct revocable **revs;
- size_t num_revs;
+};
Why does it need so much junk in it?
struct fs_revocable_replacement { struct srcu_struct srcu; bool *alive; };
That's it. When the caller sets this up it provides a bool * pointer from it's own private struct that is kept krefcounted to life cycle of the struct file.
Then the ops wrapers are a simple thing - generate them with a macro:
srcu_read_lock(&f_rr->srcu); if (*f_rr_>alive) ret = f_rr->orig_fops->XX(...) else ret = -ENODEV; srcu_read_unlock(&f_rr->srcu); return ret;
No need for all this revokable maze to do somethinig so simple.
Imagining the following example:
/* res1 and res2 are provided by hot-pluggable devices. */ struct filp_priv { void *res1; void *res2; };
/* In .open() fops */ priv = kzalloc(sizeof(struct filp_priv), ...); priv->res1 = ...; priv->res2 = ...; filp->private_data = priv;
/* In .read() fops */ priv = filp->private_data; priv->res1 // could result UAF if the device has gone priv->res2 // could result UAF if the device has gone
How does the bool * work for the example?
On Fri, Oct 17, 2025 at 02:36:58AM +0000, Tzung-Bi Shih wrote:
Imagining the following example:
/* res1 and res2 are provided by hot-pluggable devices. */ struct filp_priv { void *res1; void *res2; };
/* In .open() fops */ priv = kzalloc(sizeof(struct filp_priv), ...); priv->res1 = ...; priv->res2 = ...; filp->private_data = priv;
/* In .read() fops */ priv = filp->private_data; priv->res1 // could result UAF if the device has gone priv->res2 // could result UAF if the device has gone
How does the bool * work for the example?
You are thinking about it completely wrong, you are trying to keep the driver running conccurrently after it's remove returns - but that isn't how Linux drivers are designed.
We have a whole family of synchronous fencing APIs that drivers call in their remove() callback to shut down their concurrency. Think of things like free_irq(), cancel_work_sync(), timer_shutdown_sync(), sysfs_remove_files(). All of these guarentee the concurrent callbacks are fenced before returning.
The only issue with cros_ec is this:
static void cros_ec_chardev_remove(struct platform_device *pdev) { struct miscdevice *misc = dev_get_drvdata(&pdev->dev);
misc_deregister(misc); }
It doesn't fence the cdevs! Misc is a hard API to use because it doesn't have a misc_deregister_sync() variation!
Dan/Laurent's point and proposal was that mis_deregister() does not work like this! It is an anomaly that driver authors typically over look.
So the proposal was to add some way to get a: misc_deregister_sync()
What gives the fence. Under your proposal it would lock the SRCU and change the bool. After it returns no cdev related threads are running in fops touching res1/res2. I think your proposal to replace the fops and that related machinery is smart and has a chance to succeed.
From this perspective your example is malformed. Resources should not become revoked concurrently *while a driver is bound*. The driver should be unbound, call misc_deregister_sync()/etc, and return from remove() guaranteeing it no longer touches any resources.
For this specific cros_ec driver it's "res" is this:
struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent); struct cros_ec_platform *ec_platform = dev_get_platdata(ec->dev);
This is already properly lifetime controlled!
It *HAS* to be, and even your patches are assuming it by blindly reaching into the parent's memory!
+ misc->rps[0] = ec->ec_dev->revocable_provider;
If the parent driver has been racily unbound at this point the ec->ec_dev is already a UAF!
For cros it is safe because the cros_ec driver is a child of a MFD and the MFD logic ensures that the children are unbound as part of destroying the parent. So 'ec' is guarenteed valid from probe() to remove() return.
IHMO auto-revoke is a terrible idea, if you go down that path then why is misc special? You need to auto-revoke irqs, timers, work queues, etc too? That's a mess.
I think your previous idea for revoke was properly formed, the issue and objection was that the bug you are fixing is a miscdev complexity caused by the lack of misc_deregister_sync(). If you fix that directly then you don't need recovable at all, and it is a much more useful fix that is an easy and natural API for drivers to use.
Jason
On Fri, Oct 17, 2025 at 10:49:16AM -0300, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 02:36:58AM +0000, Tzung-Bi Shih wrote:
Imagining the following example:
/* res1 and res2 are provided by hot-pluggable devices. */ struct filp_priv { void *res1; void *res2; };
/* In .open() fops */ priv = kzalloc(sizeof(struct filp_priv), ...); priv->res1 = ...; priv->res2 = ...; filp->private_data = priv;
/* In .read() fops */ priv = filp->private_data; priv->res1 // could result UAF if the device has gone priv->res2 // could result UAF if the device has gone
How does the bool * work for the example?
You are thinking about it completely wrong, you are trying to keep the driver running conccurrently after it's remove returns - but that isn't how Linux drivers are designed.
We have a whole family of synchronous fencing APIs that drivers call in their remove() callback to shut down their concurrency. Think of things like free_irq(), cancel_work_sync(), timer_shutdown_sync(), sysfs_remove_files(). All of these guarentee the concurrent callbacks are fenced before returning.
The only issue with cros_ec is this:
static void cros_ec_chardev_remove(struct platform_device *pdev) { struct miscdevice *misc = dev_get_drvdata(&pdev->dev);
misc_deregister(misc);
}
It doesn't fence the cdevs! Misc is a hard API to use because it doesn't have a misc_deregister_sync() variation!
Dan/Laurent's point and proposal was that mis_deregister() does not work like this! It is an anomaly that driver authors typically over look.
So the proposal was to add some way to get a: misc_deregister_sync()
What gives the fence. Under your proposal it would lock the SRCU and change the bool. After it returns no cdev related threads are running in fops touching res1/res2. I think your proposal to replace the fops and that related machinery is smart and has a chance to succeed.
From this perspective your example is malformed. Resources should not become revoked concurrently *while a driver is bound*. The driver should be unbound, call misc_deregister_sync()/etc, and return from remove() guaranteeing it no longer touches any resources.
Imagining: - Driver X provides the res1. - Driver Y provides the res2. - Driver Z provides the chardev /dev/zzz. The file operations use res1 and res2. - A userspace program opened /dev/zzz.
In the approach, what is .remove() of driver X supposed to do when driver X is unbinding (e.g. due to device unplug)?
If it ends up call misc_deregister_sync(), should the synchronous function wait for the userspace program to close the FD?
The design behind revocable: the driver X waits via synchronize_srcu(), and then it is free to go. The subsequent accesses to res1 will get NULL, and should fail gracefully.
For this specific cros_ec driver it's "res" is this:
struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent); struct cros_ec_platform *ec_platform = dev_get_platdata(ec->dev);
In fact, no, the "res" we are concerning is struct cros_ec_device, e.g. [1]. (I knew the naming cros_ec_device vs. cros_ec_dev is somehow easy to confuse.)
[1] https://elixir.bootlin.com/linux/v6.17/source/drivers/platform/chrome/cros_e...
This is already properly lifetime controlled!
It *HAS* to be, and even your patches are assuming it by blindly reaching into the parent's memory!
- misc->rps[0] = ec->ec_dev->revocable_provider;
If the parent driver has been racily unbound at this point the ec->ec_dev is already a UAF!
Not really, it uses the fact that the caller is from probe(). I think the driver can't be unbound when it is still in probe(). (Probe protocol device -> register the MFD device -> add cros-ec-chardev device and probe.)
For cros it is safe because the cros_ec driver is a child of a MFD and the MFD logic ensures that the children are unbound as part of destroying the parent. So 'ec' is guarenteed valid from probe() to remove() return.
IHMO auto-revoke is a terrible idea, if you go down that path then why is misc special? You need to auto-revoke irqs, timers, work queues, etc too? That's a mess.
To be clear, I'm using misc as an example which is also the real crash we observed. If the file operations use other resources provided by a hot-pluggable device, it'd need to use revocable APIs to prevent the UAFs.
On Sat, Oct 18, 2025 at 12:07:58AM +0800, Tzung-Bi Shih wrote:
From this perspective your example is malformed. Resources should not become revoked concurrently *while a driver is bound*. The driver should be unbound, call misc_deregister_sync()/etc, and return from remove() guaranteeing it no longer touches any resources.
Imagining:
- Driver X provides the res1.
- Driver Y provides the res2.
- Driver Z provides the chardev /dev/zzz. The file operations use res1 and res2.
- A userspace program opened /dev/zzz.
Yes, then you have a mess and it is pretty hard to deal with.
We don't usually build things like that, and I'm not aware of any cases where killing the whole char dev is the right answer. Usually it's something like 'res1' is critical and 'res2' is discovered optionally.
Making up elaborate fictional use cases is not a way to justify an over complex design.
If it ends up call misc_deregister_sync(), should the synchronous function wait for the userspace program to close the FD?
Some subsystems do this, it isn't great.
The design behind revocable: the driver X waits via synchronize_srcu(), and then it is free to go. The subsequent accesses to res1 will get NULL, and should fail gracefully.
Yes, I understand what it is for, I am saying it is not required here.
For this specific cros_ec driver it's "res" is this:
struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent); struct cros_ec_platform *ec_platform = dev_get_platdata(ec->dev);
In fact, no, the "res" we are concerning is struct cros_ec_device, e.g. [1]. (I knew the naming cros_ec_device vs. cros_ec_dev is somehowg easy to confuse.)
struct cros_ec_dev *ec = dev_get_drvdata(mdev->parent); struct cros_ec_device *ec_dev = ec->ec_dev;
It's all the same stuff. The parent needs to ensure it remains bound only while it's ec->ec_dev is valid. That is its job.
This is already properly lifetime controlled!
It *HAS* to be, and even your patches are assuming it by blindly reaching into the parent's memory!
- misc->rps[0] = ec->ec_dev->revocable_provider;
If the parent driver has been racily unbound at this point the ec->ec_dev is already a UAF!
Not really, it uses the fact that the caller is from probe(). I think the driver can't be unbound when it is still in probe().
Right, but that's my point you are already relying on driver binding lifetime rules to make your access valid. You should continue to rely on that and fix the lack of synchronous remove to fix the bug.
To be clear, I'm using misc as an example which is also the real crash we observed. If the file operations use other resources provided by a hot-pluggable device, it'd need to use revocable APIs to prevent the UAFs.
I understand, but it is a very poor use of this new construct. Come with a driver that actually has two resources and needs something so complicated first.
Improve miscdev as Laurent suggested to fix this specific driver bug.
Do not mess up char dev by trying to link it to lists of recovable and build some wild auto-revoking thing nobody needs.
Jason
On 10/17/25 6:07 PM, Tzung-Bi Shih wrote:
Imagining:
- Driver X provides the res1.
- Driver Y provides the res2.
- Driver Z provides the chardev /dev/zzz. The file operations use res1 and res2.
- A userspace program opened /dev/zzz.
In the approach, what is .remove() of driver X supposed to do when driver X is unbinding (e.g. due to device unplug)?
There are use-cases for revocable, but this example is a bit too generic:
Drivers don't just share device resources with other random drivers, they usually have a defined relationship through a bus.
For instance, if you have a driver on the platform bus and another driver connected through the auxiliary bus, there is a defined lifetime: The auxiliary device has to be unbound before its parent device.
This means that as long as you are in a scope where your auxiliary device is bound, it is safe to use a device resource from that parent without further checks.
The goal should always be to proof to be in such a scope when accessing device resources (in Rust we can let the compiler ensure this at compile time :).
However, there are rare (yet valid) cases where such a scope can't be guaranteed. DRM has such cases, and, unfortunately, MISC device seems to be another one.
I know the reasons why DRM has to have this design, I'm not sure about MISC device though. Unless there's a good reason, I think MISC device should be "fenced" instead.
On Fri, Oct 17, 2025 at 06:29:10PM +0200, Danilo Krummrich wrote:
I'm not sure about MISC device though. Unless there's a good reason, I think MISC device should be "fenced" instead.
misc is a very small wrapper around raw fops, and raw fops are optimized for performance. Adding locking that many important things like normal files don't need to all fops would not be agreed.
The sketch in this series where we have a core helper to provide a shim fops that adds on the lock is smart and I think could be an agreeable way to make a synchronous misc and cdev unregister for everyone to trivially use.
Jason
On 10/17/25 6:37 PM, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 06:29:10PM +0200, Danilo Krummrich wrote:
I'm not sure about MISC device though. Unless there's a good reason, I think MISC device should be "fenced" instead.
misc is a very small wrapper around raw fops, and raw fops are optimized for performance. Adding locking that many important things like normal files don't need to all fops would not be agreed.
The sketch in this series where we have a core helper to provide a shim fops that adds on the lock is smart and I think could be an agreeable way to make a synchronous misc and cdev unregister for everyone to trivially use.
Sure, for MISC devices without a parent for instance there are no device resources to access anyways.
On Fri, Oct 17, 2025 at 08:19:06PM +0200, Danilo Krummrich wrote:
On 10/17/25 6:37 PM, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 06:29:10PM +0200, Danilo Krummrich wrote:
I'm not sure about MISC device though. Unless there's a good reason, I think MISC device should be "fenced" instead.
misc is a very small wrapper around raw fops, and raw fops are optimized for performance. Adding locking that many important things like normal files don't need to all fops would not be agreed.
The sketch in this series where we have a core helper to provide a shim fops that adds on the lock is smart and I think could be an agreeable way to make a synchronous misc and cdev unregister for everyone to trivially use.
Sure, for MISC devices without a parent for instance there are no device resources to access anyways.
There are many situations with misc that can get people into trouble without parent:
misc_deregister(x); timer_shutdown_sync(y); kfree(z);
For example. It is is buggy if the fops touch y or z.
This is why a _sync version is such a nice clean idea because with 5 letters the above can just be fixed.
Wrapping everything in a revocable would be a huge PITA.
Jason
On Fri Oct 17, 2025 at 8:44 PM CEST, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 08:19:06PM +0200, Danilo Krummrich wrote:
On 10/17/25 6:37 PM, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 06:29:10PM +0200, Danilo Krummrich wrote:
I'm not sure about MISC device though. Unless there's a good reason, I think MISC device should be "fenced" instead.
misc is a very small wrapper around raw fops, and raw fops are optimized for performance. Adding locking that many important things like normal files don't need to all fops would not be agreed.
The sketch in this series where we have a core helper to provide a shim fops that adds on the lock is smart and I think could be an agreeable way to make a synchronous misc and cdev unregister for everyone to trivially use.
Sure, for MISC devices without a parent for instance there are no device resources to access anyways.
There are many situations with misc that can get people into trouble without parent:
misc_deregister(x); timer_shutdown_sync(y); kfree(z);
For example. It is is buggy if the fops touch y or z.
This is why a _sync version is such a nice clean idea because with 5 letters the above can just be fixed.
Wrapping everything in a revocable would be a huge PITA.
That's a bit of a different problem though. Revocable clearly isn't the solution. _sync() works, but doesn't account for the actual problem, which is that the file private has at least shared ownership of y and z.
So, it's more of an ownership / lifetime problem. The file private data should either own y and z entirely or a corresponding reference count that is dropped in fops release().
Device resources are different though, since we can't just hold on to them with a reference count etc.; they're strictly gone once the bus device is unbound, hence revocable when there is no _sync().
On Fri, Oct 17, 2025 at 11:41:56PM +0200, Danilo Krummrich wrote:
On Fri Oct 17, 2025 at 8:44 PM CEST, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 08:19:06PM +0200, Danilo Krummrich wrote:
On 10/17/25 6:37 PM, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 06:29:10PM +0200, Danilo Krummrich wrote:
I'm not sure about MISC device though. Unless there's a good reason, I think MISC device should be "fenced" instead.
misc is a very small wrapper around raw fops, and raw fops are optimized for performance. Adding locking that many important things like normal files don't need to all fops would not be agreed.
The sketch in this series where we have a core helper to provide a shim fops that adds on the lock is smart and I think could be an agreeable way to make a synchronous misc and cdev unregister for everyone to trivially use.
Sure, for MISC devices without a parent for instance there are no device resources to access anyways.
There are many situations with misc that can get people into trouble without parent:
misc_deregister(x); timer_shutdown_sync(y); kfree(z);
For example. It is is buggy if the fops touch y or z.
This is why a _sync version is such a nice clean idea because with 5 letters the above can just be fixed.
Wrapping everything in a revocable would be a huge PITA.
That's a bit of a different problem though. Revocable clearly isn't the solution. _sync() works, but doesn't account for the actual problem, which is that the file private has at least shared ownership of y and z.
So, it's more of an ownership / lifetime problem. The file private data should either own y and z entirely or a corresponding reference count that is dropped in fops release().
I think both versions are popular in the kernel.. You can legimately treat y and z the same as device resources without creating a correctness problem and it is less code.
You can also do refcounts.
For instance at least in C you'd never argue that people should use refcount private data when they use a timer or irq subsystem. You'd use a simple sync cleanup and be done with it.
These bugs are coming because of mixing models, we have a bunch of sync APIs in the kernel that are easy to use and then you get these weird non-sync ones without any clear documentation :)
Jason
Hi,
On 10/15/25 10:42 PM, Tzung-Bi Shih wrote:
+/**
- fs_revocable_replace() - Replace the file operations to be revocable-aware.
- Should be used only from ->open() instances.
- */
+int fs_revocable_replace(struct file *filp,
const struct fs_revocable_operations *frops,
struct revocable_provider **rps, size_t num_rps)
+{
Please add the function parameters to the kernel-doc comment to avoid kernel-doc warnings. E.g.:
* @filp: foo description * @frops: bar description * @rps: baz description * @num_rps: number of @rps entries
On Thu, Oct 16, 2025 at 11:38:27AM -0700, Randy Dunlap wrote:
On 10/15/25 10:42 PM, Tzung-Bi Shih wrote:
+/**
- fs_revocable_replace() - Replace the file operations to be revocable-aware.
- Should be used only from ->open() instances.
- */
+int fs_revocable_replace(struct file *filp,
const struct fs_revocable_operations *frops,
struct revocable_provider **rps, size_t num_rps)
+{
Please add the function parameters to the kernel-doc comment to avoid kernel-doc warnings. E.g.:
Ah, thanks. I didn't intend to make it a kernel-doc (as PoC code) but just wrongly copy-n-paste around.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- PoC patch.
v5: - No primary changes but modify the API usage accordingly to support multiple revocable providers.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-7-tzungbi@kern... - New in the series.
drivers/char/misc.c | 8 ++++++++ include/linux/miscdevice.h | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/drivers/char/misc.c b/drivers/char/misc.c index 726516fb0a3b..5b412e18b8a6 100644 --- a/drivers/char/misc.c +++ b/drivers/char/misc.c @@ -50,6 +50,7 @@ #include <linux/tty.h> #include <linux/kmod.h> #include <linux/gfp.h> +#include <linux/fs_revocable.h>
/* * Head entry for the doubly linked miscdevice list @@ -159,6 +160,13 @@ static int misc_open(struct inode *inode, struct file *file)
err = 0; replace_fops(file, new_fops); + + if (c->frops && c->rps) { + err = fs_revocable_replace(file, c->frops, c->rps, c->num_rps); + if (err) + goto fail; + } + if (file->f_op->open) err = file->f_op->open(inode, file); fail: diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h index 7d0aa718499c..b7024a0cebb9 100644 --- a/include/linux/miscdevice.h +++ b/include/linux/miscdevice.h @@ -5,6 +5,7 @@ #include <linux/list.h> #include <linux/types.h> #include <linux/device.h> +#include <linux/revocable.h>
/* * These allocations are managed by device@lanana.org. If you need @@ -92,6 +93,9 @@ struct miscdevice { const struct attribute_group **groups; const char *nodename; umode_t mode; + struct revocable_provider **rps; + size_t num_rps; + const struct fs_revocable_operations *frops; };
extern int misc_register(struct miscdevice *misc);
Miscdevice now supports revocable fops replacement. Use it to secure the cros_ec_device.
Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org --- PoC patch.
v5: - No primary changes but modify the API usage accordingly.
v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-8-tzungbi@kern... - New in the series.
drivers/platform/chrome/cros_ec_chardev.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c index c9d80ad5b57e..01691b023d7e 100644 --- a/drivers/platform/chrome/cros_ec_chardev.c +++ b/drivers/platform/chrome/cros_ec_chardev.c @@ -13,6 +13,7 @@ #include <linux/init.h> #include <linux/device.h> #include <linux/fs.h> +#include <linux/fs_revocable.h> #include <linux/miscdevice.h> #include <linux/mod_devicetable.h> #include <linux/module.h> @@ -166,7 +167,6 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp) if (!priv) return -ENOMEM;
- priv->ec_dev = ec_dev; priv->cmd_offset = ec->cmd_offset; filp->private_data = priv; INIT_LIST_HEAD(&priv->events); @@ -370,6 +370,19 @@ static const struct file_operations chardev_fops = { #endif };
+static int cros_ec_chardev_rev_try_access(struct revocable **revs, + size_t num_revs, void *data) +{ + struct chardev_priv *priv = data; + + priv->ec_dev = revocable_try_access(revs[0]); + return priv->ec_dev ? 0 : -ENODEV; +} + +static const struct fs_revocable_operations cros_ec_chardev_frops = { + .try_access = cros_ec_chardev_rev_try_access, +}; + static int cros_ec_chardev_probe(struct platform_device *pdev) { struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent); @@ -386,6 +399,13 @@ static int cros_ec_chardev_probe(struct platform_device *pdev) misc->name = ec_platform->ec_name; misc->parent = pdev->dev.parent;
+ misc->rps = devm_kcalloc(&pdev->dev, 1, sizeof(*misc->rps), GFP_KERNEL); + if (!misc->rps) + return -ENOMEM; + misc->rps[0] = ec->ec_dev->revocable_provider; + misc->num_rps = 1; + misc->frops = &cros_ec_chardev_frops; + dev_set_drvdata(&pdev->dev, misc);
return misc_register(misc);
linux-kselftest-mirror@lists.linaro.org