diff options
| author | Johan Hovold <johan@kernel.org> | 2026-02-04 17:28:49 +0300 |
|---|---|---|
| committer | Greg Kroah-Hartman <gregkh@linuxfoundation.org> | 2026-02-06 18:08:48 +0300 |
| commit | 21bab791346e5b7902a04709231c0642ff6d69bc (patch) | |
| tree | 9745de59f6794843d33e096fb93e809ae4ae32ac /include/linux | |
| parent | 7149ce34dd48886b3f69153c7f5533dd3fd5f47e (diff) | |
| download | linux-21bab791346e5b7902a04709231c0642ff6d69bc.tar.xz | |
Revert "revocable: Revocable resource management"
This reverts commit 62eb557580eb2177cf16c3fd2b6efadff297b29a.
The revocable implementation uses two separate abstractions, struct
revocable_provider and struct revocable, in order to store the SRCU read
lock index which must be passed unaltered to srcu_read_unlock() in the
same context when a resource is no longer needed.
With the merged revocable API, multiple threads could however share the
same struct revocable and therefore potentially overwrite the SRCU index
of another thread which can cause the SRCU synchronisation in
revocable_provider_revoke() to never complete. [1]
An example revocable conversion of the gpiolib code also turned out to
be fundamentally flawed and could lead to use-after-free. [2]
An attempt to address both issues was quickly put together and merged,
but revocable is still fundamentally broken. [3]
Specifically, the latest design relies on RCU for storing a pointer to
the revocable provider, but since the resource can be shared by value
(e.g. as in the now reverted selftests) this does not work at all and
can also lead to use-after-free:
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_rcu(rp, rcu);
}
void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
{
struct revocable_provider *rp;
rp = rcu_replace_pointer(*rp_ptr, NULL, 1);
...
kref_put(&rp->kref, revocable_provider_release);
}
int revocable_init(struct revocable_provider __rcu *_rp,
struct revocable *rev)
{
struct revocable_provider *rp;
...
scoped_guard(rcu) {
rp = rcu_dereference(_rp);
if (!rp)
return -ENODEV;
if (!kref_get_unless_zero(&rp->kref))
return -ENODEV;
}
...
}
producer:
priv->rp = revocable_provider_alloc(&priv->res);
// pass priv->rp by value to consumer
revocable_provider_revoke(&priv->rp);
consumer:
struct revocable_provider __rcu *rp = filp->private_data;
struct revocable *rev;
revocable_init(rp, &rev);
as _rp would still be non-NULL in revocable_init() regardless of whether
the producer has revoked the resource and set its pointer to NULL.
Essentially revocable still relies on having a pointer to reference
counted driver data which holds the revocable provider, which makes all
the RCU protection unnecessary along with most of the current revocable
design and implementation.
As the above shows, and as has been pointed out repeatedly elsewhere,
these kind of issues are not something that should be addressed
incrementally. [4]
Revert the revocable implementation until a redesign has been proposed
and evaluated properly.
Link: https://lore.kernel.org/all/20260124170535.11756-4-johan@kernel.org/ [1]
Link: https://lore.kernel.org/all/aXT45B6vLf9R3Pbf@hovoldconsulting.com/ [2]
Link: https://lore.kernel.org/all/20260129143733.45618-1-tzungbi@kernel.org/ [3]
Link: https://lore.kernel.org/all/aXobzoeooJqxMkEj@hovoldconsulting.com/ [4]
Signed-off-by: Johan Hovold <johan@kernel.org>
Link: https://patch.msgid.link/20260204142849.22055-4-johan@kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Diffstat (limited to 'include/linux')
| -rw-r--r-- | include/linux/revocable.h | 89 |
1 files changed, 0 insertions, 89 deletions
diff --git a/include/linux/revocable.h b/include/linux/revocable.h deleted file mode 100644 index e3d6d2c953a3..000000000000 --- a/include/linux/revocable.h +++ /dev/null @@ -1,89 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -/* - * Copyright 2026 Google LLC - */ - -#ifndef __LINUX_REVOCABLE_H -#define __LINUX_REVOCABLE_H - -#include <linux/compiler.h> -#include <linux/cleanup.h> - -struct device; -struct revocable_provider; - -/** - * 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; -}; - -struct revocable_provider __rcu *revocable_provider_alloc(void *res); -void revocable_provider_revoke(struct revocable_provider __rcu **rp); - -int revocable_init(struct revocable_provider __rcu *rp, struct revocable *rev); -void revocable_deinit(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(access_rev, struct revocable *, { - if ((_T)->idx != -1) - revocable_withdraw_access(_T); - if ((_T)->rp) - revocable_deinit(_T); -}) - -#define _REVOCABLE_TRY_ACCESS_WITH(_rp, _rev, _res) \ - struct revocable _rev = {.rp = NULL, .idx = -1}; \ - struct revocable *__UNIQUE_ID(name) __free(access_rev) = &_rev; \ - _res = revocable_init(_rp, &_rev) ? NULL : revocable_try_access(&_rev) - -/** - * REVOCABLE_TRY_ACCESS_WITH() - A helper for accessing revocable resource - * @_rp: The provider's ``struct revocable_provider *`` handle. - * @_res: A pointer variable that will be assigned the resource. - * - * The macro simplifies the access-release cycle for consumers, ensuring that - * corresponding revocable_withdraw_access() and revocable_deinit() are called, - * even in the case of an early exit. - * - * It creates a local variable in the current scope. @_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 is exited. - * - * Note: It shares the same issue with guard() in cleanup.h. No goto statements - * are allowed before the helper. Otherwise, the compiler fails with - * "jump bypasses initialization of variable with __attribute__((cleanup))". - */ -#define REVOCABLE_TRY_ACCESS_WITH(_rp, _res) \ - _REVOCABLE_TRY_ACCESS_WITH(_rp, __UNIQUE_ID(name), _res) - -#define _REVOCABLE_TRY_ACCESS_SCOPED(_rp, _rev, _label, _res) \ - for (struct revocable _rev = {.rp = NULL, .idx = -1}, \ - *__UNIQUE_ID(name) __free(access_rev) = &_rev; \ - (_res = revocable_init(_rp, &_rev) ? NULL : \ - revocable_try_access(&_rev)) || true; \ - ({ goto _label; })) \ - if (0) { \ -_label: \ - break; \ - } else - -/** - * REVOCABLE_TRY_ACCESS_SCOPED() - A helper for accessing revocable resource - * @_rp: The provider's ``struct revocable_provider *`` handle. - * @_res: A pointer variable that will be assigned the resource. - * - * Similar to REVOCABLE_TRY_ACCESS_WITH() but with an explicit scope from a - * temporary ``for`` loop. - */ -#define REVOCABLE_TRY_ACCESS_SCOPED(_rp, _res) \ - _REVOCABLE_TRY_ACCESS_SCOPED(_rp, __UNIQUE_ID(name), \ - __UNIQUE_ID(label), _res) - -#endif /* __LINUX_REVOCABLE_H */ |
