linux-kernel - Re: [PATCH v3] rust: mm: add abstractions for mm_struct and vm_area

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: <CAH5fLgjd47eK=+k1vB2h6gDnZyVBOUi+KAr2L1Htoq3hPnqbqg@mail.gmail.com>
Date: Thu, 1 Aug 2024 16:07:01 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...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>, Andreas Hindborg <a.hindborg@...sung.com>, 
	Matthew Wilcox <willy@...radead.org>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
	Vlastimil Babka <vbabka@...e.cz>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3] rust: mm: add abstractions for mm_struct and vm_area_struct

On Thu, Aug 1, 2024 at 4:02 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Thu, Aug 01, 2024 at 12:58:45PM +0000, Alice Ryhl wrote:
> > This is a follow-up to the page abstractions [1] that were recently
> > merged in 6.11. Rust Binder will need these abstractions to manipulate
> > the vma in its implementation of the mmap fop on the Binder file.
> >
> > This patch is based on Wedson's implementation on the old rust branch,
> > but has been changed significantly. All mistakes are Alice's.
> >
> > Link: https://lore.kernel.org/r/20240528-alice-mm-v7-4-78222c31b8f4@google.com [1]
> > Co-developed-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> > ---
> > Changes in v3:
> > - Reorder entries in mm.rs.
> > - Use ARef for mmput_async helper.
> > - Clarify that VmArea requires you to hold the mmap read or write lock.
> > - Link to v2: https://lore.kernel.org/r/20240727-vma-v2-1-ab3e5927dc3a@google.com
> >
> > Changes in v2:
> > - mm.rs is redesigned from scratch making use of AsRef
> > - Add notes about whether destructors may sleep
> > - Rename Area to VmArea
> > - Link to v1: https://lore.kernel.org/r/20240723-vma-v1-1-32ad5a0118ee@google.com
> > ---
> >  rust/helpers.c         |  61 +++++++++
> >  rust/kernel/lib.rs     |   1 +
> >  rust/kernel/mm.rs      | 337 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/mm/virt.rs | 204 ++++++++++++++++++++++++++++++
> >  rust/kernel/types.rs   |   9 ++
> >  5 files changed, 612 insertions(+)
> >
> > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> > new file mode 100644
> > index 000000000000..ed2db893fb79
> > --- /dev/null
> > +++ b/rust/kernel/mm.rs
> > @@ -0,0 +1,337 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2024 Google LLC.
> > +
> > +//! Memory management.
> > +//!
> > +//! C header: [`include/linux/mm.h`](../../../../include/linux/mm.h)
>
> NIT: srctree

Ah, thanks. Good catch.

> > +
> > +    /// Returns a raw pointer to the inner `mm_struct`.
> > +    #[inline]
> > +    pub fn as_raw(&self) -> *mut bindings::mm_struct {
> > +        self.mm.get()
> > +    }
> > +
> > +    /// 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_mm<'a>(ptr: *const bindings::mm_struct) -> &'a Mm {
>
> I'd just call this `from_raw`, like you call the counterpart `as_raw` above.
> Same goes for `MmWithUser` and `VmArea`.

I've been using this naming convention since this discussion:
https://lore.kernel.org/all/20240401-marge-gepaukt-9a1972c848d9@brauner/

> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index bd189d646adb..143a2bf06941 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -366,6 +366,15 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> >              _p: PhantomData,
> >          }
> >      }
> > +
> > +    /// Pass ownership of the refcount to a raw pointer.
> > +    pub fn into_raw(self) -> NonNull<T> {
> > +        let ptr = self.ptr;
> > +        // Skip the destructor.
> > +        core::mem::forget(self);
> > +
> > +        ptr
> > +    }
>
> I think this should be a separate patch.

Sure.

Alice