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

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Alice Ryhl <aliceryhl@google.com> [250204 10:06]:
> On Tue, Feb 4, 2025 at 3:55 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Alice Ryhl <aliceryhl@google.com> [250204 07:45]:
> > > On Mon, Feb 3, 2025 at 4:44 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > > >
> > ...
> >
> > > > >
> > > > > +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.
> >
> > Thanks.
> >
> > Does it imply you must hold the lock or does it enforce that the lock is
> > held?
> 
> Not sure what distinction you're asking about. To obtain a
> MmapReadGuard object, you need to call the `mmap_read_lock` function,
> which locks the mmap lock and returns an MmapReadGuard. When the
> MmapReadGuard object goes out of scope, its destructor runs which
> unlocks the lock. You can only call this method given a reference to a
> MmapReadGuard, so you can only call it after calling mmap_read_lock
> and before its destructor runs.

I was wondering if the lock is ensured to be held.

Thanks for walking me through the way this works.

> 
> > > > > +        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.

> Is the mmap read
> lock not enough to read from the vma?

Yes, it's enough.

The vma write lock is needed to ensure there is no race between an rcu
reader using the vma and the mmap writer modifying the vma.  This is
done using the vma lock.

I am confident you already knew this and I'm just saying it now for
people who find this conversation on the mailing list.

...

Thanks,
Liam