Re: [PATCH v2 3/3] userfaultfd: use per-vma locks in userfaultfd operations

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

* Lokesh Gidra <lokeshgidra@google.com> [240206 11:26]:
> On Tue, Feb 6, 2024 at 6:35 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Lokesh Gidra <lokeshgidra@google.com> [240205 17:24]:
> > > On Mon, Feb 5, 2024 at 2:00 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > > >
> > > > * Lokesh Gidra <lokeshgidra@google.com> [240205 16:55]:
> > > > ...
> > > >
> > > > > > > > We can take care of anon_vma as well here right? I can take a bool
> > > > > > > > parameter ('prepare_anon' or something) and then:
> > > > > > > >
> > > > > > > >            if (vma) {
> > > > > > > >                     if (prepare_anon && vma_is_anonymous(vma)) &&
> > > > > > > > !anon_vma_prepare(vma)) {
> > > > > > > >                                       vma = ERR_PTR(-ENOMEM);
> > > > > > > >                                       goto out_unlock;
> > > > > > > >                    }
> > > > > > > > >                 vma_aquire_read_lock(vma);
> > > > > > > >            }
> > > > > > > > out_unlock:
> > > > > > > > >         mmap_read_unlock(mm);
> > > > > > > > >         return vma;
> > > > > > > > > }
> > > > > > >
> > > > > > > Do you need this?  I didn't think this was happening in the code as
> > > > > > > written?  If you need it I would suggest making it happen always and
> > > > > > > ditch the flag until a user needs this variant, but document what's
> > > > > > > going on in here or even have a better name.
> > > > > >
> > > > > > I think yes, you do need this. I can see calls to anon_vma_prepare()
> > > > > > under mmap_read_lock() protection in both mfill_atomic_hugetlb() and
> > > > > > in mfill_atomic(). This means, just like in the pagefault path, we
> > > > > > modify vma->anon_vma under mmap_read_lock protection which guarantees
> > > > > > that adjacent VMAs won't change. This is important because
> > > > > > __anon_vma_prepare() uses find_mergeable_anon_vma() that needs the
> > > > > > neighboring VMAs to be stable. Per-VMA lock guarantees stability of
> > > > > > the VMA we locked but not of its neighbors, therefore holding per-VMA
> > > > > > lock while calling anon_vma_prepare() is not enough. The solution
> > > > > > Lokesh suggests would call anon_vma_prepare() under mmap_read_lock and
> > > > > > therefore would avoid the issue.
> > > > > >
> > > >
> > > > ...
> > > >
> > > > > anon_vma_prepare() is also called in validate_move_areas() via move_pages().
> > > >
> > > > Probably worth doing it unconditionally and have a comment as to why it
> > > > is necessary.
> > > >
> > > The src_vma (in case of move_pages()) doesn't need to have it.
> > >
> > > The only reason I'm not inclined to make it unconditional is what if
> > > some future user of lock_vma() doesn't need it for their purpose? Why
> > > allocate anon_vma in that case.
> >
> > Because there isn't a user and it'll add a flag that's a constant.  If
> > there is a need for the flag later then it can be added at that time.
> > Maybe there will never be a user and we've just complicated the code for
> > no reason.  Don't implement features that aren't necessary, especially
> > if there is no intent to use them.
> >
> 
> I'm not too attached to the idea of keeping it conditional. But I have
> already sent v3 which currently does it conditionally. Please take a
> look at it. Along with any other comments/changes that I get, I'll
> also make it unconditional in v4, if you say so.

well, you use it conditionally, so it does have use.  It was not clear
in your comment above that you were going to use it.  I am not sure
about the dst/src needing/not needing it.  If you have a user, then
leave it in.

Thanks,
Liam