revocable: Fix reported race conditions

158 lines not on this page · raw message

index 873a44082b6c..1622aae92fd3 100644
@@ -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) {

index 659ba01c58db..d5da3584adbe 100644
@@ -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);

index 1b0692eb75f3..ae6c67e65f3d 100644
@@ -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;
 		}
 	}

index 1622aae92fd3..7fc4d6a3dff6 100644
@@ -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),
 	{}
 };
 

91 lines not on this page · raw message

173 lines not on this page · raw message

91 lines not on this page · raw message

index ae6c67e65f3d..a560ceda7318 100644
@@ -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,
 };

index a2818ec01298..27f5d7d96f4b 100644
@@ -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),
 	{}
 };