* Alice Ryhl <aliceryhl@xxxxxxxxxx> [250204 07:45]:
> On Mon, Feb 3, 2025 at 4:44 PM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> 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?
>
> > > + 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.
...
>
> > > + // 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.
The non-zero statement is fine. After re-reading it, I think this is
accurate.
Thanks for taking the time to explain things.
Liam
