Re: [PATCH v13 2/8] mm: rust: add vm_area_struct methods that require read access

[Alice Ryhl <aliceryhl@google.com>]

On Tue, Feb 4, 2025 at 4:46 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > > > > > +        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.
> > >
> > > Right, okay.  Thanks.  You can get the reference by only holding the rcu
> > > read lock, but you should hold the vma lock to ensure that the vma
> > > itself (and not just the pointer) is safe to use.
> >
> > Hmm... To modify the vma, you must hold the mmap *and* vma write lock,
> > so holding the mmap read lock prevents mutations?
>
> Sorry, I think I confused things with my answer.  Your code is fine.
> The phrasing of the "only be used while the mmap read lock is still
> held" made me wonder about further clarification on the locking here
> (because the locking is confusing).
>
> Yes, mmap read lock means there are no writers that can modify the vma.
> Essentially, you are using the lock to ensure the entire vma space isn't
> changed during your operation - which is heavier than just locking one
> vma.

I could extend the safety comment like this:

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. This
ensures that there are no writers because writers must hold both the
mmap and vma write lock.

Alice