[PATCH 1/3] rust: revocable: implement Revocable::access()

[PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

Implement an unsafe direct accessor for the data stored within the
Revocable.

This is useful for cases where we can proof that the data stored within
the Revocable is not and cannot be revoked for the duration of the
lifetime of the returned reference.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
The explicit lifetimes in access() probably don't serve a practical
purpose, but I found them to be useful for documentation purposes.
---
 rust/kernel/revocable.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 971d0dc38d83..33535de141ce 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
         self.try_access().map(|t| f(&*t))
     }
 
+    /// Directly access the revocable wrapped object.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
+    /// for the duration of `'a`.
+    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
+        // SAFETY: By the safety requirement of this function it is guaranteed that
+        // `self.data.get()` is a valid pointer to an instance of `T`.
+        unsafe { &*self.data.get() }
+    }
+
     /// # Safety
     ///
     /// Callers must ensure that there are no more concurrent users of the revocable object.
-- 
2.49.0

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Christian Schrefl]

On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> Implement an unsafe direct accessor for the data stored within the
> Revocable.
> 
> This is useful for cases where we can proof that the data stored within
> the Revocable is not and cannot be revoked for the duration of the
> lifetime of the returned reference.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes.
> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 971d0dc38d83..33535de141ce 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>          self.try_access().map(|t| f(&*t))
>      }
>  
> +    /// Directly access the revocable wrapped object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> +    /// for the duration of `'a`.
> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
I'm not sure if the `'s` lifetime really carries much meaning here.
I find just (explicit) `'a` on both parameter and return value is clearer to me,
but I'm not sure what others (particularly those not very familiar with rust)
think of this.

Either way:

Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

> +        // SAFETY: By the safety requirement of this function it is guaranteed that
> +        // `self.data.get()` is a valid pointer to an instance of `T`.
> +        unsafe { &*self.data.get() }
> +    }
> +
>      /// # Safety
>      ///
>      /// Callers must ensure that there are no more concurrent users of the revocable object.

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Boqun Feng]

On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > Implement an unsafe direct accessor for the data stored within the
> > Revocable.
> > 
> > This is useful for cases where we can proof that the data stored within
> > the Revocable is not and cannot be revoked for the duration of the
> > lifetime of the returned reference.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> > The explicit lifetimes in access() probably don't serve a practical
> > purpose, but I found them to be useful for documentation purposes.
> > --->  rust/kernel/revocable.rs | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 971d0dc38d83..33535de141ce 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >          self.try_access().map(|t| f(&*t))
> >      }
> >  
> > +    /// Directly access the revocable wrapped object.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > +    /// for the duration of `'a`.
> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> I'm not sure if the `'s` lifetime really carries much meaning here.
> I find just (explicit) `'a` on both parameter and return value is clearer to me,
> but I'm not sure what others (particularly those not very familiar with rust)
> think of this.

Yeah, I don't think we need two lifetimes here, the following version
should be fine (with implicit lifetime):

	pub unsafe fn access(&self) -> &T { ... }

, because if you do:

	let revocable: &'1 Revocable = ...;
	...
	let t: &'2 T = unsafe { revocable.access() };

'1 should already outlive '2 (i.e. '1: '2).

Regards,
Boqun

> 
> Either way:
> 
> Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> 
> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> > +        unsafe { &*self.data.get() }
> > +    }
> > +
> >      /// # Safety
> >      ///
> >      /// Callers must ensure that there are no more concurrent users of the revocable object.
> 

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> > On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > > Implement an unsafe direct accessor for the data stored within the
> > > Revocable.
> > > 
> > > This is useful for cases where we can proof that the data stored within
> > > the Revocable is not and cannot be revoked for the duration of the
> > > lifetime of the returned reference.
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > ---
> > > The explicit lifetimes in access() probably don't serve a practical
> > > purpose, but I found them to be useful for documentation purposes.
> > > --->  rust/kernel/revocable.rs | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > > index 971d0dc38d83..33535de141ce 100644
> > > --- a/rust/kernel/revocable.rs
> > > +++ b/rust/kernel/revocable.rs
> > > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > >          self.try_access().map(|t| f(&*t))
> > >      }
> > >  
> > > +    /// Directly access the revocable wrapped object.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > > +    /// for the duration of `'a`.
> > > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > I'm not sure if the `'s` lifetime really carries much meaning here.
> > I find just (explicit) `'a` on both parameter and return value is clearer to me,
> > but I'm not sure what others (particularly those not very familiar with rust)
> > think of this.
> 
> Yeah, I don't think we need two lifetimes here, the following version
> should be fine (with implicit lifetime):
> 
> 	pub unsafe fn access(&self) -> &T { ... }
> 
> , because if you do:
> 
> 	let revocable: &'1 Revocable = ...;
> 	...
> 	let t: &'2 T = unsafe { revocable.access() };
> 
> '1 should already outlive '2 (i.e. '1: '2).

Yes, this is indeed sufficient, that's why I wrote

	"The explicit lifetimes in access() probably don't serve a practical
	purpose, but I found them to be useful for documentation purposes."

below the commit message. :)

Any opinions in terms of documentation purposes?

> > 
> > Either way:
> > 
> > Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> > 
> > > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> > > +        unsafe { &*self.data.get() }
> > > +    }
> > > +
> > >      /// # Safety
> > >      ///
> > >      /// Callers must ensure that there are no more concurrent users of the revocable object.
> > 

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Christian Schrefl]

On 26.04.25 6:54 PM, Boqun Feng wrote:
> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>> Implement an unsafe direct accessor for the data stored within the
>>> Revocable.
>>>
>>> This is useful for cases where we can proof that the data stored within
>>> the Revocable is not and cannot be revoked for the duration of the
>>> lifetime of the returned reference.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>> ---
>>> The explicit lifetimes in access() probably don't serve a practical
>>> purpose, but I found them to be useful for documentation purposes.
>>> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>> index 971d0dc38d83..33535de141ce 100644
>>> --- a/rust/kernel/revocable.rs
>>> +++ b/rust/kernel/revocable.rs
>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>          self.try_access().map(|t| f(&*t))
>>>      }
>>>  
>>> +    /// Directly access the revocable wrapped object.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>> +    /// for the duration of `'a`.
>>> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>> I'm not sure if the `'s` lifetime really carries much meaning here.
>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>> but I'm not sure what others (particularly those not very familiar with rust)
>> think of this.
> 
> Yeah, I don't think we need two lifetimes here, the following version
> should be fine (with implicit lifetime):
> 
> 	pub unsafe fn access(&self) -> &T { ... }
> 
> , because if you do:
> 
> 	let revocable: &'1 Revocable = ...;
> 	...
> 	let t: &'2 T = unsafe { revocable.access() };
> 
> '1 should already outlive '2 (i.e. '1: '2).

I understand that implicit lifetimes desugars to 
effectively the same code, I just think that keeping
a explicit 'a makes it a bit more obvious that the
lifetimes need to be considered here.

But I'm also fine with just implicit lifetimes here.

Cheers
Christian

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Christian Schrefl]

On 26.04.25 7:01 PM, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
>> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>>> Implement an unsafe direct accessor for the data stored within the
>>>> Revocable.
>>>>
>>>> This is useful for cases where we can proof that the data stored within
>>>> the Revocable is not and cannot be revoked for the duration of the
>>>> lifetime of the returned reference.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>> The explicit lifetimes in access() probably don't serve a practical
>>>> purpose, but I found them to be useful for documentation purposes.
>>>> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>>> index 971d0dc38d83..33535de141ce 100644
>>>> --- a/rust/kernel/revocable.rs
>>>> +++ b/rust/kernel/revocable.rs
>>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>>          self.try_access().map(|t| f(&*t))
>>>>      }
>>>>  
>>>> +    /// Directly access the revocable wrapped object.
>>>> +    ///
>>>> +    /// # Safety
>>>> +    ///
>>>> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>>> +    /// for the duration of `'a`.
>>>> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>>> I'm not sure if the `'s` lifetime really carries much meaning here.
>>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>>> but I'm not sure what others (particularly those not very familiar with rust)
>>> think of this.
>>
>> Yeah, I don't think we need two lifetimes here, the following version
>> should be fine (with implicit lifetime):
>>
>> 	pub unsafe fn access(&self) -> &T { ... }
>>
>> , because if you do:
>>
>> 	let revocable: &'1 Revocable = ...;
>> 	...
>> 	let t: &'2 T = unsafe { revocable.access() };
>>
>> '1 should already outlive '2 (i.e. '1: '2).
> 
> Yes, this is indeed sufficient, that's why I wrote
> 
> 	"The explicit lifetimes in access() probably don't serve a practical
> 	purpose, but I found them to be useful for documentation purposes."
> 
> below the commit message. :)
> 
> Any opinions in terms of documentation purposes?

I would prefer just one explicit lifetime, but I'm
not sure about others.
But I think either way is fine.

Maybe I should have written it more clearly that I 
only meant that the second lifetime makes it 
IMO unnecessarily complicated when reading it.

Cheers
Christian

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Boqun Feng]

On Sat, Apr 26, 2025 at 07:01:52PM +0200, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
> > On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> > > On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > > > Implement an unsafe direct accessor for the data stored within the
> > > > Revocable.
> > > > 
> > > > This is useful for cases where we can proof that the data stored within
> > > > the Revocable is not and cannot be revoked for the duration of the
> > > > lifetime of the returned reference.
> > > > 
> > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > > ---
> > > > The explicit lifetimes in access() probably don't serve a practical
> > > > purpose, but I found them to be useful for documentation purposes.
> > > > --->  rust/kernel/revocable.rs | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > > > index 971d0dc38d83..33535de141ce 100644
> > > > --- a/rust/kernel/revocable.rs
> > > > +++ b/rust/kernel/revocable.rs
> > > > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > > >          self.try_access().map(|t| f(&*t))
> > > >      }
> > > >  
> > > > +    /// Directly access the revocable wrapped object.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > > > +    /// for the duration of `'a`.
> > > > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > > I'm not sure if the `'s` lifetime really carries much meaning here.
> > > I find just (explicit) `'a` on both parameter and return value is clearer to me,
> > > but I'm not sure what others (particularly those not very familiar with rust)
> > > think of this.
> > 
> > Yeah, I don't think we need two lifetimes here, the following version
> > should be fine (with implicit lifetime):
> > 
> > 	pub unsafe fn access(&self) -> &T { ... }
> > 
> > , because if you do:
> > 
> > 	let revocable: &'1 Revocable = ...;
> > 	...
> > 	let t: &'2 T = unsafe { revocable.access() };
> > 
> > '1 should already outlive '2 (i.e. '1: '2).
> 
> Yes, this is indeed sufficient, that's why I wrote
> 
> 	"The explicit lifetimes in access() probably don't serve a practical
> 	purpose, but I found them to be useful for documentation purposes."
> 
> below the commit message. :)
> 

Sorry, I overlooked.

> Any opinions in terms of documentation purposes?
> 

I think for access() the explicit lifetimes is unnecessary, because it's
a one-lifetime case, the two explicit lifetimes would make a simple case
looking complicated.

For access_with(), that's needed and a good idea.

Just my two cents.

Regards,
Boqun

> > > 
> > > Either way:
> > > 
> > > Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> > > 
> > > > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > > > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> > > > +        unsafe { &*self.data.get() }
> > > > +    }
> > > > +
> > > >      /// # Safety
> > > >      ///
> > > >      /// Callers must ensure that there are no more concurrent users of the revocable object.
> > > 

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sat Apr 26, 2025 at 7:03 PM CEST, Christian Schrefl wrote:
> On 26.04.25 6:54 PM, Boqun Feng wrote:
>> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>>> Implement an unsafe direct accessor for the data stored within the
>>>> Revocable.
>>>>
>>>> This is useful for cases where we can proof that the data stored within
>>>> the Revocable is not and cannot be revoked for the duration of the
>>>> lifetime of the returned reference.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>> The explicit lifetimes in access() probably don't serve a practical
>>>> purpose, but I found them to be useful for documentation purposes.
>>>> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>>> index 971d0dc38d83..33535de141ce 100644
>>>> --- a/rust/kernel/revocable.rs
>>>> +++ b/rust/kernel/revocable.rs
>>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>>          self.try_access().map(|t| f(&*t))
>>>>      }
>>>>  
>>>> +    /// Directly access the revocable wrapped object.
>>>> +    ///
>>>> +    /// # Safety
>>>> +    ///
>>>> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>>> +    /// for the duration of `'a`.
>>>> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>>> I'm not sure if the `'s` lifetime really carries much meaning here.
>>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>>> but I'm not sure what others (particularly those not very familiar with rust)
>>> think of this.
>> 
>> Yeah, I don't think we need two lifetimes here, the following version
>> should be fine (with implicit lifetime):
>> 
>> 	pub unsafe fn access(&self) -> &T { ... }
>> 
>> , because if you do:
>> 
>> 	let revocable: &'1 Revocable = ...;
>> 	...
>> 	let t: &'2 T = unsafe { revocable.access() };
>> 
>> '1 should already outlive '2 (i.e. '1: '2).
>
> I understand that implicit lifetimes desugars to 
> effectively the same code, I just think that keeping
> a explicit 'a makes it a bit more obvious that the
> lifetimes need to be considered here.
>
> But I'm also fine with just implicit lifetimes here.

We elide lifetimes all over the place especially for methods taking
`&self` and returning `&T`. I don't think that it serves a purpose here.

---
Cheers,
Benno

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> Implement an unsafe direct accessor for the data stored within the
> Revocable.
>
> This is useful for cases where we can proof that the data stored within
> the Revocable is not and cannot be revoked for the duration of the
> lifetime of the returned reference.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes.
> ---
>  rust/kernel/revocable.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 971d0dc38d83..33535de141ce 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>          self.try_access().map(|t| f(&*t))
>      }
>  
> +    /// Directly access the revocable wrapped object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> +    /// for the duration of `'a`.

Ah I missed this in my other email, in case you want to directly refer
to the lifetime, you should keep it defined. I would still remove the
`'s` lifetime though.
> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> +        // SAFETY: By the safety requirement of this function it is guaranteed that
> +        // `self.data.get()` is a valid pointer to an instance of `T`.

I don't see how the "not-being revoked" state makes the `data` ptr be
valid. Is that an invariant of `Revocable`? (it's not documented to have
any invariants)

---
Cheers,
Benno

> +        unsafe { &*self.data.get() }
> +    }
> +
>      /// # Safety
>      ///
>      /// Callers must ensure that there are no more concurrent users of the revocable object.

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> > Implement an unsafe direct accessor for the data stored within the
> > Revocable.
> >
> > This is useful for cases where we can proof that the data stored within
> > the Revocable is not and cannot be revoked for the duration of the
> > lifetime of the returned reference.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> > The explicit lifetimes in access() probably don't serve a practical
> > purpose, but I found them to be useful for documentation purposes.
> > ---
> >  rust/kernel/revocable.rs | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 971d0dc38d83..33535de141ce 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >          self.try_access().map(|t| f(&*t))
> >      }
> >  
> > +    /// Directly access the revocable wrapped object.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > +    /// for the duration of `'a`.
> 
> Ah I missed this in my other email, in case you want to directly refer
> to the lifetime, you should keep it defined. I would still remove the
> `'s` lifetime though.
> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> 
> I don't see how the "not-being revoked" state makes the `data` ptr be
> valid. Is that an invariant of `Revocable`? (it's not documented to have
> any invariants)

What else makes it valid?

AFAICS, try_access() and try_access_with_guard() argue the exact same way,
except that the reason for not being revoked is the atomic check and the RCU
read lock.

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> > Implement an unsafe direct accessor for the data stored within the
>> > Revocable.
>> >
>> > This is useful for cases where we can proof that the data stored within
>> > the Revocable is not and cannot be revoked for the duration of the
>> > lifetime of the returned reference.
>> >
>> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> > ---
>> > The explicit lifetimes in access() probably don't serve a practical
>> > purpose, but I found them to be useful for documentation purposes.
>> > ---
>> >  rust/kernel/revocable.rs | 12 ++++++++++++
>> >  1 file changed, 12 insertions(+)
>> >
>> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> > index 971d0dc38d83..33535de141ce 100644
>> > --- a/rust/kernel/revocable.rs
>> > +++ b/rust/kernel/revocable.rs
>> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>> >          self.try_access().map(|t| f(&*t))
>> >      }
>> >  
>> > +    /// Directly access the revocable wrapped object.
>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>> > +    /// for the duration of `'a`.
>> 
>> Ah I missed this in my other email, in case you want to directly refer
>> to the lifetime, you should keep it defined. I would still remove the
>> `'s` lifetime though.
>> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
>> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
>> 
>> I don't see how the "not-being revoked" state makes the `data` ptr be
>> valid. Is that an invariant of `Revocable`? (it's not documented to have
>> any invariants)
>
> What else makes it valid?

IMO an `# Invariants` section with the corresponding invariant that
`data` is valid when `is_available` is true.

> AFAICS, try_access() and try_access_with_guard() argue the exact same way,
> except that the reason for not being revoked is the atomic check and the RCU
> read lock.

Just because other code is doing the same mistake doesn't make it
correct. If I had reviewed the patch at that time I'm sure I would have
pointed this out.

I opened an issue about this:

    https://github.com/Rust-for-Linux/linux/issues/1160

Feel free to comment any additional information.

---
Cheers,
Benno

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 08:37:00AM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> > Implement an unsafe direct accessor for the data stored within the
> >> > Revocable.
> >> >
> >> > This is useful for cases where we can proof that the data stored within
> >> > the Revocable is not and cannot be revoked for the duration of the
> >> > lifetime of the returned reference.
> >> >
> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> > ---
> >> > The explicit lifetimes in access() probably don't serve a practical
> >> > purpose, but I found them to be useful for documentation purposes.
> >> > ---
> >> >  rust/kernel/revocable.rs | 12 ++++++++++++
> >> >  1 file changed, 12 insertions(+)
> >> >
> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> >> > index 971d0dc38d83..33535de141ce 100644
> >> > --- a/rust/kernel/revocable.rs
> >> > +++ b/rust/kernel/revocable.rs
> >> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >> >          self.try_access().map(|t| f(&*t))
> >> >      }
> >> >  
> >> > +    /// Directly access the revocable wrapped object.
> >> > +    ///
> >> > +    /// # Safety
> >> > +    ///
> >> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> >> > +    /// for the duration of `'a`.
> >> 
> >> Ah I missed this in my other email, in case you want to directly refer
> >> to the lifetime, you should keep it defined. I would still remove the
> >> `'s` lifetime though.
> >> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> >> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> >> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> >> 
> >> I don't see how the "not-being revoked" state makes the `data` ptr be
> >> valid. Is that an invariant of `Revocable`? (it's not documented to have
> >> any invariants)
> >
> > What else makes it valid?
> 
> IMO an `# Invariants` section with the corresponding invariant that
> `data` is valid when `is_available` is true.

Yeah, I agree that the # Invariants section is indeed missing and should be
fixed.

> > AFAICS, try_access() and try_access_with_guard() argue the exact same way,
> > except that the reason for not being revoked is the atomic check and the RCU
> > read lock.
> 
> Just because other code is doing the same mistake doesn't make it
> correct. If I had reviewed the patch at that time I'm sure I would have
> pointed this out.

I would say that try_access() and try_access_with_guard() are wrong, they rely
on the correct thing, we just missed documenting the corresponding invariant.

> I opened an issue about this:
> 
>     https://github.com/Rust-for-Linux/linux/issues/1160

Thanks for creating the issue!

What do you suggest for this patch?

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sun Apr 27, 2025 at 12:13 PM CEST, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:37:00AM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
>> > On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
>> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> >> > Implement an unsafe direct accessor for the data stored within the
>> >> > Revocable.
>> >> >
>> >> > This is useful for cases where we can proof that the data stored within
>> >> > the Revocable is not and cannot be revoked for the duration of the
>> >> > lifetime of the returned reference.
>> >> >
>> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> >> > ---
>> >> > The explicit lifetimes in access() probably don't serve a practical
>> >> > purpose, but I found them to be useful for documentation purposes.
>> >> > ---
>> >> >  rust/kernel/revocable.rs | 12 ++++++++++++
>> >> >  1 file changed, 12 insertions(+)
>> >> >
>> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> >> > index 971d0dc38d83..33535de141ce 100644
>> >> > --- a/rust/kernel/revocable.rs
>> >> > +++ b/rust/kernel/revocable.rs
>> >> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>> >> >          self.try_access().map(|t| f(&*t))
>> >> >      }
>> >> >  
>> >> > +    /// Directly access the revocable wrapped object.
>> >> > +    ///
>> >> > +    /// # Safety
>> >> > +    ///
>> >> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>> >> > +    /// for the duration of `'a`.
>> >> 
>> >> Ah I missed this in my other email, in case you want to directly refer
>> >> to the lifetime, you should keep it defined. I would still remove the
>> >> `'s` lifetime though.
>> >> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>> >> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
>> >> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
>> >> 
>> >> I don't see how the "not-being revoked" state makes the `data` ptr be
>> >> valid. Is that an invariant of `Revocable`? (it's not documented to have
>> >> any invariants)
>> >
>> > What else makes it valid?
>> 
>> IMO an `# Invariants` section with the corresponding invariant that
>> `data` is valid when `is_available` is true.
>
> Yeah, I agree that the # Invariants section is indeed missing and should be
> fixed.
>
>> > AFAICS, try_access() and try_access_with_guard() argue the exact same way,
>> > except that the reason for not being revoked is the atomic check and the RCU
>> > read lock.
>> 
>> Just because other code is doing the same mistake doesn't make it
>> correct. If I had reviewed the patch at that time I'm sure I would have
>> pointed this out.
>
> I would say that try_access() and try_access_with_guard() are wrong, they rely

Did you mean to write `wouldn't`? Otherwise the second part doesn't
match IMO.

> on the correct thing, we just missed documenting the corresponding invariant.

Yeah it's not a behavior error, but since you agree that something
should be fixed, there also is something that is 'wrong' :)

>> I opened an issue about this:
>> 
>>     https://github.com/Rust-for-Linux/linux/issues/1160
>
> Thanks for creating the issue!
>
> What do you suggest for this patch?

I don't mind if you take it with the lifetime changes, so

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

But I'd like the invariant to be documented (maybe we should tag the
issue with good-first-issue -- I don't actually think it is one, but
maybe you disagree).

---
Cheers,
Benno

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 05:15:48PM +0000, Benno Lossin wrote:
> On Sun Apr 27, 2025 at 12:13 PM CEST, Danilo Krummrich wrote:
> > On Sun, Apr 27, 2025 at 08:37:00AM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
> >> > On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
> >> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> >> > Implement an unsafe direct accessor for the data stored within the
> >> >> > Revocable.
> >> >> >
> >> >> > This is useful for cases where we can proof that the data stored within
> >> >> > the Revocable is not and cannot be revoked for the duration of the
> >> >> > lifetime of the returned reference.
> >> >> >
> >> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> >> > ---
> >> >> > The explicit lifetimes in access() probably don't serve a practical
> >> >> > purpose, but I found them to be useful for documentation purposes.
> >> >> > ---
> >> >> >  rust/kernel/revocable.rs | 12 ++++++++++++
> >> >> >  1 file changed, 12 insertions(+)
> >> >> >
> >> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> >> >> > index 971d0dc38d83..33535de141ce 100644
> >> >> > --- a/rust/kernel/revocable.rs
> >> >> > +++ b/rust/kernel/revocable.rs
> >> >> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >> >> >          self.try_access().map(|t| f(&*t))
> >> >> >      }
> >> >> >  
> >> >> > +    /// Directly access the revocable wrapped object.
> >> >> > +    ///
> >> >> > +    /// # Safety
> >> >> > +    ///
> >> >> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> >> >> > +    /// for the duration of `'a`.
> >> >> 
> >> >> Ah I missed this in my other email, in case you want to directly refer
> >> >> to the lifetime, you should keep it defined. I would still remove the
> >> >> `'s` lifetime though.
> >> >> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> >> >> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> >> >> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> >> >> 
> >> >> I don't see how the "not-being revoked" state makes the `data` ptr be
> >> >> valid. Is that an invariant of `Revocable`? (it's not documented to have
> >> >> any invariants)
> >> >
> >> > What else makes it valid?
> >> 
> >> IMO an `# Invariants` section with the corresponding invariant that
> >> `data` is valid when `is_available` is true.
> >
> > Yeah, I agree that the # Invariants section is indeed missing and should be
> > fixed.
> >
> >> > AFAICS, try_access() and try_access_with_guard() argue the exact same way,
> >> > except that the reason for not being revoked is the atomic check and the RCU
> >> > read lock.
> >> 
> >> Just because other code is doing the same mistake doesn't make it
> >> correct. If I had reviewed the patch at that time I'm sure I would have
> >> pointed this out.
> >
> > I would say that try_access() and try_access_with_guard() are wrong, they rely
> 
> Did you mean to write `wouldn't`? Otherwise the second part doesn't
> match IMO.

Yes, I meant "wouldn't". :)

> 
> > on the correct thing, we just missed documenting the corresponding invariant.
> 
> Yeah it's not a behavior error, but since you agree that something
> should be fixed, there also is something that is 'wrong' :)
> 
> >> I opened an issue about this:
> >> 
> >>     https://github.com/Rust-for-Linux/linux/issues/1160
> >
> > Thanks for creating the issue!
> >
> > What do you suggest for this patch?
> 
> I don't mind if you take it with the lifetime changes, so
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> But I'd like the invariant to be documented (maybe we should tag the
> issue with good-first-issue -- I don't actually think it is one, but
> maybe you disagree).

Yes, it should be documented; regarding the issue you created, I'd be fine
marking it as good-first-issue.

But I'd also be fine sending a fix for this myself outside the scope of this
series.