On Mon, Feb 3, 2025 at 4:44 PM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> * Alice Ryhl <aliceryhl@xxxxxxxxxx> [250203 07:15]:
>
> Hi Alice,
>
> These comments are probably for my education. I'm holding back my
> training on naming and trying to reconcile the snake-case and upper case
> camel of the rust language.
>
> > This adds a type called VmAreaRef which is used when referencing a vma
>
> How did you land on VmAreaRef? Why not VmaRef? We mostly use vma for
> vmas today, not vm_area. Is it because the header declares it
> vm_area_struct? I'd be happier with VmaRef, but I'm guessing this is a
> direct translation from vm_area_struct?
>
> > that you have read access to. Here, read access means that you hold
> > either the mmap read lock or the vma read lock (or stronger).
>
> I have questions.. they are below.
>
> >
> > Additionally, a vma_lookup method is added to the mmap read guard, which
> > enables you to obtain a &VmAreaRef in safe Rust code.
> >
> > This patch only provides a way to lock the mmap read lock, but a
> > follow-up patch also provides a way to just lock the vma read lock.
> >
> > Acked-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> > Reviewed-by: Jann Horn <jannh@xxxxxxxxxx>
> > Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
> > Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> > ---
> > rust/helpers/mm.c | 6 ++
> > rust/kernel/mm.rs | 21 +++++
> > rust/kernel/mm/virt.rs | 215 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 242 insertions(+)
> >
> > diff --git a/rust/helpers/mm.c b/rust/helpers/mm.c
> > index 7201747a5d31..7b72eb065a3e 100644
> > --- a/rust/helpers/mm.c
> > +++ b/rust/helpers/mm.c
> > @@ -37,3 +37,9 @@ void rust_helper_mmap_read_unlock(struct mm_struct *mm)
> > {
> > mmap_read_unlock(mm);
> > }
> > +
> > +struct vm_area_struct *rust_helper_vma_lookup(struct mm_struct *mm,
> > + unsigned long addr)
> > +{
> > + return vma_lookup(mm, addr);
> > +}
> > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> > index 2fb5f440af60..bd6ff40f106f 100644
> > --- a/rust/kernel/mm.rs
> > +++ b/rust/kernel/mm.rs
> > @@ -17,6 +17,8 @@
> > };
> > use core::{ops::Deref, ptr::NonNull};
> >
> > +pub mod virt;
> > +
> > /// A wrapper for the kernel's `struct mm_struct`.
> > ///
> > /// This represents the address space of a userspace process, so each process has one `Mm`
> > @@ -200,6 +202,25 @@ pub struct MmapReadGuard<'a> {
> > _nts: NotThreadSafe,
> > }
> >
> > +impl<'a> MmapReadGuard<'a> {
> > + /// Look up a vma at the given address.
> > + #[inline]
> > + pub fn vma_lookup(&self, vma_addr: usize) -> Option<&virt::VmAreaRef> {
> > + // SAFETY: We hold a reference to the mm, so the pointer must be valid. Any value is okay
> > + // for `vma_addr`.
>
> Is this true? In C we hold a reference to the mm and the vma can still
> go away. We get safety from the locking on the C side.
Notice that this function is in the `impl MmapReadGuard` block. This
implies that you *must* hold the mmap read guard to call this
function.
The safety comment should probably be updated to mention that we hold the guard.
> > + let vma = unsafe { bindings::vma_lookup(self.mm.as_raw(), vma_addr) };
> > +
> > + if vma.is_null() {
> > + None
> > + } else {
> > + // SAFETY: We just checked that a vma was found, so the pointer is valid. Furthermore,
> > + // the returned area will borrow from this read lock guard, so it can only be used
> > + // while the mmap read lock is still held.
>
> So We have complicated the locking of the vmas with rcu and per-vma
> locking recently. We are now able to look up and use a vma under the
> rcu read lock. Does this translate to rust model?
>
> I believe this is true in recent version of binder as well?
Yes. The safety requirements of VmAreaRef is that you must hold the
mmap read lock *or* the vma read lock while you have a VmAreaRef
reference. This particular method achieves that requirement by holding
the mmap read lock. But there is also a Rust lock_vma_under_rcu(), see
patch 4 for that.
> > +// Methods you can call when holding the mmap or vma read lock (or stronger). They must be usable
> > +// no matter what the vma flags are.
> > +impl VmAreaRef {
> > + /// 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 or vma
> > + /// read lock (or stronger) is held for at least the duration of 'a.
>
> What is 'a?
>
> > + #[inline]
> > + pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -> &'a Self {
>
> This 'a? What can I look up in rust to know what this is about?
This is called a lifetime. It represents some region of code, and the
idea is that the returned reference is guaranteed valid inside the
lifetime 'a. For this particular function, the caller chooses what the
lifetime is completely unsafely, so there is no check, but in other
cases it's enforced by the compiler.
For example, if we take vma_lookup from above, then its signature is
short-hand [1] for this:
pub fn vma_lookup<'a>(&'a self, vma_addr: usize) -> Option<&'a
virt::VmAreaRef> {
This signature tells the compiler that the returned pointer to
VmAreaRef can only be used in the region of code where `self` is also
valid. In this case `self` is an `MmapReadLock` (because that's the
impl block the function is defined inside), so the returned VmAreaRef
is valid in the same region as the MmapReadLock object. This enforces
that the returned pointer can only be used while you still hold the
read lock. If your code does not follow that requirement, then that's
a compilation error.
[1]: https://doc.rust-lang.org/book/ch10-03-lifetime-syntax.html#lifetime-elision
> > + // SAFETY: The caller ensures that the invariants are satisfied for the duration of 'a.
> > + unsafe { &*vma.cast() }
> > + }
> > +
> > + /// Returns a raw pointer to this area.
> > + #[inline]
> > + pub fn as_ptr(&self) -> *mut bindings::vm_area_struct {
> > + self.vma.get()
> > + }
> > +
> > + /// Access the underlying `mm_struct`.
> > + #[inline]
> > + pub fn mm(&self) -> &MmWithUser {
> > + // SAFETY: By the type invariants, this `vm_area_struct` is valid and we hold the mmap/vma
> > + // read lock or stronger. This implies that the underlying mm has a non-zero value of
> > + // `mm_users`.
>
> Again, I'm not sure this statement still holds as it once did?
If it's possible to hold the mmap/vma read lock for a vma that is part
of an mm whose mm_users count has dropped to zero, then it is
incorrect for us to have this method.
Alice
