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@kernel.org>
Closes: https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
Link: https://patch.msgid.link/20260129143733.45618-2-tzungbi@kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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
 

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.
+ *
+ * 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.
  */
-void revocable_provider_revoke(struct revocable_provider *rp)
+void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
 {
+	struct revocable_provider *rp;
+
+	rp = rcu_replace_pointer(*rp_ptr, NULL, 1);
+	if (!rp)
+		return;
+
 	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.
+ * @_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;
 
-	rev = kzalloc(sizeof(*rev), GFP_KERNEL);
-	if (!rev)
+	if (!_rp)
 		return NULL;
 
-	rev->rp = rp;
-	kref_get(&rp->kref);
+	/*
+	 * 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) {
+		kref_put(&rp->kref, revocable_provider_release);
+		return NULL;
+	}
+
+	rev->rp = rp;
 	return rev;
 }
 EXPORT_SYMBOL_GPL(revocable_alloc);

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) {

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);

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;
 		}
 	}