On Wed, Nov 20, 2024 at 7:13 PM Lorenzo Stoakes
<lorenzo.stoakes@xxxxxxxxxx> wrote:
>
> On Wed, Nov 20, 2024 at 02:49:55PM +0000, Alice Ryhl wrote:
> > These abstractions allow you to reference a `struct mm_struct` using
> > both mmgrab and mmget refcounts. This is done using two Rust types:
> >
> > * Mm - represents an mm_struct where you don't know anything about the
> > value of mm_users.
> > * MmWithUser - represents an mm_struct where you know at compile time
> > that mm_users is non-zero.
> >
> > This allows us to encode in the type system whether a method requires
> > that mm_users is non-zero or not. For instance, you can always call
> > `mmget_not_zero` but you can only call `mmap_read_lock` when mm_users is
> > non-zero.
>
> It's kind of interesting to represent these things this way, I like the
> self-documenting element of that.
>
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
>
> So obviously I'm not a rust person (yet... yet :) so from my side I can
> only look at things from an mm perspective conceptually. To avoid boring
> everyone I won't repeat this and instead you can take it as read.
>
> I will obviously inevitably ask a TON of questions as a result of not being
> a rust person so, bear with me...
>
> > ---
> > rust/helpers/helpers.c | 1 +
> > rust/helpers/mm.c | 39 +++++++++
> > rust/kernel/lib.rs | 1 +
> > rust/kernel/mm.rs | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 260 insertions(+)
> >
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 20a0c69d5cc7..60a488eb4efe 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -13,6 +13,7 @@
> > #include "build_bug.c"
> > #include "err.c"
> > #include "kunit.c"
> > +#include "mm.c"
> > #include "mutex.c"
> > #include "page.c"
> > #include "rbtree.c"
> > diff --git a/rust/helpers/mm.c b/rust/helpers/mm.c
> > new file mode 100644
> > index 000000000000..7201747a5d31
> > --- /dev/null
> > +++ b/rust/helpers/mm.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/mm.h>
> > +#include <linux/sched/mm.h>
> > +
> > +void rust_helper_mmgrab(struct mm_struct *mm)
> > +{
> > + mmgrab(mm);
> > +}
> > +
> > +void rust_helper_mmdrop(struct mm_struct *mm)
> > +{
> > + mmdrop(mm);
> > +}
> > +
> > +void rust_helper_mmget(struct mm_struct *mm)
> > +{
> > + mmget(mm);
> > +}
> > +
> > +bool rust_helper_mmget_not_zero(struct mm_struct *mm)
> > +{
> > + return mmget_not_zero(mm);
> > +}
> > +
> > +void rust_helper_mmap_read_lock(struct mm_struct *mm)
> > +{
> > + mmap_read_lock(mm);
> > +}
> > +
> > +bool rust_helper_mmap_read_trylock(struct mm_struct *mm)
> > +{
> > + return mmap_read_trylock(mm);
> > +}
> > +
> > +void rust_helper_mmap_read_unlock(struct mm_struct *mm)
> > +{
> > + mmap_read_unlock(mm);
> > +}
>
> I guess at this point we're only interested in reading?
Yeah. The write lock would be very similar.
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 66f5cde7f322..cc1963510cdf 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -43,6 +43,7 @@
> > pub mod kunit;
> > pub mod list;
> > pub mod miscdevice;
> > +pub mod mm;
> > #[cfg(CONFIG_NET)]
> > pub mod net;
> > pub mod page;
> > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> > new file mode 100644
> > index 000000000000..84cba581edaa
> > --- /dev/null
> > +++ b/rust/kernel/mm.rs
> > @@ -0,0 +1,219 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2024 Google LLC.
> > +
> > +//! Memory management.
> > +//!
> > +//! C header: [`include/linux/mm.h`](srctree/include/linux/mm.h)
> > +
> > +use crate::{
> > + bindings,
> > + types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
> > +};
> > +use core::{ops::Deref, ptr::NonNull};
> > +
> > +/// A wrapper for the kernel's `struct mm_struct`.
> > +///
> > +/// Since `mm_users` may be zero, the associated address space may not exist anymore. You can use
> > +/// [`mmget_not_zero`] to be able to access the address space.
> > +///
> > +/// The `ARef<Mm>` smart pointer holds an `mmgrab` refcount. Its destructor may sleep.
> > +///
> > +/// # Invariants
> > +///
> > +/// Values of this type are always refcounted using `mmgrab`.
> > +///
> > +/// [`mmget_not_zero`]: Mm::mmget_not_zero
> > +#[repr(transparent)]
> > +pub struct Mm {
> > + mm: Opaque<bindings::mm_struct>,
> > +}
>
> Does this tie this type to the C struct mm_struct type?
>
> Does 'Opaque' mean it is a pointer to a type which is 'opaque' in the sense
> that rust can't see into its internals?
This declaration defines a Rust type called Mm which has the same
size, alignment, and contents as `struct mm_struct`. The purpose of
Opaque is to tell Rust that it can't assume anything about the
contents at all; we do that to leave it up to C.
For example, normally if you have an immutable reference &i32, then
Rust is going to assume that the contents behind the reference are in
fact immutable. Opaque turns that off, meaning that an `&Opaque<i32>`
is allowed to reference an integer as it gets modified. It makes all
access to the contents unsafe, though.
Note that Opaque is *not* a pointer type. We're going to be dealing
with types like &Mm or ARef<Mm> where &_ and ARef<_> are two different
kinds of pointers.
> > +// SAFETY: It is safe to call `mmdrop` on another thread than where `mmgrab` was called.
> > +unsafe impl Send for Mm {}
> > +// SAFETY: All methods on `Mm` can be called in parallel from several threads.
> > +unsafe impl Sync for Mm {}
> > +
> > +// SAFETY: By the type invariants, this type is always refcounted.
> > +unsafe impl AlwaysRefCounted for Mm {
> > + #[inline]
> > + fn inc_ref(&self) {
> > + // SAFETY: The pointer is valid since self is a reference.
> > + unsafe { bindings::mmgrab(self.as_raw()) };
> > + }
> > +
> > + #[inline]
> > + unsafe fn dec_ref(obj: NonNull<Self>) {
> > + // SAFETY: The caller is giving up their refcount.
> > + unsafe { bindings::mmdrop(obj.cast().as_ptr()) };
> > + }
> > +}
>
> Under what circumstances would these be taken? Same question for MmWthUser.
This makes `Mm` compatible with the pointer type called ARef<_>. This
pointer type is used to represent ownership of a refcount. So whenever
a variable of type ARef<_> goes out of scope, the refcount is
decremented, and whenever an ARef<_> is cloned, the refcount is
incremented.
The way this works is that ARef<_> is implemented to use the
AlwaysRefCounted trait to understand how to manipulate the count. Only
types that implement the trait with an impl block like above can be
used with ARef<_>.
> > +
> > +/// A wrapper for the kernel's `struct mm_struct`.
> > +///
> > +/// This type is like [`Mm`], but with non-zero `mm_users`. It can only be used when `mm_users` can
> > +/// be proven to be non-zero at compile-time, usually because the relevant code holds an `mmget`
> > +/// refcount. It can be used to access the associated address space.
> > +///
> > +/// The `ARef<MmWithUser>` smart pointer holds an `mmget` refcount. Its destructor may sleep.
> > +///
> > +/// # Invariants
> > +///
> > +/// Values of this type are always refcounted using `mmget`. The value of `mm_users` is non-zero.
> > +#[repr(transparent)]
> > +pub struct MmWithUser {
> > + mm: Mm,
> > +}
>
> Why does Mm have this as a Opaque<bindings::mm_struct> and this sort of
> nests it?
>
> Does this somehow amount to the same thing, or would you probably never
> actually reference this mm field?
It amounts to the same thing as Opaque<bindings::mm_struct>.
> > +// SAFETY: It is safe to call `mmput` on another thread than where `mmget` was called.
> > +unsafe impl Send for MmWithUser {}
> > +// SAFETY: All methods on `MmWithUser` can be called in parallel from several threads.
> > +unsafe impl Sync for MmWithUser {}
> > +
> > +// SAFETY: By the type invariants, this type is always refcounted.
> > +unsafe impl AlwaysRefCounted for MmWithUser {
> > + #[inline]
> > + fn inc_ref(&self) {
> > + // SAFETY: The pointer is valid since self is a reference.
> > + unsafe { bindings::mmget(self.as_raw()) };
> > + }
> > +
> > + #[inline]
> > + unsafe fn dec_ref(obj: NonNull<Self>) {
> > + // SAFETY: The caller is giving up their refcount.
> > + unsafe { bindings::mmput(obj.cast().as_ptr()) };
> > + }
>
> Hm, why is it we mmget(self.as_raw()) but mmput(obj.cast().as_ptr())?
There's one assumption about references that Opaque doesn't turn off:
The memory behind the reference must not get deallocated while the
reference exists. We can't use a reference in dec_ref because the
memory might get deallocated during the call to dec_ref.
> Also I guess relatedly, why does one refer to &self and the other as a
> NonNull<Self>?
Writing `&self` means that the "self parameter" has type `&Self`,
which in this case is the same as `&MmWithUser`.
The type `NonNull<Self>` is the same as `NonNull<MmWithUser>`. The
NonNull type is a raw pointer that can't be null. Other than the
non-null requirement, nothing is assumed about the raw pointer.
> I'm guessing as a non-rust person a 'reference' is like a C++ reference in
> the sense that (well it is _assumed_ in C++ anyway) it acts like a pointer
> for the type which should never not be there, but we need .as_raw() to get
> the raw C pointer?
Yeah, Rust references come with an assumption that the object is not
deallocated while the reference exists.
The .as_raw() call converts from &MmWithUser to `*mut
bindings::mm_struct`. So note that it not only converts from reference
to raw pointer, but it also changes the target type from MmWithUser to
bindings::mm_struct.
> And I guess in the dec_ref() case we need the .cast().as_ptr() because obj
> 'boxes' around self (I guess equivalent to 'this' in C++ kinda)
> guaranteeing that it can provide non-null pointer to the current object?
Well, the thing that is equivalent to "this" would be "self".
> > +// Make all `Mm` methods available on `MmWithUser`.
> > +impl Deref for MmWithUser {
> > + type Target = Mm;
> > +
> > + #[inline]
> > + fn deref(&self) -> &Mm {
> > + &self.mm
> > + }
> > +}
>
> I rubber ducked myself a bit on this, so I guess this makes it possible to
> dereference the object, and it
It lets you transparently obtain an &Mm from an &MmWithUser, making
all Mm methods available on MmWithUser.
> > +// These methods are safe to call even if `mm_users` is zero.
> > +impl Mm {
> > + /// Call `mmgrab` on `current.mm`.
> > + #[inline]
> > + pub fn mmgrab_current() -> Option<ARef<Mm>> {
> > + // SAFETY: It's safe to get the `mm` field from current.
> > + let mm = unsafe {
> > + let current = bindings::get_current();
> > + (*current).mm
> > + };
>
> I don't see an equivalent means of obtaining mm from current for
> MmWithUser, is that intentional, would there be another means of obtaining
> an mm? (perhaps via vma->vm_mm I guess?)
>
> An aside -->
>
> If we're grabbing from current, and this is non-NULL (i.e. not a kernel
> thread), this is kinda MmWithUser to _start out_ with, but I guess if
> you're grabbing the current one you might not expect it.
>
> I guess one thing I want to point out (maybe here is wrong place) is that
> the usual way of interacting with current->mm is that we do _not_ increment
> mm->mm_count, mm->mm_users or any refernce count, as while we are in the
> kernel portion of the task, we are guaranteed the mm and the backing
> virtual address space will stick around.
>
> With reference to MmWithUser, in fact, if you look up users of
> mmget()/mmput() it is pretty rare to do that.
>
> So ideally we'd avoid doing this if we could (though having the semantics
> of grabbing a reference if we were to copy the object somehow again or hold
> its state or something would be nice).
>
> I guess this might actually be tricky in rust, because we'd probably need
> to somehow express the current task's lifetime and tie this to that
> and... yeah.
>
> <-- aside
Ah, yeah, I guess this API is incomplete. We could have an API that
lets you obtain an &MmWithUser instead. Then, if the user wants to
increment the refcount, they can manually convert that into an
ARef<Mm> or ARef<MmWithUser>.
It's true that it's slightly tricky to express in Rust, but it's
possible. We already have a way to get a &Task pointing at current.
> > +
> > + if mm.is_null() {
> > + return None;
> > + }
> > +
> > + // SAFETY: The value of `current->mm` is guaranteed to be null or a valid `mm_struct`. We
> > + // just checked that it's not null. Furthermore, the returned `&Mm` is valid only for the
> > + // duration of this function, and `current->mm` will stay valid for that long.
> > + let mm = unsafe { Mm::from_raw(mm) };
>
> Hm does mm now reference something with a different type, as in before it
> was a 'raw' pointer or such, and now it's a reference to an Mm right?
Yes ... this is using shadowing to change the type of the variable.
It's actually rather common in Rust.
The former mm variable had type `*mut bindings::mm_struct`. The latter
mm variable has type `&Mm`.
> Also I guess the 'duration of this function' is because we put this in the
> 'Aref' smart pointer which kinda takes over the lifetime of the reference
> by wrapping it right?
Yeah, basically, the lifetime gets inferred from how we use it.
> I mean I'm not a rust person so actually I have no business _commenting_ on
> this :P as this may very well be idiomatic rust, but I'm just curious about
> this.
>
> It's nitty but I feel like maybe we're somewhat overloading 'mm's here a
> bit though? As we have our wrapped Mm type and then an internal raw mm
> type. On the other hand, it's hard to now have horribly awkward and
> confusing naming here I guess, and perhaps this is fine.
>
> Not a big deal though!
>
> > +
> > + // This increments the refcount using `mmgrab`.
> > + Some(ARef::from(mm))
>
> So I get that Some() means this is like a discriminated union or such,
> where we can return None (as above) or Some, which contains the value is of
> type ARef<Mm>. And I guess this moves the 'lifetime' of mm which was
> previously with the function into that of the ARef<>?
Yes, Some() is normally a discriminated union, but in this particular
case since ARef uses a NonNull pointer for its only field, the
compiler is smart enough to represent None as a null pointer instead
of using a discriminated union with a separate tag.
As for the "lifetime" of the `mm`, that's not really the lifetime of
the mm_struct. Rather, it's the duration for which the &Mm reference
is assumed to be valid for. That lifetime ends right after the
ARef::from call, because that's our last use of the &Mm.
> Does the ARef<> 'just know' to use the AlwaysRefCounted methods?
Yep! See e.g. the destructor of ARef:
impl<T: AlwaysRefCounted> Drop for ARef<T> {
fn drop(&mut self) {
unsafe { T::dec_ref(self.ptr) };
}
}
Due to the `T: AlwaysRefCounted`, the type `T` must be a type that
implements the `AlwaysRefCounted` trait, and the compiler is able to
resolve T::dec_ref based on that.
> > + }
> > +
> > + /// Returns a raw pointer to the inner `mm_struct`.
> > + #[inline]
> > + pub fn as_raw(&self) -> *mut bindings::mm_struct {
> > + self.mm.get()
> > + }
>
> I guess this .get() method is on the Opaque<> object and returns a raw ptr?
Yes, it just creates a raw pointer.
> > + /// Obtain a reference from a raw pointer.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The caller must ensure that `ptr` points at an `mm_struct`, and that it is not deallocated
> > + /// during the lifetime 'a.
> > + #[inline]
> > + pub unsafe fn from_raw<'a>(ptr: *const bindings::mm_struct) -> &'a Mm {
>
> I'm guessing this funny 'a syntax, based on the comment, refers to the
> lifetime of the object?
It's a lifetime, but not necessarily the lifetime of the object.
Rather, it's the maximum duration in which the reference is assumed to
be valid. It must not be longer than the lifetime of the mm_struct of
course, but it's usually going to be much shorter than the lifetime of
the mm_struct.
> > + // SAFETY: Caller promises that the pointer is valid for 'a. Layouts are compatible due to
> > + // repr(transparent).
>
> God I love these SAFETY comments (I mean actually, sorry I realise it's
> almost impossible to convey 'not sarcastically, actually' in text form
> :). Is that something that gets parsed somewhere, or a convention or?
>
> I like that there is a discipline of expressing under what circumstances we
> are permitted to reference things.
They don't get parsed anywhere, except that not using a SAFETY comment
at all is a compilation warning.
> > + unsafe { &*ptr.cast() }
> > + }
> > +
> > + /// Calls `mmget_not_zero` and returns a handle if it succeeds.
> > + #[inline]
> > + pub fn mmget_not_zero(&self) -> Option<ARef<MmWithUser>> {
>
> I actually kinda love that this takes an mm and guarantees that you get an
> MmWithUser out of it which is implied by the fact this succeeds.
>
> However as to the point above, I'm concerned that this might be seen as
> 'the way' to access an mm, i.e. mm.mmgrab_current().mmget_not_zero() or
> something.
>
> Whereas, the usual way of referencing current->mm is to not increment any
> reference counts at all (assuming what you are doing resides in the same
> lifetime as the task).
>
> Obviously if you step outside of that lifetime, then you _do_ have to pin
> the mm (nearly always you want to grab rather than get though in that
> circumstance).
I can add a way to obtain an &MmWithUser from current without
incrementing the refcount.
> > + // SAFETY: The pointer is valid since self is a reference.
> > + let success = unsafe { bindings::mmget_not_zero(self.as_raw()) };
> > +
> > + if success {
> > + // SAFETY: We just created an `mmget` refcount.
> > + Some(unsafe { ARef::from_raw(NonNull::new_unchecked(self.as_raw().cast())) })
>
> When you do this, does it cause the reference count to increment, or does
> it assume it's already at 1?
This uses `from_raw` which by convention never increments the
refcount. Semantically we're taking ownership of the increment
performed by bindings::mmget_not_zero.
> > + } else {
> > + None
> > + }
> > + }
> > +}
> > +
> > +// These methods require `mm_users` to be non-zero.
> > +impl MmWithUser {
> > + /// Obtain a reference from a raw pointer.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The caller must ensure that `ptr` points at an `mm_struct`, and that `mm_users` remains
> > + /// non-zero for the duration of the lifetime 'a.
> > + #[inline]
> > + pub unsafe fn from_raw<'a>(ptr: *const bindings::mm_struct) -> &'a MmWithUser {
> > + // SAFETY: Caller promises that the pointer is valid for 'a. The layout is compatible due
> > + // to repr(transparent).
> > + unsafe { &*ptr.cast() }
> > + }
>
> I guess this is another means by which you can get the mm. I'd say an
> equivalent for getting from current is highly relevant.
This lets you write MmWithUser::from_raw in unsafe code.
> > +
> > + /// Lock the mmap read lock.
> > + #[inline]
> > + pub fn mmap_read_lock(&self) -> MmapReadGuard<'_> {
> > + // SAFETY: The pointer is valid since self is a reference.
> > + unsafe { bindings::mmap_read_lock(self.as_raw()) };
> > +
> > + // INVARIANT: We just acquired the read lock.
> > + MmapReadGuard {
> > + mm: self,
> > + _nts: NotThreadSafe,
>
> I'm sure this is a rusty thing, but curious as to why this is like that?
> What is '_nts', etc.?
_nts is the name of a field. The NotThreadSafe type has size zero, so
it doesn't exist in the compiled code. It exists only to mark that the
MmapReadGuard cannot be transferred across thread boundaries.
> > + }
> > + }
> > +
> > + /// Try to lock the mmap read lock.
> > + #[inline]
> > + pub fn mmap_read_trylock(&self) -> Option<MmapReadGuard<'_>> {
> > + // SAFETY: The pointer is valid since self is a reference.
> > + let success = unsafe { bindings::mmap_read_trylock(self.as_raw()) };
> > +
> > + if success {
> > + // INVARIANT: We just acquired the read lock.
> > + Some(MmapReadGuard {
> > + mm: self,
> > + _nts: NotThreadSafe,
> > + })
> > + } else {
> > + None
> > + }
> > + }
> > +}
> > +
> > +/// A guard for the mmap read lock.
> > +///
> > +/// # Invariants
> > +///
> > +/// This `MmapReadGuard` guard owns the mmap read lock.
> > +pub struct MmapReadGuard<'a> {
> > + mm: &'a MmWithUser,
> > + // `mmap_read_lock` and `mmap_read_unlock` must be called on the same thread
> > + _nts: NotThreadSafe,
> > +}
> > +
> > +impl Drop for MmapReadGuard<'_> {
> > + #[inline]
> > + fn drop(&mut self) {
> > + // SAFETY: We hold the read lock by the type invariants.
> > + unsafe { bindings::mmap_read_unlock(self.mm.as_raw()) };
> > + }
> > +}
>
> Ah that's nice, an actual guard for it :) I'm guessing the fact this
> implements the guard implies that you _must_ hold the lock first right?
Yeah so this code runs when a variable of type MmapReadGuard goes out
of scope. We don't provide any way to obtain an MmapReadGuard without
locking the mmap read guard.
> >
> > --
> > 2.47.0.371.ga323438b13-goog
> >
>
> Sorry for the numerous questions, I'm afraid there'll be a lot of this for
> rust stuff for the time being. Perhaps advent of code will help improve
> things on the rust front for me ;)
Thanks for all the excellent questions!
Alice
