by Wedson Almeida Filho

[permalink] [raw]

Subject: Re: [PATCH v2 08/13] rust: introduce `ARef`

On Fri, 7 Apr 2023 at 18:59, Benno Lossin <[email protected]> wrote:
>
> On 05.04.23 19:51, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <[email protected]>
> >
> > This is an owned reference to an object that is always ref-counted. This
> > is meant to be used in wrappers for C types that have their own ref
> > counting functions, for example, tasks, files, inodes, dentries, etc.
> >
> > Signed-off-by: Wedson Almeida Filho <[email protected]>
> > ---
> > v1 -> v2: No changes
> >
> > rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 107 insertions(+)
> >
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index dbfae9bb97ce..b071730253c7 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
> > use alloc::boxed::Box;
> > use core::{
> > cell::UnsafeCell,
> > + marker::PhantomData,
> > mem::MaybeUninit,
> > ops::{Deref, DerefMut},
> > + ptr::NonNull,
> > };
> >
> > /// Used to transfer ownership to and from foreign (non-Rust) languages.
> > @@ -295,6 +297,111 @@ opaque_init_funcs! {
> > "Rust" manual_init4(arg1: A1, arg2: A2, arg3: A3, arg4: A4);
> > }
> >
> > +/// Types that are _always_ reference counted.
> > +///
> > +/// It allows such types to define their own custom ref increment and decrement functions.
> > +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> > +/// [`ARef<T>`].
> > +///
> > +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
> > +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> > +/// instances of a type.
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> > +/// at least until matching decrements are performed.
> > +///
> > +/// 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.)
> > +pub unsafe trait AlwaysRefCounted {
> > + /// Increments the reference count on the object.
> > + fn inc_ref(&self);
> > +
> > + /// Decrements the reference count on the object.
> > + ///
> > + /// Frees the object when the count reaches zero.
> > + ///
> > + /// # Safety
> > + ///
> > + /// Callers must ensure that there was a previous matching increment to the reference count,
> > + /// and that the object is no longer used after its reference count is decremented (as it may
> > + /// result in the object being freed), unless the caller owns another increment on the refcount
> > + /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> > + /// [`AlwaysRefCounted::dec_ref`] once).
> > + unsafe fn dec_ref(obj: NonNull<Self>);
> > +}
> > +
> > +/// An owned reference to an always-reference-counted object.
> > +///
> > +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> > +/// dropped. It is also automatically incremented when a new instance is created via
> > +/// [`ARef::clone`].
> > +///
> > +/// # Invariants
> > +///
> > +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> > +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> > +pub struct ARef<T: AlwaysRefCounted> {
> > + ptr: NonNull<T>,
> > + _p: PhantomData<T>,
> > +}
> > +
> > +impl<T: AlwaysRefCounted> ARef<T> {
> > + /// Creates a new instance of [`ARef`].
> > + ///
> > + /// It takes over an increment of the reference count on the underlying object.
> > + ///
> > + /// # Safety
> > + ///
> > + /// 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`].
>
> I think you should also mention that the pointee at `ptr` must live
> at least until this `ARef` decides to decrement the refcount.
> Otherwise I would interpret the docs as written to allow you to give
> a pointer to the stack and then free the backing storage and then
> continue to use the `ARef`.

Note the bound `T: AlwaysRefCounted`. `AlwaysRefCounted` is unsafe and
has the following requirements:

// Implementers must ensure that increments to the reference count
keep the object alive in memory
// at least until matching decrements are performed.
///
/// 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.)

The latter guarantees that _all_ instances are refcounted. While the
former guarantees that you can only free an instance when the refcount
goes to zero.

The safety requirement of the function ensures that the caller is
donating an increment to `ARef`.

So it's ok to allocate from the stack. But you cannot free it
immediately without violating some promise you must abide by for
safety.

>
> --
> Cheers,
> Benno
>
> > + pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > + // INVARIANT: The safety requirements guarantee that the new instance now owns the
> > + // increment on the refcount.
> > + Self {
> > + ptr,
> > + _p: PhantomData,
> > + }
> > + }
> > +}
> > +
> > +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> > + fn clone(&self) -> Self {
> > + self.inc_ref();
> > + // SAFETY: We just incremented the refcount above.
> > + unsafe { Self::from_raw(self.ptr) }
> > + }
> > +}
> > +
> > +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> > + type Target = T;
> > +
> > + fn deref(&self) -> &Self::Target {
> > + // SAFETY: The type invariants guarantee that the object is valid.
> > + unsafe { self.ptr.as_ref() }
> > + }
> > +}
> > +
> > +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> > + fn from(b: &T) -> Self {
> > + b.inc_ref();
> > + // SAFETY: We just incremented the refcount above.
> > + unsafe { Self::from_raw(NonNull::from(b)) }
> > + }
> > +}
> > +
> > +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> > + fn drop(&mut self) {
> > + // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> > + // decrement.
> > + unsafe { T::dec_ref(self.ptr) };
> > + }
> > +}
> > +
> > /// A sum type that always holds either a value of type `L` or `R`.
> > pub enum Either<L, R> {
> > /// Constructs an instance of [`Either`] containing a value of type `L`.
> > --
> > 2.34.1
> >
>

2023-04-08 07:55:00

by y86-dev

[permalink] [raw]

Subject: Re: [PATCH v2 13/13] rust: sync: introduce `LockedBy`

On 08.04.23 06:11, Wedson Almeida Filho wrote:
> On Fri, 7 Apr 2023 at 18:00, Benno Lossin <[email protected]> wrote:
>>
>> On 05.04.23 19:51, Wedson Almeida Filho wrote:
>>> From: Wedson Almeida Filho <[email protected]>
>>>
>>> This allows us to have data protected by a lock despite not being
>>> wrapped by it. Access is granted by providing evidence that the lock is
>>> held by the caller.
>>>
>>> Signed-off-by: Wedson Almeida Filho <[email protected]>
>>> ---
>>> v1 -> v2: Added build_assert to rule out zero-sized types
>>>
>>> rust/kernel/sync.rs | 2 +
>>> rust/kernel/sync/lock.rs | 2 +-
>>> rust/kernel/sync/locked_by.rs | 128 ++++++++++++++++++++++++++++++++++
>>> 3 files changed, 131 insertions(+), 1 deletion(-)
>>> create mode 100644 rust/kernel/sync/locked_by.rs
>>>
>>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>>> index d6dd0e2c1678..f8edb6d0d794 100644
>>> --- a/rust/kernel/sync.rs
>>> +++ b/rust/kernel/sync.rs
>>> @@ -10,10 +10,12 @@ use crate::types::Opaque;
>>> mod arc;
>>> mod condvar;
>>> pub mod lock;
>>> +mod locked_by;
>>>
>>> pub use arc::{Arc, ArcBorrow, UniqueArc};
>>> pub use condvar::CondVar;
>>> pub use lock::{mutex::Mutex, spinlock::SpinLock};
>>> +pub use locked_by::LockedBy;
>>>
>>> /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
>>> #[repr(transparent)]
>>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>>> index f52ba9ab1b70..51c996ca2109 100644
>>> --- a/rust/kernel/sync/lock.rs
>>> +++ b/rust/kernel/sync/lock.rs
>>> @@ -111,7 +111,7 @@ pub struct Lock<T: ?Sized, B: Backend> {
>>> _pin: PhantomPinned,
>>>
>>> /// The data protected by the lock.
>>> - data: UnsafeCell<T>,
>>> + pub(crate) data: UnsafeCell<T>,
>>> }
>>>
>>> // SAFETY: `Lock` can be transferred across thread boundaries iff the data it protects can.
>>> diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs
>>> new file mode 100644
>>> index 000000000000..191e37d804e5
>>> --- /dev/null
>>> +++ b/rust/kernel/sync/locked_by.rs
>>> @@ -0,0 +1,128 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! A wrapper for data protected by a lock that does not wrap it.
>>> +
>>> +use super::{lock::Backend, lock::Lock};
>>> +use core::{cell::UnsafeCell, ptr};
>>> +
>>> +/// Allows access to some data to be serialised by a lock that does not wrap it.
>>> +///
>>> +/// In most cases, data protected by a lock is wrapped by the appropriate lock type, e.g.,
>>> +/// [`super::Mutex`] or [`super::SpinLock`]. [`LockedBy`] is meant for cases when this is not
>>> +/// possible. For example, if a container has a lock and some data in the contained elements needs
>>> +/// to be protected by the same lock.
>>> +///
>>> +/// [`LockedBy`] wraps the data in lieu of another locking primitive, and only allows access to it
>>> +/// when the caller shows evidence that the 'external' lock is locked.
>>> +///
>>
>> Maybe add that the `access`/`access_mut` functions panic when the supplied
>> external value is not the correct one.
>
> Will do for v3.
>
>>> +/// # Examples
>>> +///
>>> +/// The following is an example for illustrative purposes: `InnerDirectory::bytes_used` is an
>>> +/// aggregate of all `InnerFile::bytes_used` and must be kept consistent; so we wrap `InnerFile` in
>>> +/// a `LockedBy` so that it shares a lock with `InnerDirectory`. This allows us to enforce at
>>> +/// compile-time that access to `InnerFile` is only granted when an `InnerDirectory` is also
>>> +/// locked; we enforce at run time that the right `InnerDirectory` is locked.
>>> +///
>>> +/// ```
>>> +/// use kernel::sync::{LockedBy, Mutex};
>>> +///
>>> +/// struct InnerFile {
>>> +/// bytes_used: u64,
>>> +/// }
>>> +///
>>> +/// struct File {
>>> +/// _ino: u32,
>>> +/// inner: LockedBy<InnerFile, InnerDirectory>,
>>> +/// }
>>> +///
>>> +/// struct InnerDirectory {
>>> +/// /// The sum of the bytes used by all files.
>>> +/// bytes_used: u64,
>>> +/// _files: Vec<File>,
>>> +/// }
>>> +///
>>> +/// struct Directory {
>>> +/// _ino: u32,
>>> +/// inner: Mutex<InnerDirectory>,
>>> +/// }
>>> +///
>>> +/// /// Prints `bytes_used` from both the directory and file.
>>> +/// fn print_bytes_used(dir: &Directory, file: &File) {
>>> +/// let guard = dir.inner.lock();
>>> +/// let inner_file = file.inner.access(&guard);
>>> +/// pr_info!("{} {}", guard.bytes_used, inner_file.bytes_used);
>>> +/// }
>>> +///
>>> +/// /// Increments `bytes_used` for both the directory and file.
>>> +/// fn inc_bytes_used(dir: &Directory, file: &File) {
>>> +/// let mut guard = dir.inner.lock();
>>> +/// guard.bytes_used += 10;
>>> +///
>>> +/// let file_inner = file.inner.access_mut(&mut guard);
>>> +/// file_inner.bytes_used += 10;
>>> +/// }
>>> +///
>>> +/// /// Creates a new file.
>>> +/// fn new_file(ino: u32, dir: &Directory) -> File {
>>> +/// File {
>>> +/// _ino: ino,
>>> +/// inner: LockedBy::new(&dir.inner, InnerFile { bytes_used: 0 }),
>>> +/// }
>>> +/// }
>>> +/// ```
>>> +pub struct LockedBy<T: ?Sized, U: ?Sized> {
>>> + owner: *const U,
>>> + data: UnsafeCell<T>,
>>> +}
>>> +
>>> +// SAFETY: `LockedBy` can be transferred across thread boundaries iff the data it protects can.
>>> +unsafe impl<T: ?Sized + Send, U: ?Sized> Send for LockedBy<T, U> {}
>>> +
>>> +// SAFETY: `LockedBy` serialises the interior mutability it provides, so it is `Sync` as long as the
>>> +// data it protects is `Send`.
>>> +unsafe impl<T: ?Sized + Send, U: ?Sized> Sync for LockedBy<T, U> {}
>>> +
>>> +impl<T, U: ?Sized> LockedBy<T, U> {
>>> + /// Constructs a new instance of [`LockedBy`].
>>> + ///
>>> + /// It stores a raw pointer to the owner that is never dereferenced. It is only used to ensure
>>> + /// that the right owner is being used to access the protected data. If the owner is freed, the
>>> + /// data becomes inaccessible; if another instance of the owner is allocated *on the same
>>> + /// memory location*, the data becomes accessible again: none of this affects memory safety
>>> + /// because in any case at most one thread (or CPU) can access the protected data at a time.
>>> + pub fn new(owner: &Lock<U, impl Backend>, data: T) -> Self {
>>
>> I think it would be sensible to also do the ZST check here, then it will
>> fail immediately on construction (but also keep the other location, as it
>> does not add any runtime cost).
>
> This ZST restriction on `U` applies to `access` and `access_mut`, but
> doesn't apply to versions of these functions that work on `Guard`
> instances, which we have in the `rust` branch but not here yet. (The
> restrictions there are different.) So it doesn't make sense to block
> the _creation_ of `LockedBy` instances if `access`/`access_mut` are
> never used.

Ah because you can only get a `Guard` safely via a `Lock` and at
the moment, all locks cannot be zero-sized even if the data is.

But if we at some point decide to add a `Backend` with the
`State` being a ZST, then this code becomes unsound.

What about adding a `build_assert!(mem::size_of::<Lock<T, B>>() > 0)`
(you will sadly have to spell out the generic parameter of `Backend`).
I worry that we might not remember all of these details when
we add new locks.

--
Cheers,
Benno

>> Also, I think you should mention in the documentation that ZSTs are not
>> supported. And it would be good to have an explaining comment on the
>> `build_assert!` why we disallow ZSTs here.
>
> I'll add comments in v3 to access/access_mut.
>
>>> + Self {
>>> + owner: owner.data.get(),
>>> + data: UnsafeCell::new(data),
>>> + }
>>> + }
>>> +}
>>> +
>>> +impl<T: ?Sized, U> LockedBy<T, U> {
>>> + /// Returns a reference to the protected data when the caller provides evidence (via a
>>> + /// reference) that the owner is locked.
>>
>> Maybe add a `# Panic` section, also for `access_mut`.
>>
>> --
>> Cheers,
>> Benno
>>
>>> + pub fn access<'a>(&'a self, owner: &'a U) -> &'a T {
>>> + crate::build_assert!(core::mem::size_of::() > 0);
>>> + if !ptr::eq(owner, self.owner) {
>>> + panic!("mismatched owners");
>>> + }
>>> +
>>> + // SAFETY: `owner` is evidence that the owner is locked.
>>> + unsafe { &*self.data.get() }
>>> + }
>>> +
>>> + /// Returns a mutable reference to the protected data when the caller provides evidence (via a
>>> + /// mutable owner) that the owner is locked mutably.
>>> + ///
>>> + /// Showing a mutable reference to the owner is sufficient because we know no other references
>>> + /// can exist to it.
>>> + pub fn access_mut<'a>(&'a self, owner: &'a mut U) -> &'a mut T {
>>> + crate::build_assert!(core::mem::size_of::() > 0);
>>> + if !ptr::eq(owner, self.owner) {
>>> + panic!("mismatched owners");
>>> + }
>>> +
>>> + // SAFETY: `owner` is evidence that there is only one reference to the owner.
>>> + unsafe { &mut *self.data.get() }
>>> + }
>>> +}
>>> --
>>> 2.34.1
>>>
>>