[v2,13/13] rust: sync: introduce `LockedBy`
Commit Message
From: Wedson Almeida Filho <walmeida@microsoft.com>
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 <walmeida@microsoft.com>
---
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
Comments
On 05.04.23 19:51, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> 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 <walmeida@microsoft.com>
> ---
> 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.
> +/// # 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).
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.
> + 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::<U>() > 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::<U>() > 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
>
On Fri, 7 Apr 2023 at 18:00, Benno Lossin <y86-dev@protonmail.com> wrote:
>
> On 05.04.23 19:51, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > 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 <walmeida@microsoft.com>
> > ---
> > 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.
> 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::<U>() > 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::<U>() > 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
> >
>
On 08.04.23 06:11, Wedson Almeida Filho wrote:
> On Fri, 7 Apr 2023 at 18:00, Benno Lossin <y86-dev@protonmail.com> wrote:
>>
>> On 05.04.23 19:51, Wedson Almeida Filho wrote:
>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>>
>>> 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 <walmeida@microsoft.com>
>>> ---
>>> 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::<U>() > 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::<U>() > 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
>>>
>>
@@ -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)]
@@ -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.
new file mode 100644
@@ -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.
+///
+/// # 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 {
+ 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.
+ pub fn access<'a>(&'a self, owner: &'a U) -> &'a T {
+ crate::build_assert!(core::mem::size_of::<U>() > 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::<U>() > 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() }
+ }
+}