linux-kernel - Re: [PATCH v4] 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: <CAH5fLggpERviqXQDBwPOqgMieXKOvoPi7xcq4JcO9DJ2ZFqDOQ@mail.gmail.com>
Date: Fri, 2 Aug 2024 11:40:33 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Benno Lossin <benno.lossin@...ton.me>
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>, 
	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 v4] rust: mm: add abstractions for mm_struct and vm_area_struct

On Fri, Aug 2, 2024 at 11:39 AM Benno Lossin <benno.lossin@...ton.me> wrote:
>
> On 02.08.24 09:38, 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>
>
> I have one nit below, with that fixed:
>
> Reviewed-by: Benno Lossin <benno.lossin@...ton.me>
>
> > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
> > new file mode 100644
> > index 000000000000..ec8cadb09626
> > --- /dev/null
> > +++ b/rust/kernel/mm/virt.rs
> > @@ -0,0 +1,204 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2024 Google LLC.
> > +
> > +//! Virtual memory.
> > +
> > +use crate::{
> > +    bindings,
> > +    error::{to_result, Result},
> > +    page::Page,
> > +    types::Opaque,
> > +};
> > +
> > +/// A wrapper for the kernel's `struct vm_area_struct`.
> > +///
> > +/// It represents an area of virtual memory.
> > +///
> > +/// # Invariants
> > +///
> > +/// * If the caller has shared access to this type, then they must hold the mmap read lock.
> > +/// * If the caller has exclusive access to this type, then they must hold the mmap write lock.
> > +#[repr(transparent)]
> > +pub struct VmArea {
> > +    vma: Opaque<bindings::vm_area_struct>,
> > +}
> > +
> > +impl VmArea {
> > +    /// Access a virtual memory area given a raw pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `vma` is valid for the duration of 'a, and that the mmap read lock
> > +    /// (or write lock) is held for at least the duration of 'a.
> > +    #[inline]
> > +    pub unsafe fn from_raw_vma<'a>(vma: *const bindings::vm_area_struct) -> &'a Self {
>
> I think this also should be named `from_raw`.
>
> I took a look at your conversation with Christian Brauner and I
> personally don't see the benefit of `File::from_raw_file` over
> `File::from_raw`. To me it's clear that this function takes some raw C
> structure that represents a `File` and turns it into a `File`.
> In the situation where there are multiple ways of creating something
> from different C structs, I think we should include the name. But if
> there is only one possible struct, then the name should be `from_raw`.
>
> Do you think we should re-open that discussion/start a new one on a
> naming convention for this?

Oh, I actually intended to rename it here too. I just forgot that
VmArea also had a from_raw.

Alice