linux-kernel - Re: [PATCH v9 3/5] rust: Add missing SAFETY documentation for ARef example

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o6wbz46a.fsf@kernel.org>
Date: Fri, 02 May 2025 12:40:29 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Oliver Mangold <oliver.mangold@...me>
Cc: Miguel Ojeda <ojeda@...nel.org>,  Alex Gaynor <alex.gaynor@...il.com>,
  Boqun Feng <boqun.feng@...il.com>,  Gary Guo <gary@...yguo.net>,
  Björn
 Roy Baron <bjorn3_gh@...tonmail.com>,  Benno Lossin
 <benno.lossin@...ton.me>,  Alice Ryhl <aliceryhl@...gle.com>,  Trevor
 Gross <tmgross@...ch.edu>,  Asahi Lina <lina@...hilina.net>,
  rust-for-linux@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 3/5] rust: Add missing SAFETY documentation for ARef
 example

Oliver Mangold <oliver.mangold@...me> writes:

> On 250409 1126, Andreas Hindborg wrote:
>> "Oliver Mangold" <oliver.mangold@...me> writes:
>> 
>> > SAFETY comment in rustdoc example was just 'TODO'. Fixed.
>> >
>> > Signed-off-by: Oliver Mangold <oliver.mangold@...me>
>> > ---
>> >  rust/kernel/types.rs | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> > index c8b78bcad259132808cc38c56b9f2bd525a0b755..db29f7c725e631c11099fa9122901ec2b3f4a039 100644
>> > --- a/rust/kernel/types.rs
>> > +++ b/rust/kernel/types.rs
>> > @@ -492,7 +492,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>> >      ///
>> >      /// struct Empty {}
>> >      ///
>> > -    /// # // SAFETY: TODO.
>> > +    /// // SAFETY: We do not free anything.
>> 
>> How about:
>> 
>>   This implementation will never free the underlying object, so the
>>   object is kept alive longer than the safety requirement specifies.
>
> Yes, was rather sloppy wording. Thanks, I will use your version.
>
>> >      /// unsafe impl RefCounted for Empty {
>> >      ///     fn inc_ref(&self) {}
>> >      ///     unsafe fn dec_ref(_obj: NonNull<Self>) {}
>> > @@ -500,7 +500,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>> >      ///
>> >      /// let mut data = Empty {};
>> >      /// let ptr = NonNull::<Empty>::new(&mut data).unwrap();
>> > -    /// # // SAFETY: TODO.
>> > +    /// // SAFETY: We keep `data` around longer than the `ARef`.
>> 
>> This is not sufficient. The safety requirement is:
>> 
>>   Callers must ensure that the reference count was incremented at least once, and that they
>>   are properly relinquishing one increment. That is, if there is only one increment, callers
>>   must not use the underlying object anymore -- it is only safe to do so via the newly
>>   created [`ARef`].
>> 
>> How about:
>> 
>>   The `RefCounted` implementation for `Empty` does not count references
>>   and never frees the underlying object. Thus we can act as having a
>>   refcount on the object that we pass to the newly created `ARef`.
>> 
>> I think this example actually exposes a soundness hole. When the
>> underlying object is allocated on the stack, the safety requirements are
>> not sufficient to ensure the lifetime of the object. We could safely
>> return `data_ref` and have the underlying object go away. We should add
>> to the safety requirement of `ARef::from_raw`:
>> 
>>   `ptr` must be valid while the refcount is positive.
>
> Wouldn't this already be covered by the below in the doc for
> AlwaysRefCounted?
>
>     Implementers must ensure that increments to the reference count keep
>     the object alive in memory at least until matching decrements are
>     performed."

No, I don't think that is enough. We can implement the trait properly
with refcounts and still we can allocate an object on the stack and then
do a `from_raw` on that object without violating any safety
requirements. I think the `ARef::from_raw` should have the safety
requirement above. But we can do that as a separate patch.

>
> OTOH, it also says this (which would be violated):
>
>     Implementers must also ensure that all instances are reference-counted.
>     (Otherwise they won’t be able to honour the requirement that
>     AlwaysRefCounted::inc_ref keep the object alive.)"
>
> Should I change the example to one with an actual reference count?
> Not sure, would make it more complex and less readable, of course.

No I think that is fine. `Empty` is reference counted in the sense that
the refcount can considered to always be positive.



Best regards,
Andreas Hindborg