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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Thu, Jan 9, 2025 at 9:19 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > On Mon, Dec 16, 2024 at 3:51 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> "Alice Ryhl" <aliceryhl@google.com> writes:
>> >>
>> >> > 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 <lorenzo.stoakes@oracle.com> (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