[PATCH 0/4] revocable: Fix reported race conditions

[PATCH 0/4] revocable: Fix reported race conditions

[Tzung-Bi Shih]

The 1st patch fixes the issue reported in [1].  The 2nd patch adds KUnit
test for the cases.

The 3rd patch fixes the issue reported in [2].  The 4th patch adds KUnit
test for one of the cases.

[1] https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/
[2] https://lore.kernel.org/all/20260124170535.11756-4-johan@kernel.org/

Tzung-Bi Shih (4):
  revocable: Fix races in revocable_alloc() using RCU
  revocable: Add KUnit test for provider lifetime races
  revocable: fix SRCU index corruption by requiring caller-provided
    storage
  revocable: Add KUnit test for concurrent access

 .../driver-api/driver-model/revocable.rst     |  17 +-
 drivers/base/revocable.c                      | 118 +++++-----
 drivers/base/revocable_test.c                 | 216 +++++++++++++++---
 include/linux/revocable.h                     |  60 +++--
 .../revocable/test_modules/revocable_test.c   |  54 ++---
 5 files changed, 300 insertions(+), 165 deletions(-)

-- 
2.53.0.rc1.217.geba53bf80e-goog

[PATCH 1/4] revocable: Fix races in revocable_alloc() using RCU

[Tzung-Bi Shih]

There are two race conditions when allocating a revocable instance:

1. After a struct revocable_provider is revoked, the caller might still
   hold a dangling pointer to it.  A subsequent call to
   revocable_alloc() can trigger a use-after-free.
2. If revocable_provider_release() runs concurrently with
   revocable_alloc(), the memory of struct revocable_provider can be
   accessed during or after kfree().

To fix these:
- Manage the lifetime of struct revocable_provider using RCU.  Annotate
  pointers to it with __rcu and use kfree_rcu() for deallocation.
- Update revocable_alloc() to safely acquire a reference using RCU
  primitives.
- Update revocable_provider_revoke() to take a double pointer (`**rp`).
  It atomically NULLs out the caller's pointer before starting
  revocation.  This prevents the caller from holding a dangling pointer.
- Drop devm_revocable_provider_alloc().  The devm-managed model cannot
  support the required double-pointer semantic for safe pointer nulling.

Reported-by: Johan Hovold <johan@kernel.org>
Closes: https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 .../driver-api/driver-model/revocable.rst     |  3 -
 drivers/base/revocable.c                      | 93 ++++++++++---------
 drivers/base/revocable_test.c                 | 20 ++--
 include/linux/revocable.h                     |  8 +-
 .../revocable/test_modules/revocable_test.c   | 19 ++--
 5 files changed, 72 insertions(+), 71 deletions(-)

diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst
index 22a442cc8d7f..ab7c0af5fbb6 100644
--- a/Documentation/driver-api/driver-model/revocable.rst
+++ b/Documentation/driver-api/driver-model/revocable.rst
@@ -76,9 +76,6 @@ 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
 
diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
index b068e18a847d..1bcd1cf54764 100644
--- a/drivers/base/revocable.c
+++ b/drivers/base/revocable.c
@@ -64,11 +64,13 @@
  * @srcu: The SRCU to protect the resource.
  * @res:  The pointer of resource.  It can point to anything.
  * @kref: The refcount for this handle.
+ * @rcu: The RCU to protect pointer to itself.
  */
 struct revocable_provider {
 	struct srcu_struct srcu;
 	void __rcu *res;
 	struct kref kref;
+	struct rcu_head rcu;
 };
 
 /**
@@ -88,8 +90,9 @@ struct revocable {
  * This holds an initial refcount to the struct.
  *
  * Return: The pointer of struct revocable_provider.  NULL on errors.
+ * It enforces the caller handles the returned pointer in RCU ways.
  */
-struct revocable_provider *revocable_provider_alloc(void *res)
+struct revocable_provider __rcu *revocable_provider_alloc(void *res)
 {
 	struct revocable_provider *rp;
 
@@ -98,10 +101,10 @@ struct revocable_provider *revocable_provider_alloc(void *res)
 		return NULL;
 
 	init_srcu_struct(&rp->srcu);
-	rcu_assign_pointer(rp->res, res);
+	RCU_INIT_POINTER(rp->res, res);
 	kref_init(&rp->kref);
 
-	return rp;
+	return (struct revocable_provider __rcu *)rp;
 }
 EXPORT_SYMBOL_GPL(revocable_provider_alloc);
 
@@ -111,82 +114,80 @@ static void revocable_provider_release(struct kref *kref)
 			struct revocable_provider, kref);
 
 	cleanup_srcu_struct(&rp->srcu);
-	kfree(rp);
+	kfree_rcu(rp, rcu);
 }
 
 /**
  * revocable_provider_revoke() - Revoke the managed resource.
- * @rp: The pointer of resource provider.
+ * @rp_ptr: The pointer of 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.
+ * It enforces the caller to pass a pointer of pointer of resource provider so
+ * that it sets \*rp_ptr to NULL to prevent from keeping a dangling pointer.
  */
-struct revocable_provider *devm_revocable_provider_alloc(struct device *dev,
-							 void *res)
+void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
 {
 	struct revocable_provider *rp;
 
-	rp = revocable_provider_alloc(res);
+	rp = rcu_replace_pointer(*rp_ptr, NULL, 1);
 	if (!rp)
-		return NULL;
+		return;
 
-	if (devm_add_action_or_reset(dev, devm_revocable_provider_revoke, rp))
-		return NULL;
-
-	return rp;
+	rcu_assign_pointer(rp->res, NULL);
+	synchronize_srcu(&rp->srcu);
+	kref_put(&rp->kref, revocable_provider_release);
 }
-EXPORT_SYMBOL_GPL(devm_revocable_provider_alloc);
+EXPORT_SYMBOL_GPL(revocable_provider_revoke);
 
 /**
  * revocable_alloc() - Allocate struct revocable.
- * @rp: The pointer of resource provider.
+ * @_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 *revocable_alloc(struct revocable_provider __rcu *_rp)
 {
+	struct revocable_provider *rp;
 	struct revocable *rev;
 
+	if (!_rp)
+		return NULL;
+
+	/*
+	 * Enter a read-side critical section.
+	 *
+	 * This prevents kfree_rcu() from freeing the struct revocable_provider
+	 * memory, for the duration of this scope.
+	 */
+	scoped_guard(rcu) {
+		rp = rcu_dereference(_rp);
+		if (!rp)
+			/* The revocable provider has been revoked. */
+			return NULL;
+
+		if (!kref_get_unless_zero(&rp->kref))
+			/*
+			 * The revocable provider is releasing (i.e.,
+			 * revocable_provider_release() has been called).
+			 */
+			return NULL;
+	}
+	/* At this point, `rp` is safe to access as holding a kref of it */
+
 	rev = kzalloc(sizeof(*rev), GFP_KERNEL);
-	if (!rev)
+	if (!rev) {
+		kref_put(&rp->kref, revocable_provider_release);
 		return NULL;
+	}
 
 	rev->rp = rp;
-	kref_get(&rp->kref);
-
 	return rev;
 }
 EXPORT_SYMBOL_GPL(revocable_alloc);
diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
index 873a44082b6c..1622aae92fd3 100644
--- a/drivers/base/revocable_test.c
+++ b/drivers/base/revocable_test.c
@@ -21,7 +21,7 @@
 
 static void revocable_test_basic(struct kunit *test)
 {
-	struct revocable_provider *rp;
+	struct revocable_provider __rcu *rp;
 	struct revocable *rev;
 	void *real_res = (void *)0x12345678, *res;
 
@@ -36,12 +36,13 @@ static void revocable_test_basic(struct kunit *test)
 	revocable_withdraw_access(rev);
 
 	revocable_free(rev);
-	revocable_provider_revoke(rp);
+	revocable_provider_revoke(&rp);
+	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 }
 
 static void revocable_test_revocation(struct kunit *test)
 {
-	struct revocable_provider *rp;
+	struct revocable_provider __rcu *rp;
 	struct revocable *rev;
 	void *real_res = (void *)0x12345678, *res;
 
@@ -55,7 +56,8 @@ static void revocable_test_revocation(struct kunit *test)
 	KUNIT_EXPECT_PTR_EQ(test, res, real_res);
 	revocable_withdraw_access(rev);
 
-	revocable_provider_revoke(rp);
+	revocable_provider_revoke(&rp);
+	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 
 	res = revocable_try_access(rev);
 	KUNIT_EXPECT_PTR_EQ(test, res, NULL);
@@ -66,7 +68,7 @@ static void revocable_test_revocation(struct kunit *test)
 
 static void revocable_test_try_access_macro(struct kunit *test)
 {
-	struct revocable_provider *rp;
+	struct revocable_provider __rcu *rp;
 	struct revocable *rev;
 	void *real_res = (void *)0x12345678, *res;
 
@@ -81,7 +83,8 @@ static void revocable_test_try_access_macro(struct kunit *test)
 		KUNIT_EXPECT_PTR_EQ(test, res, real_res);
 	}
 
-	revocable_provider_revoke(rp);
+	revocable_provider_revoke(&rp);
+	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 
 	{
 		REVOCABLE_TRY_ACCESS_WITH(rev, res);
@@ -93,7 +96,7 @@ static void revocable_test_try_access_macro(struct kunit *test)
 
 static void revocable_test_try_access_macro2(struct kunit *test)
 {
-	struct revocable_provider *rp;
+	struct revocable_provider __rcu *rp;
 	struct revocable *rev;
 	void *real_res = (void *)0x12345678, *res;
 	bool accessed;
@@ -111,7 +114,8 @@ static void revocable_test_try_access_macro2(struct kunit *test)
 	}
 	KUNIT_EXPECT_TRUE(test, accessed);
 
-	revocable_provider_revoke(rp);
+	revocable_provider_revoke(&rp);
+	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 
 	accessed = false;
 	REVOCABLE_TRY_ACCESS_SCOPED(rev, res) {
diff --git a/include/linux/revocable.h b/include/linux/revocable.h
index 659ba01c58db..d5da3584adbe 100644
--- a/include/linux/revocable.h
+++ b/include/linux/revocable.h
@@ -13,12 +13,10 @@ 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_provider __rcu *revocable_provider_alloc(void *res);
+void revocable_provider_revoke(struct revocable_provider __rcu **rp);
 
-struct revocable *revocable_alloc(struct revocable_provider *rp);
+struct revocable *revocable_alloc(struct revocable_provider __rcu *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);
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
index 1b0692eb75f3..ae6c67e65f3d 100644
--- a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
+++ b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
@@ -17,7 +17,7 @@
 static struct dentry *debugfs_dir;
 
 struct revocable_test_provider_priv {
-	struct revocable_provider *rp;
+	struct revocable_provider __rcu *rp;
 	struct dentry *dentry;
 	char res[16];
 };
@@ -25,7 +25,7 @@ struct revocable_test_provider_priv {
 static int revocable_test_consumer_open(struct inode *inode, struct file *filp)
 {
 	struct revocable *rev;
-	struct revocable_provider *rp = inode->i_private;
+	struct revocable_provider __rcu *rp = inode->i_private;
 
 	rev = revocable_alloc(rp);
 	if (!rev)
@@ -106,8 +106,8 @@ static int revocable_test_provider_release(struct inode *inode,
 	struct revocable_test_provider_priv *priv = filp->private_data;
 
 	debugfs_remove(priv->dentry);
-	if (priv->rp)
-		revocable_provider_revoke(priv->rp);
+	if (unrcu_pointer(priv->rp))
+		revocable_provider_revoke(&priv->rp);
 	kfree(priv);
 
 	return 0;
@@ -137,8 +137,8 @@ static ssize_t revocable_test_provider_write(struct file *filp,
 	 * gone.
 	 */
 	if (!strcmp(data, TEST_CMD_RESOURCE_GONE)) {
-		revocable_provider_revoke(priv->rp);
-		priv->rp = NULL;
+		revocable_provider_revoke(&priv->rp);
+		rcu_assign_pointer(priv->rp, NULL);
 	} else {
 		if (priv->res[0] != '\0')
 			return 0;
@@ -146,14 +146,15 @@ static ssize_t revocable_test_provider_write(struct file *filp,
 		strscpy(priv->res, data);
 
 		priv->rp = revocable_provider_alloc(&priv->res);
-		if (!priv->rp)
+		if (!unrcu_pointer(priv->rp))
 			return -ENOMEM;
 
 		priv->dentry = debugfs_create_file("consumer", 0400,
-						   debugfs_dir, priv->rp,
+						   debugfs_dir,
+						   unrcu_pointer(priv->rp),
 						   &revocable_test_consumer_fops);
 		if (!priv->dentry) {
-			revocable_provider_revoke(priv->rp);
+			revocable_provider_revoke(&priv->rp);
 			return -ENOMEM;
 		}
 	}
-- 
2.53.0.rc1.217.geba53bf80e-goog

[PATCH 2/4] revocable: Add KUnit test for provider lifetime races

[Tzung-Bi Shih]

Add a test to verify that revocable_alloc() correctly handles race
conditions where the provider is being released.

The test covers three scenarios:
1. Allocating from a NULL provider.
2. Allocating from a provider that has been detached (pointer is NULL).
3. Allocating from a provider that is in the process of destruction
   (refcount is 0), simulating a race between revocable_alloc() and
   revocable_provider_release().

A way to run the test:
$ ./tools/testing/kunit/kunit.py run \
	--kconfig_add CONFIG_REVOCABLE_KUNIT_TEST=y \
	--kconfig_add CONFIG_PROVE_LOCKING=y \
	--kconfig_add CONFIG_DEBUG_KERNEL=y \
	--kconfig_add CONFIG_DEBUG_INFO=y \
	--kconfig_add CONFIG_DEBUG_INFO_DWARF5=y \
	--kconfig_add CONFIG_KASAN=y \
	--kconfig_add CONFIG_DETECT_HUNG_TASK=y \
	--kconfig_add CONFIG_DEFAULT_HUNG_TASK_TIMEOUT="10" \
	--arch=x86_64 --raw_output=all \
	revocable_test

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/base/revocable_test.c | 41 +++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
index 1622aae92fd3..7fc4d6a3dff6 100644
--- a/drivers/base/revocable_test.c
+++ b/drivers/base/revocable_test.c
@@ -14,9 +14,13 @@
  *
  * - Try Access Macro: Same as "Revocation" but uses the
  *   REVOCABLE_TRY_ACCESS_WITH() and REVOCABLE_TRY_ACCESS_SCOPED().
+ *
+ * - Provider Use-after-free: Verifies revocable_alloc() correctly handles
+ *   race conditions where the provider is being released.
  */
 
 #include <kunit/test.h>
+#include <linux/refcount.h>
 #include <linux/revocable.h>
 
 static void revocable_test_basic(struct kunit *test)
@@ -127,11 +131,48 @@ static void revocable_test_try_access_macro2(struct kunit *test)
 	revocable_free(rev);
 }
 
+static void revocable_test_provider_use_after_free(struct kunit *test)
+{
+	struct revocable_provider __rcu *rp;
+	struct revocable_provider *old_rp;
+	void *real_res = (void *)0x12345678;
+	struct revocable *rev;
+
+	rp = revocable_provider_alloc(real_res);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
+
+	rev = revocable_alloc(NULL);
+	KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
+
+	/* Simulate the provider has been freed. */
+	old_rp = rcu_replace_pointer(rp, NULL, 1);
+	rev = revocable_alloc(rp);
+	KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
+	rcu_replace_pointer(rp, old_rp, 1);
+
+	struct {
+		struct srcu_struct srcu;
+		void __rcu *res;
+		struct kref kref;
+		struct rcu_head rcu;
+	} *rp_internal = (void *)rp;
+
+	/* Simulate the provider is releasing. */
+	refcount_set(&rp_internal->kref.refcount, 0);
+	rev = revocable_alloc(rp);
+	KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
+	refcount_set(&rp_internal->kref.refcount, 1);
+
+	revocable_provider_revoke(&rp);
+	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
+}
+
 static struct kunit_case revocable_test_cases[] = {
 	KUNIT_CASE(revocable_test_basic),
 	KUNIT_CASE(revocable_test_revocation),
 	KUNIT_CASE(revocable_test_try_access_macro),
 	KUNIT_CASE(revocable_test_try_access_macro2),
+	KUNIT_CASE(revocable_test_provider_use_after_free),
 	{}
 };
 
-- 
2.53.0.rc1.217.geba53bf80e-goog

[PATCH 3/4] revocable: fix SRCU index corruption by requiring caller-provided storage

[Tzung-Bi Shih]

The struct revocable handle stores the SRCU read-side index (idx) for
the duration of a resource access.  If multiple threads share the same
struct revocable instance, they race on writing to the idx field,
corrupting the SRCU state and potentially causing unsafe unlocks.

Refactor the API to replace revocable_alloc()/revocable_free() with
revocable_init()/revocable_deinit().  This change requires the caller
to provide the storage for struct revocable.

By moving storage ownership to the caller, the API ensures that
concurrent users maintain their own private idx storage, eliminating
the race condition.

Reported-by: Johan Hovold <johan@kernel.org>
Closes: https://lore.kernel.org/all/20260124170535.11756-4-johan@kernel.org/
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 .../driver-api/driver-model/revocable.rst     | 14 ++--
 drivers/base/revocable.c                      | 41 ++++-------
 drivers/base/revocable_test.c                 | 69 +++++++++----------
 include/linux/revocable.h                     | 54 ++++++++++-----
 .../revocable/test_modules/revocable_test.c   | 37 ++++------
 5 files changed, 102 insertions(+), 113 deletions(-)

diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst
index ab7c0af5fbb6..350e7faeccdc 100644
--- a/Documentation/driver-api/driver-model/revocable.rst
+++ b/Documentation/driver-api/driver-model/revocable.rst
@@ -81,14 +81,14 @@ For Resource Providers
 
 For Resource Consumers
 ----------------------
-.. kernel-doc:: drivers/base/revocable.c
+.. kernel-doc:: include/linux/revocable.h
    :identifiers: revocable
 
 .. kernel-doc:: drivers/base/revocable.c
-   :identifiers: revocable_alloc
+   :identifiers: revocable_init
 
 .. kernel-doc:: drivers/base/revocable.c
-   :identifiers: revocable_free
+   :identifiers: revocable_deinit
 
 .. kernel-doc:: drivers/base/revocable.c
    :identifiers: revocable_try_access
@@ -104,11 +104,11 @@ Example Usage
 
 .. code-block:: c
 
-    void consumer_use_resource(struct revocable *rev)
+    void consumer_use_resource(struct revocable_provider *rp)
     {
         struct foo_resource *res;
 
-        REVOCABLE_TRY_ACCESS_WITH(rev, res);
+        REVOCABLE_TRY_ACCESS_WITH(rp, res);
         // Always check if the resource is valid.
         if (!res) {
             pr_warn("Resource is not available\n");
@@ -129,11 +129,11 @@ Example Usage
 
 .. code-block:: c
 
-    void consumer_use_resource(struct revocable *rev)
+    void consumer_use_resource(struct revocable_provider *rp)
     {
         struct foo_resource *res;
 
-        REVOCABLE_TRY_ACCESS_SCOPED(rev, res) {
+        REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
             // Always check if the resource is valid.
             if (!res) {
                 pr_warn("Resource is not available\n");
diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
index 1bcd1cf54764..8532ca6a371c 100644
--- a/drivers/base/revocable.c
+++ b/drivers/base/revocable.c
@@ -73,16 +73,6 @@ struct revocable_provider {
 	struct rcu_head rcu;
 };
 
-/**
- * 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.
@@ -145,20 +135,20 @@ void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
 EXPORT_SYMBOL_GPL(revocable_provider_revoke);
 
 /**
- * revocable_alloc() - Allocate struct revocable.
+ * revocable_init() - Initialize struct revocable.
  * @_rp: The pointer of resource provider.
+ * @rev: The pointer of resource consumer.
  *
  * This holds a refcount to the resource provider.
  *
- * Return: The pointer of struct revocable.  NULL on errors.
+ * Return: 0 on success, -errno otherwise.
  */
-struct revocable *revocable_alloc(struct revocable_provider __rcu *_rp)
+int revocable_init(struct revocable_provider __rcu *_rp, struct revocable *rev)
 {
 	struct revocable_provider *rp;
-	struct revocable *rev;
 
 	if (!_rp)
-		return NULL;
+		return -ENODEV;
 
 	/*
 	 * Enter a read-side critical section.
@@ -170,43 +160,36 @@ struct revocable *revocable_alloc(struct revocable_provider __rcu *_rp)
 		rp = rcu_dereference(_rp);
 		if (!rp)
 			/* The revocable provider has been revoked. */
-			return NULL;
+			return -ENODEV;
 
 		if (!kref_get_unless_zero(&rp->kref))
 			/*
 			 * The revocable provider is releasing (i.e.,
 			 * revocable_provider_release() has been called).
 			 */
-			return NULL;
+			return -ENODEV;
 	}
 	/* At this point, `rp` is safe to access as holding a kref of it */
 
-	rev = kzalloc(sizeof(*rev), GFP_KERNEL);
-	if (!rev) {
-		kref_put(&rp->kref, revocable_provider_release);
-		return NULL;
-	}
-
 	rev->rp = rp;
-	return rev;
+	return 0;
 }
-EXPORT_SYMBOL_GPL(revocable_alloc);
+EXPORT_SYMBOL_GPL(revocable_init);
 
 /**
- * revocable_free() - Free struct revocable.
+ * revocable_deinit() - Deinitialize 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)
+void revocable_deinit(struct revocable *rev)
 {
 	struct revocable_provider *rp = rev->rp;
 
 	kref_put(&rp->kref, revocable_provider_release);
-	kfree(rev);
 }
-EXPORT_SYMBOL_GPL(revocable_free);
+EXPORT_SYMBOL_GPL(revocable_deinit);
 
 /**
  * revocable_try_access() - Try to access the resource.
diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
index 7fc4d6a3dff6..a2818ec01298 100644
--- a/drivers/base/revocable_test.c
+++ b/drivers/base/revocable_test.c
@@ -15,7 +15,7 @@
  * - Try Access Macro: Same as "Revocation" but uses the
  *   REVOCABLE_TRY_ACCESS_WITH() and REVOCABLE_TRY_ACCESS_SCOPED().
  *
- * - Provider Use-after-free: Verifies revocable_alloc() correctly handles
+ * - Provider Use-after-free: Verifies revocable_init() correctly handles
  *   race conditions where the provider is being released.
  */
 
@@ -26,20 +26,21 @@
 static void revocable_test_basic(struct kunit *test)
 {
 	struct revocable_provider __rcu *rp;
-	struct revocable *rev;
+	struct revocable rev;
 	void *real_res = (void *)0x12345678, *res;
+	int ret;
 
 	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);
+	ret = revocable_init(rp, &rev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
 
-	res = revocable_try_access(rev);
+	res = revocable_try_access(&rev);
 	KUNIT_EXPECT_PTR_EQ(test, res, real_res);
-	revocable_withdraw_access(rev);
+	revocable_withdraw_access(&rev);
 
-	revocable_free(rev);
+	revocable_deinit(&rev);
 	revocable_provider_revoke(&rp);
 	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 }
@@ -47,43 +48,40 @@ static void revocable_test_basic(struct kunit *test)
 static void revocable_test_revocation(struct kunit *test)
 {
 	struct revocable_provider __rcu *rp;
-	struct revocable *rev;
+	struct revocable rev;
 	void *real_res = (void *)0x12345678, *res;
+	int ret;
 
 	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);
+	ret = revocable_init(rp, &rev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
 
-	res = revocable_try_access(rev);
+	res = revocable_try_access(&rev);
 	KUNIT_EXPECT_PTR_EQ(test, res, real_res);
-	revocable_withdraw_access(rev);
+	revocable_withdraw_access(&rev);
 
 	revocable_provider_revoke(&rp);
 	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 
-	res = revocable_try_access(rev);
+	res = revocable_try_access(&rev);
 	KUNIT_EXPECT_PTR_EQ(test, res, NULL);
-	revocable_withdraw_access(rev);
+	revocable_withdraw_access(&rev);
 
-	revocable_free(rev);
+	revocable_deinit(&rev);
 }
 
 static void revocable_test_try_access_macro(struct kunit *test)
 {
 	struct revocable_provider __rcu *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);
-
 	{
-		REVOCABLE_TRY_ACCESS_WITH(rev, res);
+		REVOCABLE_TRY_ACCESS_WITH(rp, res);
 		KUNIT_EXPECT_PTR_EQ(test, res, real_res);
 	}
 
@@ -91,28 +89,22 @@ static void revocable_test_try_access_macro(struct kunit *test)
 	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 
 	{
-		REVOCABLE_TRY_ACCESS_WITH(rev, res);
+		REVOCABLE_TRY_ACCESS_WITH(rp, res);
 		KUNIT_EXPECT_PTR_EQ(test, res, NULL);
 	}
-
-	revocable_free(rev);
 }
 
 static void revocable_test_try_access_macro2(struct kunit *test)
 {
 	struct revocable_provider __rcu *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_SCOPED(rev, res) {
+	REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
 		KUNIT_EXPECT_PTR_EQ(test, res, real_res);
 		accessed = true;
 	}
@@ -122,32 +114,31 @@ static void revocable_test_try_access_macro2(struct kunit *test)
 	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
 
 	accessed = false;
-	REVOCABLE_TRY_ACCESS_SCOPED(rev, res) {
+	REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
 		KUNIT_EXPECT_PTR_EQ(test, res, NULL);
 		accessed = true;
 	}
 	KUNIT_EXPECT_TRUE(test, accessed);
-
-	revocable_free(rev);
 }
 
 static void revocable_test_provider_use_after_free(struct kunit *test)
 {
 	struct revocable_provider __rcu *rp;
 	struct revocable_provider *old_rp;
+	struct revocable rev;
 	void *real_res = (void *)0x12345678;
-	struct revocable *rev;
+	int ret;
 
 	rp = revocable_provider_alloc(real_res);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
 
-	rev = revocable_alloc(NULL);
-	KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
+	ret = revocable_init(NULL, &rev);
+	KUNIT_EXPECT_NE(test, ret, 0);
 
 	/* Simulate the provider has been freed. */
 	old_rp = rcu_replace_pointer(rp, NULL, 1);
-	rev = revocable_alloc(rp);
-	KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
+	ret = revocable_init(rp, &rev);
+	KUNIT_EXPECT_NE(test, ret, 0);
 	rcu_replace_pointer(rp, old_rp, 1);
 
 	struct {
@@ -159,12 +150,14 @@ static void revocable_test_provider_use_after_free(struct kunit *test)
 
 	/* Simulate the provider is releasing. */
 	refcount_set(&rp_internal->kref.refcount, 0);
-	rev = revocable_alloc(rp);
-	KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
+	ret = revocable_init(rp, &rev);
+	KUNIT_EXPECT_NE(test, ret, 0);
 	refcount_set(&rp_internal->kref.refcount, 1);
 
 	revocable_provider_revoke(&rp);
 	KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
+	ret = revocable_init(rp, &rev);
+	KUNIT_EXPECT_NE(test, ret, 0);
 }
 
 static struct kunit_case revocable_test_cases[] = {
diff --git a/include/linux/revocable.h b/include/linux/revocable.h
index d5da3584adbe..e3d6d2c953a3 100644
--- a/include/linux/revocable.h
+++ b/include/linux/revocable.h
@@ -10,27 +10,46 @@
 #include <linux/cleanup.h>
 
 struct device;
-struct revocable;
 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);
 
-struct revocable *revocable_alloc(struct revocable_provider __rcu *rp);
-void revocable_free(struct revocable *rev);
+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) revocable_withdraw_access(_T))
+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
- * @_rev: The consumer's ``struct revocable *`` handle.
+ * @_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
- * revocable_withdraw_access() is always called, even in the case of an early
- * exit.
+ * 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
@@ -41,13 +60,15 @@ DEFINE_FREE(access_rev, struct revocable *, if (_T) revocable_withdraw_access(_T
  * are allowed before the helper.  Otherwise, the compiler fails with
  * "jump bypasses initialization of variable with __attribute__((cleanup))".
  */
-#define REVOCABLE_TRY_ACCESS_WITH(_rev, _res)					\
-	struct revocable *__UNIQUE_ID(name) __free(access_rev) = _rev;		\
-	_res = revocable_try_access(_rev)
+#define REVOCABLE_TRY_ACCESS_WITH(_rp, _res)					\
+	_REVOCABLE_TRY_ACCESS_WITH(_rp, __UNIQUE_ID(name), _res)
 
-#define _REVOCABLE_TRY_ACCESS_SCOPED(_rev, _label, _res)			\
-	for (struct revocable *__UNIQUE_ID(name) __free(access_rev) = _rev;	\
-	     (_res = revocable_try_access(_rev)) || true; ({ goto _label; }))	\
+#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;							\
@@ -55,13 +76,14 @@ _label:										\
 
 /**
  * REVOCABLE_TRY_ACCESS_SCOPED() - A helper for accessing revocable resource
- * @_rev: The consumer's ``struct revocable *`` handle.
+ * @_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(_rev, _res)					\
-	_REVOCABLE_TRY_ACCESS_SCOPED(_rev, __UNIQUE_ID(label), _res)
+#define REVOCABLE_TRY_ACCESS_SCOPED(_rp, _res)					\
+	_REVOCABLE_TRY_ACCESS_SCOPED(_rp, __UNIQUE_ID(name),			\
+				     __UNIQUE_ID(label), _res)
 
 #endif /* __LINUX_REVOCABLE_H */
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
index ae6c67e65f3d..a560ceda7318 100644
--- a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
+++ b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
@@ -24,23 +24,7 @@ struct revocable_test_provider_priv {
 
 static int revocable_test_consumer_open(struct inode *inode, struct file *filp)
 {
-	struct revocable *rev;
-	struct revocable_provider __rcu *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);
+	filp->private_data = inode->i_private;
 	return 0;
 }
 
@@ -48,25 +32,33 @@ static ssize_t revocable_test_consumer_read(struct file *filp,
 					    char __user *buf,
 					    size_t count, loff_t *offset)
 {
+	int ret;
 	char *res;
 	char data[16];
 	size_t len;
-	struct revocable *rev = filp->private_data;
+	struct revocable rev;
+	struct revocable_provider __rcu *rp = filp->private_data;
 
 	switch (*offset) {
 	case 0:
-		res = revocable_try_access(rev);
+		ret = revocable_init(rp, &rev);
+		if (ret) {
+			snprintf(data, sizeof(data), "%s", "(null)");
+			break;
+		}
+		res = revocable_try_access(&rev);
 		snprintf(data, sizeof(data), "%s", res ?: "(null)");
-		revocable_withdraw_access(rev);
+		revocable_withdraw_access(&rev);
+		revocable_deinit(&rev);
 		break;
 	case TEST_MAGIC_OFFSET:
 		{
-			REVOCABLE_TRY_ACCESS_WITH(rev, res);
+			REVOCABLE_TRY_ACCESS_WITH(rp, res);
 			snprintf(data, sizeof(data), "%s", res ?: "(null)");
 		}
 		break;
 	case TEST_MAGIC_OFFSET2:
-		REVOCABLE_TRY_ACCESS_SCOPED(rev, res)
+		REVOCABLE_TRY_ACCESS_SCOPED(rp, res)
 			snprintf(data, sizeof(data), "%s", res ?: "(null)");
 		break;
 	default:
@@ -83,7 +75,6 @@ static ssize_t revocable_test_consumer_read(struct file *filp,
 
 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,
 };
-- 
2.53.0.rc1.217.geba53bf80e-goog

[PATCH 4/4] revocable: Add KUnit test for concurrent access

[Tzung-Bi Shih]

Add a test case to verify correct synchronization between concurrent
readers and a revocation.

The test setup involves:
1. Consumer 1 enters the critical section (SRCU read lock) and verifies
   access to the resource.
2. Provider attempts to revoke the resource.  This should block until
   Consumer 1 releases the lock.
3. Consumer 2 attempts to enter the critical section while revocation
   is pending.  It should see the resource as revoked (NULL).
4. Consumer 1 exits, allowing the revocation to complete.

This ensures that the SRCU mechanism correctly enforces grace periods
and that new readers are properly prevented from accessing the resource
once revocation has begun.

A way to run the test:
$ ./tools/testing/kunit/kunit.py run \
	--kconfig_add CONFIG_REVOCABLE_KUNIT_TEST=y \
	--kconfig_add CONFIG_PROVE_LOCKING=y \
	--kconfig_add CONFIG_DEBUG_KERNEL=y \
	--kconfig_add CONFIG_DEBUG_INFO=y \
	--kconfig_add CONFIG_DEBUG_INFO_DWARF5=y \
	--kconfig_add CONFIG_KASAN=y \
	--kconfig_add CONFIG_DETECT_HUNG_TASK=y \
	--kconfig_add CONFIG_DEFAULT_HUNG_TASK_TIMEOUT="10" \
	--arch=x86_64 --raw_output=all \
	revocable_test

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/base/revocable_test.c | 104 ++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
index a2818ec01298..27f5d7d96f4b 100644
--- a/drivers/base/revocable_test.c
+++ b/drivers/base/revocable_test.c
@@ -17,9 +17,14 @@
  *
  * - Provider Use-after-free: Verifies revocable_init() correctly handles
  *   race conditions where the provider is being released.
+ *
+ * - Concurrent Access: Verifies multiple threads can access the resource.
  */
 
 #include <kunit/test.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/kthread.h>
 #include <linux/refcount.h>
 #include <linux/revocable.h>
 
@@ -160,12 +165,111 @@ static void revocable_test_provider_use_after_free(struct kunit *test)
 	KUNIT_EXPECT_NE(test, ret, 0);
 }
 
+struct test_concurrent_access_context {
+	struct kunit *test;
+	struct revocable_provider __rcu *rp;
+	struct revocable rev;
+	struct completion started, enter, exit;
+	struct task_struct *thread;
+	void *expected_res;
+};
+
+static int test_concurrent_access_provider(void *data)
+{
+	struct test_concurrent_access_context *ctx = data;
+
+	complete(&ctx->started);
+
+	wait_for_completion(&ctx->enter);
+	revocable_provider_revoke(&ctx->rp);
+	KUNIT_EXPECT_PTR_EQ(ctx->test, unrcu_pointer(ctx->rp), NULL);
+
+	return 0;
+}
+
+static int test_concurrent_access_consumer(void *data)
+{
+	struct test_concurrent_access_context *ctx = data;
+	void *res;
+
+	complete(&ctx->started);
+
+	wait_for_completion(&ctx->enter);
+	res = revocable_try_access(&ctx->rev);
+	KUNIT_EXPECT_PTR_EQ(ctx->test, res, ctx->expected_res);
+
+	wait_for_completion(&ctx->exit);
+	revocable_withdraw_access(&ctx->rev);
+
+	return 0;
+}
+
+static void revocable_test_concurrent_access(struct kunit *test)
+{
+	struct revocable_provider __rcu *rp;
+	void *real_res = (void *)0x12345678;
+	struct test_concurrent_access_context *ctx;
+	int ret, i;
+
+	rp = revocable_provider_alloc(real_res);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
+
+	ctx = kunit_kmalloc_array(test, 3, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+	for (i = 0; i < 3; ++i) {
+		ctx[i].test = test;
+		init_completion(&ctx[i].started);
+		init_completion(&ctx[i].enter);
+		init_completion(&ctx[i].exit);
+
+		if (i == 0) {
+			ctx[i].rp = rp;
+			ctx[i].thread = kthread_run(
+				test_concurrent_access_provider, ctx + i,
+				"revocable_provider_%d", i);
+		} else {
+			ret = revocable_init(rp, &ctx[i].rev);
+			KUNIT_ASSERT_EQ(test, ret, 0);
+
+			ctx[i].thread = kthread_run(
+				test_concurrent_access_consumer, ctx + i,
+				"revocable_consumer_%d", i);
+		}
+		KUNIT_ASSERT_FALSE(test, IS_ERR(ctx[i].thread));
+
+		wait_for_completion(&ctx[i].started);
+	}
+	ctx[1].expected_res = real_res;
+	ctx[2].expected_res = NULL;
+
+	/* consumer1 enters read-side critical section */
+	complete(&ctx[1].enter);
+	msleep(100);
+	/* provider0 revokes the resource */
+	complete(&ctx[0].enter);
+	msleep(100);
+	/* consumer2 enters read-side critical section */
+	complete(&ctx[2].enter);
+	msleep(100);
+
+	/* consumer{1,2} exit read-side critical section */
+	complete(&ctx[1].exit);
+	complete(&ctx[2].exit);
+
+	for (i = 0; i < 3; ++i)
+		kthread_stop(ctx[i].thread);
+	for (i = 1; i < 3; ++i)
+		revocable_deinit(&ctx[i].rev);
+}
+
 static struct kunit_case revocable_test_cases[] = {
 	KUNIT_CASE(revocable_test_basic),
 	KUNIT_CASE(revocable_test_revocation),
 	KUNIT_CASE(revocable_test_try_access_macro),
 	KUNIT_CASE(revocable_test_try_access_macro2),
 	KUNIT_CASE(revocable_test_provider_use_after_free),
+	KUNIT_CASE(revocable_test_concurrent_access),
 	{}
 };
 
-- 
2.53.0.rc1.217.geba53bf80e-goog