Message-ID: <20260129143733.45618-2-tzungbi@kernel.org>
Date: Thu, 29 Jan 2026 14:37:30 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>
Cc: Jonathan Corbet <corbet@....net>,
	Shuah Khan <shuah@...nel.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Jason Gunthorpe <jgg@...dia.com>,
	Johan Hovold <johan@...nel.org>,
	linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org,
	chrome-platform@...ts.linux.dev,
	tzungbi@...nel.org
Subject: [PATCH 1/4] revocable: Fix races in revocable_alloc() using RCU

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@...nel.org>
Closes: https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/
Signed-off-by: Tzung-Bi Shih <tzungbi@...nel.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

Powered by blists - more mailing lists