Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks

[Suren Baghdasaryan <surenb@google.com>]

On Sat, Apr 13, 2024 at 3:52 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sat, Apr 13, 2024 at 02:46:56PM -0700, Suren Baghdasaryan wrote:
> > On Fri, Apr 12, 2024 at 8:31 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >  - Rename lock_vma() to uffd_lock_vma() because it really is uffd
> > >    specific.
> >
> > I'm planning to expand the scope of lock_vma() and reuse it for
> > /proc/pid/maps reading under per-VMA locks. No objection to renaming
> > it for now but I'll likely rename it back later once it's used in more
> > places.
>
> That would seem like a mistake.  The uffd lock_vma() will create an
> anon_vma for VMAs that don't have one, and you wouldn't want that.
> It seems to me that lock_vma_under_rcu() does everything you want except
> the fallback to mmap_read_lock().  And I'm not sure there's a good way
> to package that up ... indeed, I don't see why you'd want the "take
> the mmap_lock, look up the VMA, drop the mmap read lock" part at all --
> once you've got the mmap_lock, just hold it until you're done.

Yeah, you are right about anon_vma creation. I definitely don't want
that part when reading maps files.

Not sure about holding mmap_lock until I'm done. The goal of that
patch is to minimize blocking of any modifications while we are
reading maps files, so locking smaller parts might still make sense.
But it would be hard to argue one way or another without any data.