Revert "revocable: Revocable resource management"

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.

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.

Quoted 4 lines · Johan Hovold

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:

[...]

Quoted 13 lines

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

Quoted 2 lines

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.

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?

Quoted 4 lines · Johan Hovold

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:

[...]

Quoted 13 lines

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

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 27 lines · Tzung-Bi Shih

Quoted 4 lines · Johan Hovold

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:

[...]

Quoted 13 lines

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

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 13 lines

Quoted 4 lines

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].

Quoted 2 lines

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

Quoted 7 lines · Johan Hovold

Quoted 5 lines · Tzung-Bi Shih

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 3 lines

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/[email protected]/?

Quoted 24 lines

Quoted 4 lines

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.

Quoted 15 lines

Quoted 4 lines

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?

Quoted 5 lines

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/[email protected]/.

Quoted 1 line

revocable_provider should be renamed struct revocable).

Probably no, struct revocable is for consumer handle. It needs somewhere to store the SRCU index.

Quoted 1 line

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.

Quoted 2 lines

But this is designing yet another version of revocable. And that should be done out-of-tree.

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.

Quoted 28 lines · Johan Hovold

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

Quoted 5 lines

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.

Quoted 28 lines · Johan Hovold

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

Quoted 5 lines

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.