* [PATCH v2 1/3] Revert "selftests: revocable: Add kselftest cases"
2026-02-04 14:28 [PATCH v2 0/3] Revert "revocable: Revocable resource management" Johan Hovold
@ 2026-02-04 14:28 ` Johan Hovold
2026-02-04 14:28 ` [PATCH v2 2/3] Revert "revocable: Add Kunit test cases" Johan Hovold
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2026-02-04 14:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J . Wysocki, Danilo Krummrich, Tzung-Bi Shih,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel,
Johan Hovold
This reverts commit 9d4502fef00fa7a798d3c0806d4da4466a7ffc6f.
The new revocable functionality is fundamentally broken and at a minimum
needs to be redesigned.
Drop the revocable selftests to allow the implementation to be reverted.
Signed-off-by: Johan Hovold <johan@kernel.org>
---
MAINTAINERS | 1 -
.../selftests/drivers/base/revocable/Makefile | 7 -
.../drivers/base/revocable/revocable_test.c | 136 -------------
.../drivers/base/revocable/test-revocable.sh | 39 ----
.../base/revocable/test_modules/Makefile | 10 -
.../revocable/test_modules/revocable_test.c | 187 ------------------
6 files changed, 380 deletions(-)
delete mode 100644 tools/testing/selftests/drivers/base/revocable/Makefile
delete mode 100644 tools/testing/selftests/drivers/base/revocable/revocable_test.c
delete mode 100755 tools/testing/selftests/drivers/base/revocable/test-revocable.sh
delete mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/Makefile
delete mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
diff --git a/MAINTAINERS b/MAINTAINERS
index b277baee5eb6..7759e9103070 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22392,7 +22392,6 @@ S: Maintained
F: drivers/base/revocable.c
F: drivers/base/revocable_test.c
F: include/linux/revocable.h
-F: tools/testing/selftests/drivers/base/revocable/
RFKILL
M: Johannes Berg <johannes@sipsolutions.net>
diff --git a/tools/testing/selftests/drivers/base/revocable/Makefile b/tools/testing/selftests/drivers/base/revocable/Makefile
deleted file mode 100644
index afa5ca0fa452..000000000000
--- a/tools/testing/selftests/drivers/base/revocable/Makefile
+++ /dev/null
@@ -1,7 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0
-
-TEST_GEN_MODS_DIR := test_modules
-TEST_GEN_PROGS_EXTENDED := revocable_test
-TEST_PROGS := test-revocable.sh
-
-include ../../../lib.mk
diff --git a/tools/testing/selftests/drivers/base/revocable/revocable_test.c b/tools/testing/selftests/drivers/base/revocable/revocable_test.c
deleted file mode 100644
index f024164e9273..000000000000
--- a/tools/testing/selftests/drivers/base/revocable/revocable_test.c
+++ /dev/null
@@ -1,136 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright 2026 Google LLC
- *
- * A selftest for the revocable API.
- *
- * The test cases cover the following scenarios:
- *
- * - Basic: Verifies that a consumer can successfully access the resource
- * provided via the provider.
- *
- * - Revocation: Verifies that after the provider revokes the resource,
- * the consumer correctly receives a NULL pointer on a subsequent access.
- *
- * - Try Access Macro: Same as "Revocation" but uses the
- * REVOCABLE_TRY_ACCESS_WITH() and REVOCABLE_TRY_ACCESS_SCOPED().
- */
-
-#include <fcntl.h>
-#include <unistd.h>
-
-#include "../../../kselftest_harness.h"
-
-#define DEBUGFS_PATH "/sys/kernel/debug/revocable_test"
-#define TEST_CMD_RESOURCE_GONE "resource_gone"
-#define TEST_DATA "12345678"
-#define TEST_MAGIC_OFFSET 0x1234
-#define TEST_MAGIC_OFFSET2 0x5678
-
-FIXTURE(revocable_fixture) {
- int pfd;
- int cfd;
-};
-
-FIXTURE_SETUP(revocable_fixture) {
- int ret;
-
- self->pfd = open(DEBUGFS_PATH "/provider", O_WRONLY);
- ASSERT_NE(-1, self->pfd)
- TH_LOG("failed to open provider fd");
-
- ret = write(self->pfd, TEST_DATA, strlen(TEST_DATA));
- ASSERT_NE(-1, ret) {
- close(self->pfd);
- TH_LOG("failed to write test data");
- }
-
- self->cfd = open(DEBUGFS_PATH "/consumer", O_RDONLY);
- ASSERT_NE(-1, self->cfd)
- TH_LOG("failed to open consumer fd");
-}
-
-FIXTURE_TEARDOWN(revocable_fixture) {
- close(self->cfd);
- close(self->pfd);
-}
-
-/*
- * ASSERT_* is only available in TEST or TEST_F block. Use
- * macro for the helper.
- */
-#define READ_TEST_DATA(_fd, _offset, _data, _msg) \
- do { \
- int ret; \
- \
- ret = lseek(_fd, _offset, SEEK_SET); \
- ASSERT_NE(-1, ret) \
- TH_LOG("failed to lseek"); \
- \
- ret = read(_fd, _data, sizeof(_data) - 1); \
- ASSERT_NE(-1, ret) \
- TH_LOG(_msg); \
- data[ret] = '\0'; \
- } while (0)
-
-TEST_F(revocable_fixture, basic) {
- char data[16];
-
- READ_TEST_DATA(self->cfd, 0, data, "failed to read test data");
- EXPECT_STREQ(TEST_DATA, data);
-}
-
-TEST_F(revocable_fixture, revocation) {
- char data[16];
- int ret;
-
- READ_TEST_DATA(self->cfd, 0, data, "failed to read test data");
- EXPECT_STREQ(TEST_DATA, data);
-
- ret = write(self->pfd, TEST_CMD_RESOURCE_GONE,
- strlen(TEST_CMD_RESOURCE_GONE));
- ASSERT_NE(-1, ret)
- TH_LOG("failed to write resource gone cmd");
-
- READ_TEST_DATA(self->cfd, 0, data,
- "failed to read test data after resource gone");
- EXPECT_STREQ("(null)", data);
-}
-
-TEST_F(revocable_fixture, try_access_macro) {
- char data[16];
- int ret;
-
- READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET, data,
- "failed to read test data");
- EXPECT_STREQ(TEST_DATA, data);
-
- ret = write(self->pfd, TEST_CMD_RESOURCE_GONE,
- strlen(TEST_CMD_RESOURCE_GONE));
- ASSERT_NE(-1, ret)
- TH_LOG("failed to write resource gone cmd");
-
- READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET, data,
- "failed to read test data after resource gone");
- EXPECT_STREQ("(null)", data);
-}
-
-TEST_F(revocable_fixture, try_access_macro2) {
- char data[16];
- int ret;
-
- READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET2, data,
- "failed to read test data");
- EXPECT_STREQ(TEST_DATA, data);
-
- ret = write(self->pfd, TEST_CMD_RESOURCE_GONE,
- strlen(TEST_CMD_RESOURCE_GONE));
- ASSERT_NE(-1, ret)
- TH_LOG("failed to write resource gone cmd");
-
- READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET2, data,
- "failed to read test data after resource gone");
- EXPECT_STREQ("(null)", data);
-}
-
-TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/drivers/base/revocable/test-revocable.sh b/tools/testing/selftests/drivers/base/revocable/test-revocable.sh
deleted file mode 100755
index 3a34be28001a..000000000000
--- a/tools/testing/selftests/drivers/base/revocable/test-revocable.sh
+++ /dev/null
@@ -1,39 +0,0 @@
-#!/bin/bash
-# SPDX-License-Identifier: GPL-2.0
-
-mod_name="revocable_test"
-ksft_fail=1
-ksft_skip=4
-
-if [ "$(id -u)" -ne 0 ]; then
- echo "$0: Must be run as root"
- exit "$ksft_skip"
-fi
-
-if ! which insmod > /dev/null 2>&1; then
- echo "$0: Need insmod"
- exit "$ksft_skip"
-fi
-
-if ! which rmmod > /dev/null 2>&1; then
- echo "$0: Need rmmod"
- exit "$ksft_skip"
-fi
-
-insmod test_modules/"${mod_name}".ko
-
-if [ ! -d /sys/kernel/debug/revocable_test/ ]; then
- mount -t debugfs none /sys/kernel/debug/
-
- if [ ! -d /sys/kernel/debug/revocable_test/ ]; then
- echo "$0: Error mounting debugfs"
- exit "$ksft_fail"
- fi
-fi
-
-./revocable_test
-ret=$?
-
-rmmod "${mod_name}"
-
-exit "${ret}"
diff --git a/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile b/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile
deleted file mode 100644
index f29e4f909402..000000000000
--- a/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile
+++ /dev/null
@@ -1,10 +0,0 @@
-TESTMODS_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST)))))
-KDIR ?= /lib/modules/$(shell uname -r)/build
-
-obj-m += revocable_test.o
-
-all:
- $(Q)$(MAKE) -C $(KDIR) M=$(TESTMODS_DIR)
-
-clean:
- $(Q)$(MAKE) -C $(KDIR) M=$(TESTMODS_DIR) clean
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
deleted file mode 100644
index a560ceda7318..000000000000
--- a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
+++ /dev/null
@@ -1,187 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright 2026 Google LLC
- *
- * A kernel module for testing the revocable API.
- */
-
-#include <linux/debugfs.h>
-#include <linux/module.h>
-#include <linux/revocable.h>
-#include <linux/slab.h>
-
-#define TEST_CMD_RESOURCE_GONE "resource_gone"
-#define TEST_MAGIC_OFFSET 0x1234
-#define TEST_MAGIC_OFFSET2 0x5678
-
-static struct dentry *debugfs_dir;
-
-struct revocable_test_provider_priv {
- struct revocable_provider __rcu *rp;
- struct dentry *dentry;
- char res[16];
-};
-
-static int revocable_test_consumer_open(struct inode *inode, struct file *filp)
-{
- filp->private_data = inode->i_private;
- return 0;
-}
-
-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;
- struct revocable_provider __rcu *rp = filp->private_data;
-
- switch (*offset) {
- case 0:
- 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_deinit(&rev);
- break;
- case TEST_MAGIC_OFFSET:
- {
- REVOCABLE_TRY_ACCESS_WITH(rp, res);
- snprintf(data, sizeof(data), "%s", res ?: "(null)");
- }
- break;
- case TEST_MAGIC_OFFSET2:
- REVOCABLE_TRY_ACCESS_SCOPED(rp, res)
- snprintf(data, sizeof(data), "%s", res ?: "(null)");
- break;
- default:
- return 0;
- }
-
- len = min_t(size_t, strlen(data), count);
- if (copy_to_user(buf, data, len))
- return -EFAULT;
-
- *offset = len;
- return len;
-}
-
-static const struct file_operations revocable_test_consumer_fops = {
- .open = revocable_test_consumer_open,
- .read = revocable_test_consumer_read,
- .llseek = default_llseek,
-};
-
-static int revocable_test_provider_open(struct inode *inode, struct file *filp)
-{
- struct revocable_test_provider_priv *priv;
-
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
- filp->private_data = priv;
-
- return 0;
-}
-
-static int revocable_test_provider_release(struct inode *inode,
- struct file *filp)
-{
- struct revocable_test_provider_priv *priv = filp->private_data;
-
- debugfs_remove(priv->dentry);
- if (unrcu_pointer(priv->rp))
- revocable_provider_revoke(&priv->rp);
- kfree(priv);
-
- return 0;
-}
-
-static ssize_t revocable_test_provider_write(struct file *filp,
- const char __user *buf,
- size_t count, loff_t *offset)
-{
- size_t copied;
- char data[64];
- struct revocable_test_provider_priv *priv = filp->private_data;
-
- copied = strncpy_from_user(data, buf, sizeof(data));
- if (copied < 0)
- return copied;
- if (copied == sizeof(data))
- data[sizeof(data) - 1] = '\0';
-
- /*
- * Note: The test can't just close the FD for signaling the
- * resource gone. Subsequent file operations on the opening
- * FD of debugfs return -EIO after calling debugfs_remove().
- * See also debugfs_file_get().
- *
- * Here is a side command channel for signaling the resource
- * gone.
- */
- if (!strcmp(data, TEST_CMD_RESOURCE_GONE)) {
- revocable_provider_revoke(&priv->rp);
- rcu_assign_pointer(priv->rp, NULL);
- } else {
- if (priv->res[0] != '\0')
- return 0;
-
- strscpy(priv->res, data);
-
- priv->rp = revocable_provider_alloc(&priv->res);
- if (!unrcu_pointer(priv->rp))
- return -ENOMEM;
-
- priv->dentry = debugfs_create_file("consumer", 0400,
- debugfs_dir,
- unrcu_pointer(priv->rp),
- &revocable_test_consumer_fops);
- if (!priv->dentry) {
- revocable_provider_revoke(&priv->rp);
- return -ENOMEM;
- }
- }
-
- return copied;
-}
-
-static const struct file_operations revocable_test_provider_fops = {
- .open = revocable_test_provider_open,
- .release = revocable_test_provider_release,
- .write = revocable_test_provider_write,
-};
-
-static int __init revocable_test_init(void)
-{
- debugfs_dir = debugfs_create_dir("revocable_test", NULL);
- if (!debugfs_dir)
- return -ENOMEM;
-
- if (!debugfs_create_file("provider", 0200, debugfs_dir, NULL,
- &revocable_test_provider_fops)) {
- debugfs_remove_recursive(debugfs_dir);
- return -ENOMEM;
- }
-
- return 0;
-}
-
-static void __exit revocable_test_exit(void)
-{
- debugfs_remove_recursive(debugfs_dir);
-}
-
-module_init(revocable_test_init);
-module_exit(revocable_test_exit);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Tzung-Bi Shih <tzungbi@kernel.org>");
-MODULE_DESCRIPTION("Revocable Kselftest");
--
2.52.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 2/3] Revert "revocable: Add Kunit test cases"
2026-02-04 14:28 [PATCH v2 0/3] Revert "revocable: Revocable resource management" Johan Hovold
2026-02-04 14:28 ` [PATCH v2 1/3] Revert "selftests: revocable: Add kselftest cases" Johan Hovold
@ 2026-02-04 14:28 ` Johan Hovold
2026-02-04 14:28 ` [PATCH v2 3/3] Revert "revocable: Revocable resource management" Johan Hovold
2026-02-06 15:13 ` [PATCH v2 0/3] " Greg Kroah-Hartman
3 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2026-02-04 14:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J . Wysocki, Danilo Krummrich, Tzung-Bi Shih,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel,
Johan Hovold
This reverts commit cd7693419bb5abd91ad2f407dab69c480e417a61.
The new revocable functionality is fundamentally broken and at a minimum
needs to be redesigned.
Drop the revocable Kunit tests to allow the implementation to be reverted.
Signed-off-by: Johan Hovold <johan@kernel.org>
---
MAINTAINERS | 1 -
drivers/base/Kconfig | 8 -
drivers/base/Makefile | 3 -
drivers/base/revocable_test.c | 284 ----------------------------------
4 files changed, 296 deletions(-)
delete mode 100644 drivers/base/revocable_test.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7759e9103070..93c07c645c68 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22390,7 +22390,6 @@ M: Tzung-Bi Shih <tzungbi@kernel.org>
L: linux-kernel@vger.kernel.org
S: Maintained
F: drivers/base/revocable.c
-F: drivers/base/revocable_test.c
F: include/linux/revocable.h
RFKILL
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 9f318b98144d..1786d87b29e2 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -250,11 +250,3 @@ config FW_DEVLINK_SYNC_STATE_TIMEOUT
work on.
endmenu
-
-# Kunit test cases
-config REVOCABLE_KUNIT_TEST
- tristate "Kunit tests for revocable" if !KUNIT_ALL_TESTS
- depends on KUNIT && BROKEN
- default KUNIT_ALL_TESTS
- help
- Kunit tests for the revocable API.
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c6607616a73..8074a10183dc 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -35,6 +35,3 @@ ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
# define_trace.h needs to know how to find our header
CFLAGS_trace.o := -I$(src)
obj-$(CONFIG_TRACING) += trace.o
-
-# Kunit test cases
-obj-$(CONFIG_REVOCABLE_KUNIT_TEST) += revocable_test.o
diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
deleted file mode 100644
index 27f5d7d96f4b..000000000000
--- a/drivers/base/revocable_test.c
+++ /dev/null
@@ -1,284 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright 2026 Google LLC
- *
- * Kunit tests for the revocable API.
- *
- * The test cases cover the following scenarios:
- *
- * - Basic: Verifies that a consumer can successfully access the resource
- * provided via the provider.
- *
- * - Revocation: Verifies that after the provider revokes the resource,
- * the consumer correctly receives a NULL pointer on a subsequent access.
- *
- * - Try Access Macro: Same as "Revocation" but uses the
- * REVOCABLE_TRY_ACCESS_WITH() and REVOCABLE_TRY_ACCESS_SCOPED().
- *
- * - 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>
-
-static void revocable_test_basic(struct kunit *test)
-{
- struct revocable_provider __rcu *rp;
- 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);
-
- ret = revocable_init(rp, &rev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- res = revocable_try_access(&rev);
- KUNIT_EXPECT_PTR_EQ(test, res, real_res);
- revocable_withdraw_access(&rev);
-
- revocable_deinit(&rev);
- revocable_provider_revoke(&rp);
- KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
-}
-
-static void revocable_test_revocation(struct kunit *test)
-{
- struct revocable_provider __rcu *rp;
- 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);
-
- ret = revocable_init(rp, &rev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- res = revocable_try_access(&rev);
- KUNIT_EXPECT_PTR_EQ(test, res, real_res);
- revocable_withdraw_access(&rev);
-
- 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);
- revocable_withdraw_access(&rev);
-
- revocable_deinit(&rev);
-}
-
-static void revocable_test_try_access_macro(struct kunit *test)
-{
- struct revocable_provider __rcu *rp;
- void *real_res = (void *)0x12345678, *res;
-
- rp = revocable_provider_alloc(real_res);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
-
- {
- REVOCABLE_TRY_ACCESS_WITH(rp, res);
- KUNIT_EXPECT_PTR_EQ(test, res, real_res);
- }
-
- revocable_provider_revoke(&rp);
- KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
-
- {
- REVOCABLE_TRY_ACCESS_WITH(rp, res);
- KUNIT_EXPECT_PTR_EQ(test, res, NULL);
- }
-}
-
-static void revocable_test_try_access_macro2(struct kunit *test)
-{
- struct revocable_provider __rcu *rp;
- void *real_res = (void *)0x12345678, *res;
- bool accessed;
-
- rp = revocable_provider_alloc(real_res);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
-
- accessed = false;
- REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
- KUNIT_EXPECT_PTR_EQ(test, res, real_res);
- accessed = true;
- }
- KUNIT_EXPECT_TRUE(test, accessed);
-
- revocable_provider_revoke(&rp);
- KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
-
- accessed = false;
- REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
- KUNIT_EXPECT_PTR_EQ(test, res, NULL);
- accessed = true;
- }
- KUNIT_EXPECT_TRUE(test, accessed);
-}
-
-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;
- int ret;
-
- rp = revocable_provider_alloc(real_res);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
-
- 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);
- ret = revocable_init(rp, &rev);
- KUNIT_EXPECT_NE(test, ret, 0);
- 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);
- 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);
-}
-
-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),
- {}
-};
-
-static struct kunit_suite revocable_test_suite = {
- .name = "revocable_test",
- .test_cases = revocable_test_cases,
-};
-
-kunit_test_suite(revocable_test_suite);
-
-MODULE_DESCRIPTION("KUnit tests for the revocable API");
-MODULE_LICENSE("GPL");
--
2.52.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 3/3] Revert "revocable: Revocable resource management"
2026-02-04 14:28 [PATCH v2 0/3] Revert "revocable: Revocable resource management" Johan Hovold
2026-02-04 14:28 ` [PATCH v2 1/3] Revert "selftests: revocable: Add kselftest cases" Johan Hovold
2026-02-04 14:28 ` [PATCH v2 2/3] Revert "revocable: Add Kunit test cases" Johan Hovold
@ 2026-02-04 14:28 ` Johan Hovold
2026-02-05 8:51 ` Tzung-Bi Shih
2026-02-06 15:13 ` [PATCH v2 0/3] " Greg Kroah-Hartman
3 siblings, 1 reply; 13+ messages in thread
From: Johan Hovold @ 2026-02-04 14:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J . Wysocki, Danilo Krummrich, Tzung-Bi Shih,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel,
Johan Hovold
This reverts commit 62eb557580eb2177cf16c3fd2b6efadff297b29a.
The revocable implementation uses two separate abstractions, struct
revocable_provider and struct revocable, in order to store the SRCU read
lock index which must be passed unaltered to srcu_read_unlock() in the
same context when a resource is no longer needed.
With the merged revocable API, multiple threads could however share the
same struct revocable and therefore potentially overwrite the SRCU index
of another thread which can cause the SRCU synchronisation in
revocable_provider_revoke() to never complete. [1]
An example revocable conversion of the gpiolib code also turned out to
be fundamentally flawed and could lead to use-after-free. [2]
An attempt to address both issues was quickly put together and merged,
but revocable is still fundamentally broken. [3]
Specifically, the latest design relies on RCU for storing a pointer to
the revocable provider, but since the resource can be shared by value
(e.g. as in the now reverted selftests) this does not work at all and
can also lead to use-after-free:
static void revocable_provider_release(struct kref *kref)
{
struct revocable_provider *rp = container_of(kref,
struct revocable_provider, kref);
cleanup_srcu_struct(&rp->srcu);
kfree_rcu(rp, rcu);
}
void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
{
struct revocable_provider *rp;
rp = rcu_replace_pointer(*rp_ptr, NULL, 1);
...
kref_put(&rp->kref, revocable_provider_release);
}
int revocable_init(struct revocable_provider __rcu *_rp,
struct revocable *rev)
{
struct revocable_provider *rp;
...
scoped_guard(rcu) {
rp = rcu_dereference(_rp);
if (!rp)
return -ENODEV;
if (!kref_get_unless_zero(&rp->kref))
return -ENODEV;
}
...
}
producer:
priv->rp = revocable_provider_alloc(&priv->res);
// pass priv->rp by value to consumer
revocable_provider_revoke(&priv->rp);
consumer:
struct revocable_provider __rcu *rp = filp->private_data;
struct revocable *rev;
revocable_init(rp, &rev);
as _rp would still be non-NULL in revocable_init() regardless of whether
the producer has revoked the resource and set its pointer to NULL.
Essentially revocable still relies on having a pointer to reference
counted driver data which holds the revocable provider, which makes all
the RCU protection unnecessary along with most of the current revocable
design and implementation.
As the above shows, and as has been pointed out repeatedly elsewhere,
these kind of issues are not something that should be addressed
incrementally. [4]
Revert the revocable implementation until a redesign has been proposed
and evaluated properly.
Link: https://lore.kernel.org/all/20260124170535.11756-4-johan@kernel.org/ [1]
Link: https://lore.kernel.org/all/aXT45B6vLf9R3Pbf@hovoldconsulting.com/ [2]
Link: https://lore.kernel.org/all/20260129143733.45618-1-tzungbi@kernel.org/ [3]
Link: https://lore.kernel.org/all/aXobzoeooJqxMkEj@hovoldconsulting.com/ [4]
Signed-off-by: Johan Hovold <johan@kernel.org>
---
.../driver-api/driver-model/index.rst | 1 -
.../driver-api/driver-model/revocable.rst | 149 ------------
MAINTAINERS | 7 -
drivers/base/revocable.c | 225 ------------------
include/linux/revocable.h | 89 -------
5 files changed, 471 deletions(-)
delete mode 100644 Documentation/driver-api/driver-model/revocable.rst
delete mode 100644 drivers/base/revocable.c
delete mode 100644 include/linux/revocable.h
diff --git a/Documentation/driver-api/driver-model/index.rst b/Documentation/driver-api/driver-model/index.rst
index 8e1ee21185df..4831bdd92e5c 100644
--- a/Documentation/driver-api/driver-model/index.rst
+++ b/Documentation/driver-api/driver-model/index.rst
@@ -14,7 +14,6 @@ Driver Model
overview
platform
porting
- revocable
.. only:: subproject and html
diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst
deleted file mode 100644
index 350e7faeccdc..000000000000
--- a/Documentation/driver-api/driver-model/revocable.rst
+++ /dev/null
@@ -1,149 +0,0 @@
-.. SPDX-License-Identifier: GPL-2.0
-
-==============================
-Revocable Resource Management
-==============================
-
-Overview
-========
-
-.. kernel-doc:: drivers/base/revocable.c
- :doc: Overview
-
-Revocable vs. Devres (devm)
-===========================
-
-It's important to understand the distinct roles of the Revocable and Devres,
-and how they can complement each other. They address different problems in
-resource management:
-
-* **Devres:** Primarily address **resource leaks**. The lifetime of the
- resources is tied to the lifetime of the device. The resource is
- automatically freed when the device is unbound. This cleanup happens
- irrespective of any potential active users.
-
-* **Revocable:** Primarily addresses **invalid memory access**,
- such as Use-After-Free (UAF). It's an independent synchronization
- primitive that decouples consumer access from the resource's actual
- presence. Consumers interact with a "revocable object" (an intermediary),
- not the underlying resource directly. This revocable object persists as
- long as there are active references to it from consumer handles.
-
-**Key Distinctions & How They Complement Each Other:**
-
-1. **Reference Target:** Consumers of a resource managed by the Revocable
- mechanism hold a reference to the *revocable object*, not the
- encapsulated resource itself.
-
-2. **Resource Lifetime vs. Access:** The underlying resource's lifetime is
- independent of the number of references to the revocable object. The
- resource can be freed at any point. A common scenario is the resource
- being freed by `devres` when the providing device is unbound.
-
-3. **Safe Access:** Revocable provides a safe way to attempt access. Before
- using the resource, a consumer uses the Revocable API (e.g.,
- revocable_try_access()). This function checks if the resource is still
- valid. It returns a pointer to the resource only if it hasn't been
- revoked; otherwise, it returns NULL. This prevents UAF by providing a
- clear signal that the resource is gone.
-
-4. **Complementary Usage:** `devres` and Revocable work well together.
- `devres` can handle the automatic allocation and deallocation of a
- resource tied to a device. The Revocable mechanism can be layered on top
- to provide safe access for consumers whose lifetimes might extend beyond
- the provider device's lifetime. For instance, a userspace program might
- keep a character device file open even after the physical device has been
- removed. In this case:
-
- * `devres` frees the device-specific resource upon unbinding.
- * The Revocable mechanism ensures that any subsequent operations on the
- open file handle, which attempt to access the now-freed resource,
- will fail gracefully (e.g., revocable_try_access() returns NULL)
- instead of causing a UAF.
-
-In summary, `devres` ensures resources are *released* to prevent leaks, while
-the Revocable mechanism ensures that attempts to *access* these resources are
-done safely, even if the resource has been released.
-
-API and Usage
-=============
-
-For Resource Providers
-----------------------
-.. kernel-doc:: drivers/base/revocable.c
- :identifiers: revocable_provider
-
-.. kernel-doc:: drivers/base/revocable.c
- :identifiers: revocable_provider_alloc
-
-.. kernel-doc:: drivers/base/revocable.c
- :identifiers: revocable_provider_revoke
-
-For Resource Consumers
-----------------------
-.. kernel-doc:: include/linux/revocable.h
- :identifiers: revocable
-
-.. kernel-doc:: drivers/base/revocable.c
- :identifiers: revocable_init
-
-.. kernel-doc:: drivers/base/revocable.c
- :identifiers: revocable_deinit
-
-.. kernel-doc:: drivers/base/revocable.c
- :identifiers: revocable_try_access
-
-.. kernel-doc:: drivers/base/revocable.c
- :identifiers: revocable_withdraw_access
-
-.. kernel-doc:: include/linux/revocable.h
- :identifiers: REVOCABLE_TRY_ACCESS_WITH
-
-Example Usage
-~~~~~~~~~~~~~
-
-.. code-block:: c
-
- void consumer_use_resource(struct revocable_provider *rp)
- {
- struct foo_resource *res;
-
- REVOCABLE_TRY_ACCESS_WITH(rp, res);
- // Always check if the resource is valid.
- if (!res) {
- pr_warn("Resource is not available\n");
- return;
- }
-
- // At this point, 'res' is guaranteed to be valid until
- // this block exits.
- do_something_with(res);
-
- } // revocable_withdraw_access() is automatically called here.
-
-.. kernel-doc:: include/linux/revocable.h
- :identifiers: REVOCABLE_TRY_ACCESS_SCOPED
-
-Example Usage
-~~~~~~~~~~~~~
-
-.. code-block:: c
-
- void consumer_use_resource(struct revocable_provider *rp)
- {
- struct foo_resource *res;
-
- REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
- // Always check if the resource is valid.
- if (!res) {
- pr_warn("Resource is not available\n");
- return;
- }
-
- // At this point, 'res' is guaranteed to be valid until
- // this block exits.
- do_something_with(res);
- }
-
- // revocable_withdraw_access() is automatically called here.
- }
diff --git a/MAINTAINERS b/MAINTAINERS
index 93c07c645c68..1c5ceccc9928 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22385,13 +22385,6 @@ F: include/uapi/linux/rseq.h
F: kernel/rseq.c
F: tools/testing/selftests/rseq/
-REVOCABLE RESOURCE MANAGEMENT
-M: Tzung-Bi Shih <tzungbi@kernel.org>
-L: linux-kernel@vger.kernel.org
-S: Maintained
-F: drivers/base/revocable.c
-F: include/linux/revocable.h
-
RFKILL
M: Johannes Berg <johannes@sipsolutions.net>
L: linux-wireless@vger.kernel.org
diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
deleted file mode 100644
index 8532ca6a371c..000000000000
--- a/drivers/base/revocable.c
+++ /dev/null
@@ -1,225 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright 2026 Google LLC
- *
- * Revocable resource management
- */
-
-#include <linux/device.h>
-#include <linux/kref.h>
-#include <linux/revocable.h>
-#include <linux/slab.h>
-#include <linux/srcu.h>
-
-/**
- * DOC: Overview
- *
- * The "revocable" mechanism is a synchronization primitive designed to manage
- * safe access to resources that can be asynchronously removed or invalidated.
- * Its primary purpose is to prevent Use-After-Free (UAF) errors when
- * interacting with resources whose lifetimes are not guaranteed to outlast
- * their consumers.
- *
- * This is particularly useful in systems where resources can disappear
- * unexpectedly, such as those provided by hot-pluggable devices like USB.
- * When a consumer holds a reference to such a resource, the underlying device
- * might be removed, causing the resource's memory to be freed. Subsequent
- * access attempts by the consumer would then lead to UAF errors.
- *
- * Revocable addresses this by providing a form of "weak reference" and a
- * controlled access method. It allows a resource consumer to safely attempt to
- * access the resource. The mechanism guarantees that any access granted is
- * valid for the duration of its use. If the resource has already been
- * revoked (i.e., freed), the access attempt will fail safely, typically by
- * returning NULL, instead of causing a crash.
- *
- * The implementation uses a provider/consumer model built on Sleepable
- * RCU (SRCU) to guarantee safe memory access:
- *
- * - A resource provider, such as a driver for a hot-pluggable device,
- * allocates a struct revocable_provider and initializes it with a pointer
- * to the resource.
- *
- * - A resource consumer that wants to access the resource allocates a
- * struct revocable which acts as a handle containing a reference to the
- * provider.
- *
- * - To access the resource, the consumer uses revocable_try_access().
- * This function enters an SRCU read-side critical section and returns
- * the pointer to the resource. If the provider has already freed the
- * resource, it returns NULL. After use, the consumer calls
- * revocable_withdraw_access() to exit the SRCU critical section. The
- * REVOCABLE_TRY_ACCESS_WITH() and REVOCABLE_TRY_ACCESS_SCOPED() are
- * convenient helpers for doing that.
- *
- * - When the provider needs to remove the resource, it calls
- * revocable_provider_revoke(). This function sets the internal resource
- * pointer to NULL and then calls synchronize_srcu() to wait for all
- * current readers to finish before the resource can be completely torn
- * down.
- */
-
-/**
- * struct revocable_provider - A handle for resource provider.
- * @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;
-};
-
-/**
- * revocable_provider_alloc() - Allocate struct revocable_provider.
- * @res: The pointer of resource.
- *
- * 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 __rcu *revocable_provider_alloc(void *res)
-{
- struct revocable_provider *rp;
-
- rp = kzalloc(sizeof(*rp), GFP_KERNEL);
- if (!rp)
- return NULL;
-
- init_srcu_struct(&rp->srcu);
- RCU_INIT_POINTER(rp->res, res);
- kref_init(&rp->kref);
-
- return (struct revocable_provider __rcu *)rp;
-}
-EXPORT_SYMBOL_GPL(revocable_provider_alloc);
-
-static void revocable_provider_release(struct kref *kref)
-{
- struct revocable_provider *rp = container_of(kref,
- struct revocable_provider, kref);
-
- cleanup_srcu_struct(&rp->srcu);
- kfree_rcu(rp, rcu);
-}
-
-/**
- * revocable_provider_revoke() - Revoke the managed resource.
- * @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 __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);
-
-/**
- * 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: 0 on success, -errno otherwise.
- */
-int revocable_init(struct revocable_provider __rcu *_rp, struct revocable *rev)
-{
- struct revocable_provider *rp;
-
- if (!_rp)
- return -ENODEV;
-
- /*
- * 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 -ENODEV;
-
- if (!kref_get_unless_zero(&rp->kref))
- /*
- * The revocable provider is releasing (i.e.,
- * revocable_provider_release() has been called).
- */
- return -ENODEV;
- }
- /* At this point, `rp` is safe to access as holding a kref of it */
-
- rev->rp = rp;
- return 0;
-}
-EXPORT_SYMBOL_GPL(revocable_init);
-
-/**
- * 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_deinit(struct revocable *rev)
-{
- struct revocable_provider *rp = rev->rp;
-
- kref_put(&rp->kref, revocable_provider_release);
-}
-EXPORT_SYMBOL_GPL(revocable_deinit);
-
-/**
- * revocable_try_access() - Try to access the resource.
- * @rev: The pointer of struct revocable.
- *
- * This tries to de-reference to the resource and enters a RCU critical
- * section.
- *
- * Return: The pointer to the resource. NULL if the resource has gone.
- */
-void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu)
-{
- struct revocable_provider *rp = rev->rp;
-
- rev->idx = srcu_read_lock(&rp->srcu);
- return srcu_dereference(rp->res, &rp->srcu);
-}
-EXPORT_SYMBOL_GPL(revocable_try_access);
-
-/**
- * revocable_withdraw_access() - Stop accessing to the resource.
- * @rev: The pointer of struct revocable.
- *
- * Call this function to indicate the resource is no longer used. It exits
- * the RCU critical section.
- */
-void revocable_withdraw_access(struct revocable *rev) __releases(&rev->rp->srcu)
-{
- struct revocable_provider *rp = rev->rp;
-
- srcu_read_unlock(&rp->srcu, rev->idx);
-}
-EXPORT_SYMBOL_GPL(revocable_withdraw_access);
diff --git a/include/linux/revocable.h b/include/linux/revocable.h
deleted file mode 100644
index e3d6d2c953a3..000000000000
--- a/include/linux/revocable.h
+++ /dev/null
@@ -1,89 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Copyright 2026 Google LLC
- */
-
-#ifndef __LINUX_REVOCABLE_H
-#define __LINUX_REVOCABLE_H
-
-#include <linux/compiler.h>
-#include <linux/cleanup.h>
-
-struct device;
-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);
-
-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)->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
- * @_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
- * 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
- * @_res is ``NULL`` before using it. The revocable_withdraw_access() function
- * is automatically called when the scope is exited.
- *
- * Note: It shares the same issue with guard() in cleanup.h. No goto statements
- * are allowed before the helper. Otherwise, the compiler fails with
- * "jump bypasses initialization of variable with __attribute__((cleanup))".
- */
-#define REVOCABLE_TRY_ACCESS_WITH(_rp, _res) \
- _REVOCABLE_TRY_ACCESS_WITH(_rp, __UNIQUE_ID(name), _res)
-
-#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; \
- } else
-
-/**
- * REVOCABLE_TRY_ACCESS_SCOPED() - A helper for accessing revocable resource
- * @_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(_rp, _res) \
- _REVOCABLE_TRY_ACCESS_SCOPED(_rp, __UNIQUE_ID(name), \
- __UNIQUE_ID(label), _res)
-
-#endif /* __LINUX_REVOCABLE_H */
--
2.52.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] Revert "revocable: Revocable resource management"
2026-02-04 14:28 ` [PATCH v2 3/3] Revert "revocable: Revocable resource management" Johan Hovold
@ 2026-02-05 8:51 ` Tzung-Bi Shih
2026-02-05 11:56 ` Danilo Krummrich
2026-02-05 14:03 ` Johan Hovold
0 siblings, 2 replies; 13+ messages in thread
From: Tzung-Bi Shih @ 2026-02-05 8:51 UTC (permalink / raw)
To: Johan Hovold
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Wed, Feb 04, 2026 at 03:28:49PM +0100, Johan Hovold wrote:
> Specifically, the latest design relies on RCU for storing a pointer to
> the revocable provider, but since the resource can be shared by value
> (e.g. as in the now reverted selftests) this does not work at all and
> can also lead to use-after-free:
[...]
> producer:
>
> priv->rp = revocable_provider_alloc(&priv->res);
> // pass priv->rp by value to consumer
> revocable_provider_revoke(&priv->rp);
>
> consumer:
>
> struct revocable_provider __rcu *rp = filp->private_data;
> struct revocable *rev;
>
> revocable_init(rp, &rev);
>
> as _rp would still be non-NULL in revocable_init() regardless of whether
> the producer has revoked the resource and set its pointer to NULL.
You're right to point out the issue with copying the pointer of revocable
provider. If a consumer stores this pointer directly, rcu_replace_pointer()
in the producer's revocable_provider_revoke() will not affect the consumer's
copy. I understand this concern.
The intention was never for consumers to cache the pointer of revocable
provider long-term. The design relies on consumers obtaining the current
valid provider pointer at the point of access.
In the latest GPIO transition series [5], the usage pattern has been refined
to avoid locally storing the pointer of revocable provider. Instead, it's
fetched from a source of truth when needed.
I agree that the risks and correct usage patterns need to be much clearer.
I'll update the Documentation and the selftests to explicitly highlight
this limitation and demonstrate the proper way to interact with the API,
avoiding the storage of the provider pointer by value in consumer contexts.
> Essentially revocable still relies on having a pointer to reference
> counted driver data which holds the revocable provider, which makes all
> the RCU protection unnecessary along with most of the current revocable
> design and implementation.
(I'm assuming you are referring to the example in [6].)
I'm not sure I follow your reasoning. Per my understanding:
- The reference counted driver data (e.g. `gdev` in the GPIO example) is to
ensure the pointer of revocable provider isn't freed.
- The RCU protects the pointer value from concurrent access and updates
during the revocation process [7].
These seem to address different aspects. Could you provide more context
on why you see the RCU protection as redundant?
[5] https://lore.kernel.org/all/20260203061059.975605-9-tzungbi@kernel.org
[6] https://lore.kernel.org/all/20260203061059.975605-8-tzungbi@kernel.org
[7] https://lore.kernel.org/all/20260129143733.45618-2-tzungbi@kernel.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] Revert "revocable: Revocable resource management"
2026-02-05 8:51 ` Tzung-Bi Shih
@ 2026-02-05 11:56 ` Danilo Krummrich
2026-02-06 9:14 ` Tzung-Bi Shih
2026-02-05 14:03 ` Johan Hovold
1 sibling, 1 reply; 13+ messages in thread
From: Danilo Krummrich @ 2026-02-05 11:56 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Johan Hovold, Greg Kroah-Hartman, Rafael J . Wysocki,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Thu Feb 5, 2026 at 9:51 AM CET, Tzung-Bi Shih wrote:
> On Wed, Feb 04, 2026 at 03:28:49PM +0100, Johan Hovold wrote:
>> Specifically, the latest design relies on RCU for storing a pointer to
>> the revocable provider, but since the resource can be shared by value
>> (e.g. as in the now reverted selftests) this does not work at all and
>> can also lead to use-after-free:
> [...]
>> producer:
>>
>> priv->rp = revocable_provider_alloc(&priv->res);
>> // pass priv->rp by value to consumer
>> revocable_provider_revoke(&priv->rp);
>>
>> consumer:
>>
>> struct revocable_provider __rcu *rp = filp->private_data;
>> struct revocable *rev;
>>
>> revocable_init(rp, &rev);
>>
>> as _rp would still be non-NULL in revocable_init() regardless of whether
>> the producer has revoked the resource and set its pointer to NULL.
>
> You're right to point out the issue with copying the pointer of revocable
> provider. If a consumer stores this pointer directly, rcu_replace_pointer()
> in the producer's revocable_provider_revoke() will not affect the consumer's
> copy. I understand this concern.
>
> The intention was never for consumers to cache the pointer of revocable
> provider long-term. The design relies on consumers obtaining the current
> valid provider pointer at the point of access.
Yeah, I think this part is not a bug in the API, but I think revocable_init()
should be
int revocable_init(struct revocable_provider __rcu **_rp, ...)
instead of
int revocable_init(struct revocable_provider __rcu *_rp, ...)
for the same reason revocable_provider_revoke() takes a double pointer.
Otherwise this seems racy:
int revocable_init(struct revocable_provider __rcu *_rp, struct revocable *rev)
{
struct revocable_provider *rp;
if (!_rp)
return -ENODEV;
/*
* If revocable_provider_revoke() is called concurrently at this
* point, _rp is not affectd by rcu_replace_pointer().
*
* Additionally, nothing prevents a concurrent kfree_rcu() from
* freeing the revocable provider before we enter the RCU
* read-side critical section below.
*/
/*
* 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) {
...
}
Do I miss anything?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] Revert "revocable: Revocable resource management"
2026-02-05 11:56 ` Danilo Krummrich
@ 2026-02-06 9:14 ` Tzung-Bi Shih
0 siblings, 0 replies; 13+ messages in thread
From: Tzung-Bi Shih @ 2026-02-06 9:14 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Johan Hovold, Greg Kroah-Hartman, Rafael J . Wysocki,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Thu, Feb 05, 2026 at 12:56:47PM +0100, Danilo Krummrich wrote:
> should be
>
> int revocable_init(struct revocable_provider __rcu **_rp, ...)
>
> instead of
>
> int revocable_init(struct revocable_provider __rcu *_rp, ...)
>
> for the same reason revocable_provider_revoke() takes a double pointer.
>
> Otherwise this seems racy:
>
> int revocable_init(struct revocable_provider __rcu *_rp, struct revocable *rev)
> {
> struct revocable_provider *rp;
>
> if (!_rp)
> return -ENODEV;
>
> /*
> * If revocable_provider_revoke() is called concurrently at this
> * point, _rp is not affectd by rcu_replace_pointer().
> *
> * Additionally, nothing prevents a concurrent kfree_rcu() from
> * freeing the revocable provider before we enter the RCU
> * read-side critical section below.
> */
>
> /*
> * 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) {
>
> ...
> }
>
> Do I miss anything?
You're right. Will fix that.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] Revert "revocable: Revocable resource management"
2026-02-05 8:51 ` Tzung-Bi Shih
2026-02-05 11:56 ` Danilo Krummrich
@ 2026-02-05 14:03 ` Johan Hovold
2026-02-06 9:14 ` Tzung-Bi Shih
1 sibling, 1 reply; 13+ messages in thread
From: Johan Hovold @ 2026-02-05 14:03 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Thu, Feb 05, 2026 at 08:51:19AM +0000, Tzung-Bi Shih wrote:
> On Wed, Feb 04, 2026 at 03:28:49PM +0100, Johan Hovold wrote:
> > Specifically, the latest design relies on RCU for storing a pointer to
> > the revocable provider, but since the resource can be shared by value
> > (e.g. as in the now reverted selftests) this does not work at all and
> > can also lead to use-after-free:
> [...]
> > producer:
> >
> > priv->rp = revocable_provider_alloc(&priv->res);
> > // pass priv->rp by value to consumer
> > revocable_provider_revoke(&priv->rp);
> >
> > consumer:
> >
> > struct revocable_provider __rcu *rp = filp->private_data;
> > struct revocable *rev;
> >
> > revocable_init(rp, &rev);
> >
> > as _rp would still be non-NULL in revocable_init() regardless of whether
> > the producer has revoked the resource and set its pointer to NULL.
>
> You're right to point out the issue with copying the pointer of revocable
> provider. If a consumer stores this pointer directly, rcu_replace_pointer()
> in the producer's revocable_provider_revoke() will not affect the consumer's
> copy. I understand this concern.
>
> The intention was never for consumers to cache the pointer of revocable
> provider long-term. The design relies on consumers obtaining the current
> valid provider pointer at the point of access.
But that is not what the selftest does currently, nor is it what you
need for your initial motivation for this which was miscdev where you
don't have any reference counted driver data to store the pointer in.
> In the latest GPIO transition series [5], the usage pattern has been refined
> to avoid locally storing the pointer of revocable provider. Instead, it's
> fetched from a source of truth when needed.
Right, but then you don't need all the RCU stuff. And revocable becomes
just a convoluted abstraction for a lock and flag (as was pointed out
early on).
> I agree that the risks and correct usage patterns need to be much clearer.
> I'll update the Documentation and the selftests to explicitly highlight
> this limitation and demonstrate the proper way to interact with the API,
> avoiding the storage of the provider pointer by value in consumer contexts.
And again, that's precisely why we need to evaluate the API in a
non-trivial context *before* merging yet another version of this.
> > Essentially revocable still relies on having a pointer to reference
> > counted driver data which holds the revocable provider, which makes all
> > the RCU protection unnecessary along with most of the current revocable
> > design and implementation.
>
> (I'm assuming you are referring to the example in [6].)
>
> I'm not sure I follow your reasoning. Per my understanding:
> - The reference counted driver data (e.g. `gdev` in the GPIO example) is to
> ensure the pointer of revocable provider isn't freed.
> - The RCU protects the pointer value from concurrent access and updates
> during the revocation process [7].
>
> These seem to address different aspects. Could you provide more context
> on why you see the RCU protection as redundant?
I wasn't thinking of any particular example.
The struct revocable_provider is already reference counted and you don't
need anything more than that as long as you only take another reference
in a context where you already hold a reference. (And struct
revocable_provider should be renamed struct revocable).
SRCU is what prevents the race against revoke, no need for RCU.
But this is designing yet another version of revocable. And that should
be done out-of-tree.
Johan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] Revert "revocable: Revocable resource management"
2026-02-05 14:03 ` Johan Hovold
@ 2026-02-06 9:14 ` Tzung-Bi Shih
2026-02-06 15:07 ` Johan Hovold
0 siblings, 1 reply; 13+ messages in thread
From: Tzung-Bi Shih @ 2026-02-06 9:14 UTC (permalink / raw)
To: Johan Hovold
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Thu, Feb 05, 2026 at 03:03:21PM +0100, Johan Hovold wrote:
> On Thu, Feb 05, 2026 at 08:51:19AM +0000, Tzung-Bi Shih wrote:
> > On Wed, Feb 04, 2026 at 03:28:49PM +0100, Johan Hovold wrote:
> > > Specifically, the latest design relies on RCU for storing a pointer to
> > > the revocable provider, but since the resource can be shared by value
> > > (e.g. as in the now reverted selftests) this does not work at all and
> > > can also lead to use-after-free:
> > [...]
> > > producer:
> > >
> > > priv->rp = revocable_provider_alloc(&priv->res);
> > > // pass priv->rp by value to consumer
> > > revocable_provider_revoke(&priv->rp);
> > >
> > > consumer:
> > >
> > > struct revocable_provider __rcu *rp = filp->private_data;
> > > struct revocable *rev;
> > >
> > > revocable_init(rp, &rev);
> > >
> > > as _rp would still be non-NULL in revocable_init() regardless of whether
> > > the producer has revoked the resource and set its pointer to NULL.
> >
> > You're right to point out the issue with copying the pointer of revocable
> > provider. If a consumer stores this pointer directly, rcu_replace_pointer()
> > in the producer's revocable_provider_revoke() will not affect the consumer's
> > copy. I understand this concern.
> >
> > The intention was never for consumers to cache the pointer of revocable
> > provider long-term. The design relies on consumers obtaining the current
> > valid provider pointer at the point of access.
>
> But that is not what the selftest does currently, nor is it what you
Understood. As mentioned in my previous email, I'll update the selftests to
reflect this. The test case somehow is simplified to setup the scenario it
wants to test.
> need for your initial motivation for this which was miscdev where you
> don't have any reference counted driver data to store the pointer in.
You're right that cros_ec_chardev.c needs to be adjusted before using the
version of revocable.
>
> > In the latest GPIO transition series [5], the usage pattern has been refined
> > to avoid locally storing the pointer of revocable provider. Instead, it's
> > fetched from a source of truth when needed.
>
> Right, but then you don't need all the RCU stuff. And revocable becomes
> just a convoluted abstraction for a lock and flag (as was pointed out
> early on).
I'm not sure if I understand you: as we're discussing about the stale pointer
of revocable provider consumers might cache here, do you mean it doesn't need
RCU at all to protect the race you reported in
https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/?
>
> > I agree that the risks and correct usage patterns need to be much clearer.
> > I'll update the Documentation and the selftests to explicitly highlight
> > this limitation and demonstrate the proper way to interact with the API,
> > avoiding the storage of the provider pointer by value in consumer contexts.
>
> And again, that's precisely why we need to evaluate the API in a
> non-trivial context *before* merging yet another version of this.
>
> > > Essentially revocable still relies on having a pointer to reference
> > > counted driver data which holds the revocable provider, which makes all
> > > the RCU protection unnecessary along with most of the current revocable
> > > design and implementation.
> >
> > (I'm assuming you are referring to the example in [6].)
> >
> > I'm not sure I follow your reasoning. Per my understanding:
> > - The reference counted driver data (e.g. `gdev` in the GPIO example) is to
> > ensure the pointer of revocable provider isn't freed.
> > - The RCU protects the pointer value from concurrent access and updates
> > during the revocation process [7].
> >
> > These seem to address different aspects. Could you provide more context
> > on why you see the RCU protection as redundant?
>
> I wasn't thinking of any particular example.
>
> The struct revocable_provider is already reference counted and you don't
> need anything more than that as long as you only take another reference
> in a context where you already hold a reference. (And struct
The reference count in struct revocable_provider can't protect the race
reported https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/.
> revocable_provider should be renamed struct revocable).
Probably no, struct revocable is for consumer handle. It needs somewhere to
store the SRCU index.
>
> SRCU is what prevents the race against revoke, no need for RCU.
No, without the RCU, it can't prevent the race between revoking and creating
a new consumer handle.
>
> But this is designing yet another version of revocable. And that should
> be done out-of-tree.
>
> Johan
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] Revert "revocable: Revocable resource management"
2026-02-06 9:14 ` Tzung-Bi Shih
@ 2026-02-06 15:07 ` Johan Hovold
0 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2026-02-06 15:07 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Fri, Feb 06, 2026 at 09:14:13AM +0000, Tzung-Bi Shih wrote:
> On Thu, Feb 05, 2026 at 03:03:21PM +0100, Johan Hovold wrote:
> > On Thu, Feb 05, 2026 at 08:51:19AM +0000, Tzung-Bi Shih wrote:
> > > You're right to point out the issue with copying the pointer of revocable
> > > provider. If a consumer stores this pointer directly, rcu_replace_pointer()
> > > in the producer's revocable_provider_revoke() will not affect the consumer's
> > > copy. I understand this concern.
> > >
> > > The intention was never for consumers to cache the pointer of revocable
> > > provider long-term. The design relies on consumers obtaining the current
> > > valid provider pointer at the point of access.
> >
> > But that is not what the selftest does currently, nor is it what you
>
> Understood. As mentioned in my previous email, I'll update the selftests to
> reflect this. The test case somehow is simplified to setup the scenario it
> wants to test.
>
> > need for your initial motivation for this which was miscdev where you
> > don't have any reference counted driver data to store the pointer in.
>
> You're right that cros_ec_chardev.c needs to be adjusted before using the
> version of revocable.
That's part of the problem here. It's unclear what problem or problems
you're trying to solve. You started off with the miscdev issue, this
thing morphed into something else, and now everything is just a big
mess with unclear semantics and a totally confused implementation.
Look, I'm not blaming you for how we ended up here. This thing should
not have been merged until there was a general agreement on the
usefulness of the API (which requires a problem statement and use case).
The right people clearly didn't look at this in any detail since they
did not expect it to be merged any time soon.
And it's just unfair to you to have to redesign the whole thing under
time pressure a few days before the merge window opens. New code is
supposed to be ready weeks in advance, not be created and merged only
days before.
> > > In the latest GPIO transition series [5], the usage pattern has been refined
> > > to avoid locally storing the pointer of revocable provider. Instead, it's
> > > fetched from a source of truth when needed.
> >
> > Right, but then you don't need all the RCU stuff. And revocable becomes
> > just a convoluted abstraction for a lock and flag (as was pointed out
> > early on).
>
> I'm not sure if I understand you: as we're discussing about the stale pointer
> of revocable provider consumers might cache here, do you mean it doesn't need
> RCU at all to protect the race you reported in
> https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/?
Right. It's not needed. You have a refcounted structure and you can use
the refcount to prevent it from going away.
> > > I agree that the risks and correct usage patterns need to be much clearer.
> > > I'll update the Documentation and the selftests to explicitly highlight
> > > this limitation and demonstrate the proper way to interact with the API,
> > > avoiding the storage of the provider pointer by value in consumer contexts.
> >
> > And again, that's precisely why we need to evaluate the API in a
> > non-trivial context *before* merging yet another version of this.
> >
> > > > Essentially revocable still relies on having a pointer to reference
> > > > counted driver data which holds the revocable provider, which makes all
> > > > the RCU protection unnecessary along with most of the current revocable
> > > > design and implementation.
> > >
> > > (I'm assuming you are referring to the example in [6].)
> > >
> > > I'm not sure I follow your reasoning. Per my understanding:
> > > - The reference counted driver data (e.g. `gdev` in the GPIO example) is to
> > > ensure the pointer of revocable provider isn't freed.
> > > - The RCU protects the pointer value from concurrent access and updates
> > > during the revocation process [7].
> > >
> > > These seem to address different aspects. Could you provide more context
> > > on why you see the RCU protection as redundant?
> >
> > I wasn't thinking of any particular example.
> >
> > The struct revocable_provider is already reference counted and you don't
> > need anything more than that as long as you only take another reference
> > in a context where you already hold a reference. (And struct
>
> The reference count in struct revocable_provider can't protect the race
> reported https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/.
Yes, it can. You just need to make sure you hold a reference. And
redesign the API to let you do so.
> > revocable_provider should be renamed struct revocable).
>
> Probably no, struct revocable is for consumer handle. It needs somewhere to
> store the SRCU index.
Yeah, you'd need to store the SRCU index elsewhere (e.g. int, typedef,
context struct).
> > SRCU is what prevents the race against revoke, no need for RCU.
>
> No, without the RCU, it can't prevent the race between revoking and creating
> a new consumer handle.
Yes, it can. You just need to take a step back and redesign this thing
again.
> > But this is designing yet another version of revocable. And that should
> > be done out-of-tree.
And that should be done out-of-tree.
Johan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] Revert "revocable: Revocable resource management"
2026-02-04 14:28 [PATCH v2 0/3] Revert "revocable: Revocable resource management" Johan Hovold
` (2 preceding siblings ...)
2026-02-04 14:28 ` [PATCH v2 3/3] Revert "revocable: Revocable resource management" Johan Hovold
@ 2026-02-06 15:13 ` Greg Kroah-Hartman
2026-02-07 14:00 ` Tzung-Bi Shih
2026-02-13 8:32 ` Bartosz Golaszewski
3 siblings, 2 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2026-02-06 15:13 UTC (permalink / raw)
To: Johan Hovold
Cc: Rafael J . Wysocki, Danilo Krummrich, Tzung-Bi Shih,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Wed, Feb 04, 2026 at 03:28:46PM +0100, Johan Hovold wrote:
> I was surprised to learn that the revocable functionality was merged the other
> week given the community feedback on list and at LPC, but not least since there
> are no users of it, which we are supposed to require to be able to evaluate it
> properly.
>
> The chromeos ec driver issue which motivated this work turned out not to need
> it as was found during review. And the example gpiolib conversion was posted
> the very same morning that this was merged which hardly provides enough time
> for evaluation (even if Bartosz quickly reported a performance regression).
>
> Turns out there are correctness issues with both the gpiolib conversion and
> the revocable design itself that can lead to use-after-free and hung tasks (see
> [1] and [2]).
>
> And as was pointed out repeatedly during review, and again at the day of the
> merge, this does not look like the right interface for the chardev unplug
> issue.
>
> Despite the last-minute attempt at addressing the issues mentioned above
> incrementally, the revocable design is still fundamentally flawed (see patch
> 3/3).
>
> We have processes like requiring a user before merging a new interface so that
> issues like these can be identified and the soundness of an API be evaluated.
> They also give a sense of when things are expected to happen, which allows our
> scarce reviewers to manage their time (e.g. to not be forced to drop everything
> else they are doing when things are merged prematurely).
>
> There really is no reason to exempt any new interface from this regardless of
> whether one likes the underlying concept or not.
>
> Revert the revocable implementation until a redesign has been proposed and
> evaluated properly.
After thinking about this a lot, and talking it over with Danilo a bit,
I've applied this series that reverts these changes.
Kernel developers / maintainers are only "allowed" one major argument /
fight a year, and I really don't want to burn my 2026 usage so early in
the year :)
Tzung-Bi, can you take the feedback here, and what you have learned from
the gpio patch series, and rework this into a "clean" patch series for
us to review and comment on for future releases? That should give us
all a baseline on which to work off of, without having to worry about
the different versions/fixes floating around at the moment.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/3] Revert "revocable: Revocable resource management"
2026-02-06 15:13 ` [PATCH v2 0/3] " Greg Kroah-Hartman
@ 2026-02-07 14:00 ` Tzung-Bi Shih
2026-02-13 8:32 ` Bartosz Golaszewski
1 sibling, 0 replies; 13+ messages in thread
From: Tzung-Bi Shih @ 2026-02-07 14:00 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Johan Hovold, Rafael J . Wysocki, Danilo Krummrich,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Fri, Feb 06, 2026 at 04:13:00PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 04, 2026 at 03:28:46PM +0100, Johan Hovold wrote:
> > I was surprised to learn that the revocable functionality was merged the other
> > week given the community feedback on list and at LPC, but not least since there
> > are no users of it, which we are supposed to require to be able to evaluate it
> > properly.
> >
> > The chromeos ec driver issue which motivated this work turned out not to need
> > it as was found during review. And the example gpiolib conversion was posted
> > the very same morning that this was merged which hardly provides enough time
> > for evaluation (even if Bartosz quickly reported a performance regression).
> >
> > Turns out there are correctness issues with both the gpiolib conversion and
> > the revocable design itself that can lead to use-after-free and hung tasks (see
> > [1] and [2]).
> >
> > And as was pointed out repeatedly during review, and again at the day of the
> > merge, this does not look like the right interface for the chardev unplug
> > issue.
> >
> > Despite the last-minute attempt at addressing the issues mentioned above
> > incrementally, the revocable design is still fundamentally flawed (see patch
> > 3/3).
> >
> > We have processes like requiring a user before merging a new interface so that
> > issues like these can be identified and the soundness of an API be evaluated.
> > They also give a sense of when things are expected to happen, which allows our
> > scarce reviewers to manage their time (e.g. to not be forced to drop everything
> > else they are doing when things are merged prematurely).
> >
> > There really is no reason to exempt any new interface from this regardless of
> > whether one likes the underlying concept or not.
> >
> > Revert the revocable implementation until a redesign has been proposed and
> > evaluated properly.
>
> After thinking about this a lot, and talking it over with Danilo a bit,
> I've applied this series that reverts these changes.
>
> Kernel developers / maintainers are only "allowed" one major argument /
> fight a year, and I really don't want to burn my 2026 usage so early in
> the year :)
>
> Tzung-Bi, can you take the feedback here, and what you have learned from
> the gpio patch series, and rework this into a "clean" patch series for
> us to review and comment on for future releases? That should give us
> all a baseline on which to work off of, without having to worry about
> the different versions/fixes floating around at the moment.
Acknowledged. I'll start reworking this into a unified series that
incorporates the feedback and lessons learned.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] Revert "revocable: Revocable resource management"
2026-02-06 15:13 ` [PATCH v2 0/3] " Greg Kroah-Hartman
2026-02-07 14:00 ` Tzung-Bi Shih
@ 2026-02-13 8:32 ` Bartosz Golaszewski
1 sibling, 0 replies; 13+ messages in thread
From: Bartosz Golaszewski @ 2026-02-13 8:32 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Johan Hovold, Rafael J . Wysocki, Danilo Krummrich, Tzung-Bi Shih,
Bartosz Golaszewski, Linus Walleij, Jonathan Corbet, Shuah Khan,
Laurent Pinchart, Wolfram Sang, Simona Vetter, Dan Williams,
Jason Gunthorpe, linux-doc, linux-kselftest, linux-kernel
On Fri, Feb 6, 2026 at 4:17 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Feb 04, 2026 at 03:28:46PM +0100, Johan Hovold wrote:
> > I was surprised to learn that the revocable functionality was merged the other
> > week given the community feedback on list and at LPC, but not least since there
> > are no users of it, which we are supposed to require to be able to evaluate it
> > properly.
> >
> > The chromeos ec driver issue which motivated this work turned out not to need
> > it as was found during review. And the example gpiolib conversion was posted
> > the very same morning that this was merged which hardly provides enough time
> > for evaluation (even if Bartosz quickly reported a performance regression).
> >
> > Turns out there are correctness issues with both the gpiolib conversion and
> > the revocable design itself that can lead to use-after-free and hung tasks (see
> > [1] and [2]).
> >
> > And as was pointed out repeatedly during review, and again at the day of the
> > merge, this does not look like the right interface for the chardev unplug
> > issue.
> >
> > Despite the last-minute attempt at addressing the issues mentioned above
> > incrementally, the revocable design is still fundamentally flawed (see patch
> > 3/3).
> >
> > We have processes like requiring a user before merging a new interface so that
> > issues like these can be identified and the soundness of an API be evaluated.
> > They also give a sense of when things are expected to happen, which allows our
> > scarce reviewers to manage their time (e.g. to not be forced to drop everything
> > else they are doing when things are merged prematurely).
> >
> > There really is no reason to exempt any new interface from this regardless of
> > whether one likes the underlying concept or not.
> >
> > Revert the revocable implementation until a redesign has been proposed and
> > evaluated properly.
>
> After thinking about this a lot, and talking it over with Danilo a bit,
> I've applied this series that reverts these changes.
>
> Kernel developers / maintainers are only "allowed" one major argument /
> fight a year, and I really don't want to burn my 2026 usage so early in
> the year :)
>
> Tzung-Bi, can you take the feedback here, and what you have learned from
> the gpio patch series, and rework this into a "clean" patch series for
> us to review and comment on for future releases? That should give us
> all a baseline on which to work off of, without having to worry about
> the different versions/fixes floating around at the moment.
>
I think it's a good decision. I definitely want to see it upstream but
it needs a serious rework and I think it should go upstream together
with the first user. I'm fine with it being the GPIO subsystem and
happy to help with reviewing and development.
Bartosz
^ permalink raw reply [flat|nested] 13+ messages in thread