Re: [PATCH v4] rust: mm: add abstractions for mm_struct and vm_area_struct

[Benno Lossin <benno.lossin@proton.me>]

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@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

I have one nit below, with that fixed:

Reviewed-by: Benno Lossin <benno.lossin@proton.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?

---
Cheers,
Benno

> +        // SAFETY: The caller ensures that the invariants are satisfied for the duration of 'a.
> +        unsafe { &*vma.cast() }
> +    }