Re: [PATCH v11 6/8] mm: rust: add VmAreaNew for f_ops->mmap()

"Alice Ryhl" [off-list ref] writes:

On Thu, Jan 9, 2025 at 9:19 AM Andreas Hindborg [off-list ref] wrote:

quoted

"Alice Ryhl" [off-list ref] writes:

quoted

On Mon, Dec 16, 2024 at 3:51 PM Andreas Hindborg [off-list ref] wrote:

quoted

"Alice Ryhl" [off-list ref] writes:

quoted

This type will be used when setting up a new vma in an f_ops->mmap()
hook. Using a separate type from VmAreaRef allows us to have a separate
set of operations that you are only able to use during the mmap() hook.
For example, the VM_MIXEDMAP flag must not be changed after the initial
setup that happens during the f_ops->mmap() hook.

To avoid setting invalid flag values, the methods for clearing
VM_MAYWRITE and similar involve a check of VM_WRITE, and return an error
if VM_WRITE is set. Trying to use `try_clear_maywrite` without checking
the return value results in a compilation error because the `Result`
type is marked #[must_use].

For now, there's only a method for VM_MIXEDMAP and not VM_PFNMAP. When
we add a VM_PFNMAP method, we will need some way to prevent you from
setting both VM_MIXEDMAP and VM_PFNMAP on the same vma.

Acked-by: Lorenzo Stoakes <redacted> (for mm bits)
Reviewed-by: Jann Horn <jannh@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/mm/virt.rs | 181 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 180 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
index 3a23854e14f4..6d9ba56d4f95 100644
--- a/rust/kernel/mm/virt.rs
+++ b/rust/kernel/mm/virt.rs

@@ -6,7 +6,7 @@

 use crate::{
     bindings,
-    error::{to_result, Result},
+    error::{code::EINVAL, to_result, Result},
     mm::MmWithUser,
     page::Page,
     types::Opaque,

@@ -171,6 +171,185 @@ pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result {
     }
 }

+/// A builder for setting up a vma in an `f_ops->mmap()` hook.

Reading this line, I would expect to be able to chain update methods as
in `Builder::new().prop_a().prop_b().build()`. Could/should this type
accommodate a proper builder pattern? Or is "builder" not the right word
to use here?

You cannot create values of this type yourself. Only the C
infrastructure can do so.

What would you call it if not "builder"?

It looks more like a newtype with a bunch of setters and getters. It
also does not have a method to instantiate (`build()` or similar). So
how about newtype?

I don't think newtype is helpful. Ultimately, the f_ops->mmap() hook
is a *constructor* for a VMA, and the VmAreaNew type represents a VMA
whose constructor is currently running. The "method to instantiate" is
called "return".

fn mmap(new_vma: &VmAreaNew) -> Result {
    // VMAs for this driver must not be mapped as executable
    new_vma.try_clear_may_exec()?;

    // we are done constructing the vma, so return
    Ok(())
}

Alice

Right. Let's update the docs for the `mmap` hook then:

+    /// Handle for mmap.
+    fn mmap(
+        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
+        _file: &File,
+        _vma: &VmAreaNew,
+    ) -> Result {
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+    }
+

Handle for mmap.

This function is invoked when a user space process invokes the `mmap`
system call on `_file`. The function is a callback that is part of the
VMA initializer. The kernel will do initial setup of the VMA before
calling this function. The function can then interact with the VMA
initialization by calling methods of `_vma`. If the function does not
return an error, the kernel will complete intialization of the VMA
according to the properties of `_vma`.

But I still do not think "builder" is the right term. `VmAreaNew` is
more like a configuration object to pass initialization properties of
the VMA back to the kernel.

How about "configuration object"?


Best regards,
Andreas Hindborg