[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87cycrz1pa.fsf@kernel.org>
Date: Fri, 02 May 2025 13:33:53 +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
Andreas Hindborg <a.hindborg@...nel.org> writes:
> 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.
On second thought, I think you are right. I was trying to implement a
counter example, and I think it is not possible to implement
`RefCounted` while following the safety requirements in a way that would
trigger this issue. Implementers will have to make sure that the the
type that implement `RefCounted` cannot be directly constructed. In
other words the implementation for `Empty` is illegal in this regard.
We can do this instead for the example
mod empty {
use crate::alloc::KBox;
use core::ptr::NonNull;
pub struct Empty {
// Prevent direct construction
_p: (),
}
// SAFETY: 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`.
unsafe impl super::RefCounted for Empty {
fn inc_ref(&self) {}
unsafe fn dec_ref(_obj: NonNull<Self>) {}
}
impl Empty {
pub fn new() -> NonNull<Self> {
NonNull::new(KBox::into_raw(
KBox::new(Self { _p: () }, kernel::alloc::flags::GFP_KERNEL).unwrap(),
))
.unwrap()
}
}
}
let ptr = empty::Empty::new();
// SAFETY: 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`.
let data_ref: ARef<empty::Empty> = unsafe { ARef::from_raw(ptr) };
let _raw_ptr: NonNull<empty::Empty> = ARef::into_raw(data_ref);
Best regards,
Andreas Hindborg
Powered by blists - more mailing lists