This patch series brings documentation and refactorings to the `Revocable` type.

Changes include:
- Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
- Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization), now returning `bool` to indicate revocation status.

Marcelo Moreira (2):
  rust: revocable: Refactor revocation mechanism to remove generic
    revoke_internal
  rust: revocable: Clarify write invariant and update safety comments

Changelog
---------

Changes since v4:
- Rebased the series onto the latest `rfl/rust-next` to integrate recent changes, specifically the `bool` return for `revoke()` and `revoke_nosync()`.
- Dropped the "rust: revocable: simplify RevocableGuard for internal safety" patch, as the approach of using a direct reference (`&'a T`) for `RevocableGuard` was found to be unsound due to Rust's aliasing rules and LLVM's `dereferencable` attribute guarantees, which require references to remain valid for the entire function call duration, even if the internal RCU guard is dropped earlier.
- Refined the `PinnedDrop::drop` `SAFETY` comment based on Benno Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
- Corrected a duplicated line in the commit message of the second patch.

Link to v4: https://lore.kernel.org/rust-for-linux/DAOMIWBZXFO9.U353H8NWTLC5@kernel.org/T/#u

Changes since v3:
- Refined the wording of the `Revocable<T>` invariants to be more precise about read and write validity conditions, specifically including RCU read-side lock acquisition timing for reads and RCU grace period for writes.
- Simplified the `try_access_with_guard` safety comment for better conciseness.
- Refactored `RevocableGuard` to use `&'a T` instead of `*const T`, removing its internal invariants and `unsafe` blocks.
- Simplified `Revocable::try_access` to leverage `try_access_with_guard` and `map`.
- Split `revoke_internal` into `revoke()` and `revoke_nosync()` functions, making synchronization behavior explicit.
- Link to v3: https://lore.kernel.org/rust-for-linux/DAOMIWBZXFO9.U353H8NWTLC5@kernel.org/T/#u

Changes in v2:
- Refined the wording of the invariants in `Revocable<T>` to be more direct and address feedback regarding the phrase 'must occur'.
- Added '// INVARIANT:' comments in `try_access` and `try_access_with_guard` as suggested by reviewers.
- Added the missing invariant for `RevocableGuard<'_, T>` regarding the validity of `data_ref`.
- Updated the safety comment in the `Deref` implementation of `RevocableGuard` to refer to the new invariant.
- Link to v2: https://lore.kernel.org/rust-for-linux/CAPZ3m_jw0LxK1MmseaamNYhj9VY8AXtJ0AOcYd9qcn=5wPE4eA@mail.gmail.com/T/#t

 rust/kernel/revocable.rs | 68 +++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 32 deletions(-)

base-commit: 0303584766b7bdb6564c7e8f13e0b59b6ef44984

-- 
2.50.0

On Thu Jun 26, 2025 at 6:59 PM CEST, Marcelo Moreira wrote:
> This patch series brings documentation and refactorings to the `Revocable` type.
>
> Changes include:
> - Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
> - Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization), now returning `bool` to indicate revocation status.

Could you wrap your text to a more readable column? Thanks!

>
> Marcelo Moreira (2):
>   rust: revocable: Refactor revocation mechanism to remove generic
>     revoke_internal
>   rust: revocable: Clarify write invariant and update safety comments
>
> Changelog
> ---------
>
> Changes since v4:
> - Rebased the series onto the latest `rfl/rust-next` to integrate recent changes, specifically the `bool` return for `revoke()` and `revoke_nosync()`.
> - Dropped the "rust: revocable: simplify RevocableGuard for internal safety" patch, as the approach of using a direct reference (`&'a T`) for `RevocableGuard` was found to be unsound due to Rust's aliasing rules and LLVM's `dereferencable` attribute guarantees, which require references to remain valid for the entire function call duration, even if the internal RCU guard is dropped earlier.
> - Refined the `PinnedDrop::drop` `SAFETY` comment based on Benno Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
> - Corrected a duplicated line in the commit message of the second patch.

Now since we had to drop the `RevocableGuard` change, its safety
invariant & comment in `deref` is insufficient. It shouldn't have the
invariant that the rcu lock is held (since it owns an `rcu::Guard`, that
already is guaranteed), but instead it should require that the
`data_ref` pointer is valid. That invariant is then used by the safety
comment in `deref` to justify dereferencing the pointer.

Also, I think it's better to reorder the patches again (since the
current first one relies on changes from the second one), the first one
should be the change to the invariants section of `Revocable` (so
currently the second patch). Then the second and third patches can be
the removal of `revoke_internal` and the `RevocableGuard` safety
documentation fix.

---
Cheers,
Benno

> Link to v4: https://lore.kernel.org/rust-for-linux/DAOMIWBZXFO9.U353H8NWTLC5@kernel.org/T/#u
>
> Changes since v3:
> - Refined the wording of the `Revocable<T>` invariants to be more precise about read and write validity conditions, specifically including RCU read-side lock acquisition timing for reads and RCU grace period for writes.
> - Simplified the `try_access_with_guard` safety comment for better conciseness.
> - Refactored `RevocableGuard` to use `&'a T` instead of `*const T`, removing its internal invariants and `unsafe` blocks.
> - Simplified `Revocable::try_access` to leverage `try_access_with_guard` and `map`.
> - Split `revoke_internal` into `revoke()` and `revoke_nosync()` functions, making synchronization behavior explicit.
> - Link to v3: https://lore.kernel.org/rust-for-linux/DAOMIWBZXFO9.U353H8NWTLC5@kernel.org/T/#u
>
> Changes in v2:
> - Refined the wording of the invariants in `Revocable<T>` to be more direct and address feedback regarding the phrase 'must occur'.
> - Added '// INVARIANT:' comments in `try_access` and `try_access_with_guard` as suggested by reviewers.
> - Added the missing invariant for `RevocableGuard<'_, T>` regarding the validity of `data_ref`.
> - Updated the safety comment in the `Deref` implementation of `RevocableGuard` to refer to the new invariant.
> - Link to v2: https://lore.kernel.org/rust-for-linux/CAPZ3m_jw0LxK1MmseaamNYhj9VY8AXtJ0AOcYd9qcn=5wPE4eA@mail.gmail.com/T/#t
>
>  rust/kernel/revocable.rs | 68 +++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 32 deletions(-)
>
> base-commit: 0303584766b7bdb6564c7e8f13e0b59b6ef44984

Em qui., 3 de jul. de 2025 às 05:24, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Thu Jun 26, 2025 at 6:59 PM CEST, Marcelo Moreira wrote:
> > This patch series brings documentation and refactorings to the `Revocable` type.
> >
> > Changes include:
> > - Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
> > - Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization), now returning `bool` to indicate revocation status.
>
> Could you wrap your text to a more readable column? Thanks!

Sure! Thanks!

>
> >
> > Marcelo Moreira (2):
> >   rust: revocable: Refactor revocation mechanism to remove generic
> >     revoke_internal
> >   rust: revocable: Clarify write invariant and update safety comments
> >
> > Changelog
> > ---------
> >
> > Changes since v4:
> > - Rebased the series onto the latest `rfl/rust-next` to integrate recent changes, specifically the `bool` return for `revoke()` and `revoke_nosync()`.
> > - Dropped the "rust: revocable: simplify RevocableGuard for internal safety" patch, as the approach of using a direct reference (`&'a T`) for `RevocableGuard` was found to be unsound due to Rust's aliasing rules and LLVM's `dereferencable` attribute guarantees, which require references to remain valid for the entire function call duration, even if the internal RCU guard is dropped earlier.
> > - Refined the `PinnedDrop::drop` `SAFETY` comment based on Benno Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
> > - Corrected a duplicated line in the commit message of the second patch.
>
> Now since we had to drop the `RevocableGuard` change, its safety
> invariant & comment in `deref` is insufficient. It shouldn't have the
> invariant that the rcu lock is held (since it owns an `rcu::Guard`, that
> already is guaranteed), but instead it should require that the
> `data_ref` pointer is valid. That invariant is then used by the safety
> comment in `deref` to justify dereferencing the pointer.
>
> Also, I think it's better to reorder the patches again (since the
> current first one relies on changes from the second one), the first one
> should be the change to the invariants section of `Revocable` (so
> currently the second patch). Then the second and third patches can be
> the removal of `revoke_internal` and the `RevocableGuard` safety
> documentation fix.

All right Benno, I'll prepare the comment for `RevocableGuard` and send v6.

The order now is:
1- Documentation for invariant and updates associated `SAFETY` comments
2- Remove `revoke_internal` (Refactoring)
3- `RevocableGuard` safety documentation fix.

Thanks! :)

--
Cheers,
Marcelo Moreira

On Sat Jul 5, 2025 at 7:09 AM CEST, Marcelo Moreira wrote:
> Em qui., 3 de jul. de 2025 às 05:24, Benno Lossin <lossin@kernel.org> escreveu:
>>
>> On Thu Jun 26, 2025 at 6:59 PM CEST, Marcelo Moreira wrote:
>> > This patch series brings documentation and refactorings to the `Revocable` type.
>> >
>> > Changes include:
>> > - Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
>> > - Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization), now returning `bool` to indicate revocation status.
>>
>> Could you wrap your text to a more readable column? Thanks!
>
> Sure! Thanks!
>
>>
>> >
>> > Marcelo Moreira (2):
>> >   rust: revocable: Refactor revocation mechanism to remove generic
>> >     revoke_internal
>> >   rust: revocable: Clarify write invariant and update safety comments
>> >
>> > Changelog
>> > ---------
>> >
>> > Changes since v4:
>> > - Rebased the series onto the latest `rfl/rust-next` to integrate recent changes, specifically the `bool` return for `revoke()` and `revoke_nosync()`.
>> > - Dropped the "rust: revocable: simplify RevocableGuard for internal safety" patch, as the approach of using a direct reference (`&'a T`) for `RevocableGuard` was found to be unsound due to Rust's aliasing rules and LLVM's `dereferencable` attribute guarantees, which require references to remain valid for the entire function call duration, even if the internal RCU guard is dropped earlier.
>> > - Refined the `PinnedDrop::drop` `SAFETY` comment based on Benno Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
>> > - Corrected a duplicated line in the commit message of the second patch.
>>
>> Now since we had to drop the `RevocableGuard` change, its safety
>> invariant & comment in `deref` is insufficient. It shouldn't have the
>> invariant that the rcu lock is held (since it owns an `rcu::Guard`, that
>> already is guaranteed), but instead it should require that the
>> `data_ref` pointer is valid. That invariant is then used by the safety
>> comment in `deref` to justify dereferencing the pointer.
>>
>> Also, I think it's better to reorder the patches again (since the
>> current first one relies on changes from the second one), the first one
>> should be the change to the invariants section of `Revocable` (so
>> currently the second patch). Then the second and third patches can be
>> the removal of `revoke_internal` and the `RevocableGuard` safety
>> documentation fix.
>
> All right Benno, I'll prepare the comment for `RevocableGuard` and send v6.
>
> The order now is:
> 1- Documentation for invariant and updates associated `SAFETY` comments
> 2- Remove `revoke_internal` (Refactoring)
> 3- `RevocableGuard` safety documentation fix.

Sounds good!

---
Cheers,
Benno

On Thu, Jun 26, 2025 at 6:59 PM Marcelo Moreira
<marcelomoreira1905@gmail.com> wrote:
>
> This patch series brings documentation and refactorings to the `Revocable` type.
>
> Changes include:
> - Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
> - Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization), now returning `bool` to indicate revocation status.
>
> Marcelo Moreira (2):
>   rust: revocable: Refactor revocation mechanism to remove generic
>     revoke_internal
>   rust: revocable: Clarify write invariant and update safety comments

Danilo, did you have Revocable / Devres changes that conflict with this?

Alice

On Tue, Jul 01, 2025 at 01:27:17PM +0200, Alice Ryhl wrote:
> On Thu, Jun 26, 2025 at 6:59 PM Marcelo Moreira
> <marcelomoreira1905@gmail.com> wrote:
> >
> > This patch series brings documentation and refactorings to the `Revocable` type.
> >
> > Changes include:
> > - Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
> > - Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization), now returning `bool` to indicate revocation status.
> >
> > Marcelo Moreira (2):
> >   rust: revocable: Refactor revocation mechanism to remove generic
> >     revoke_internal
> >   rust: revocable: Clarify write invariant and update safety comments
> 
> Danilo, did you have Revocable / Devres changes that conflict with this?

Yes, but I sent them to Linus for -rc3 already. Given that rust-next is based on
-rc3, we should be good. There shouldn't be any further conflicts.

Em ter., 1 de jul. de 2025 às 08:40, Danilo Krummrich
<dakr@kernel.org> escreveu:
>
> On Tue, Jul 01, 2025 at 01:27:17PM +0200, Alice Ryhl wrote:
> > On Thu, Jun 26, 2025 at 6:59 PM Marcelo Moreira
> > <marcelomoreira1905@gmail.com> wrote:
> > >
> > > This patch series brings documentation and refactorings to the `Revocable` type.
> > >
> > > Changes include:
> > > - Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
> > > - Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization), now returning `bool` to indicate revocation status.
> > >
> > > Marcelo Moreira (2):
> > >   rust: revocable: Refactor revocation mechanism to remove generic
> > >     revoke_internal
> > >   rust: revocable: Clarify write invariant and update safety comments
> >
> > Danilo, did you have Revocable / Devres changes that conflict with this?
>
> Yes, but I sent them to Linus for -rc3 already. Given that rust-next is based on
> -rc3, we should be good. There shouldn't be any further conflicts.

Hi guys!

I resolved the conflicts. I hope it is the way you expected.

-- 
Cheers,
Marcelo Moreira