revocable: fix SRCU index corruption by requiring caller-provided storage

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>
Link: https://patch.msgid.link/20260129143733.45618-4-tzungbi@kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

2 files changed, 43 insertions, 67 deletions

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
@@ -74,16 +74,6 @@ 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;
-};
-
-/**
  * 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[] = {