* Suren Baghdasaryan <surenb@xxxxxxxxxx> [240129 15:53]:
> On Mon, Jan 29, 2024 at 12:36 PM Liam R. Howlett
...
> > > @@ -465,7 +503,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > >
> > > if (unlikely(err == -ENOENT)) {
> > > up_read(&ctx->map_changing_lock);
> > > - mmap_read_unlock(dst_mm);
> > > + unpin_vma(dst_mm, dst_vma, mmap_locked);
> > > BUG_ON(!folio);
> > >
> > > err = copy_folio_from_user(folio,
> > > @@ -474,17 +512,6 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
> > > err = -EFAULT;
> > > goto out;
> > > }
> > > - mmap_read_lock(dst_mm);
> > > - down_read(&ctx->map_changing_lock);
> > > - /*
> > > - * If memory mappings are changing because of non-cooperative
> > > - * operation (e.g. mremap) running in parallel, bail out and
> > > - * request the user to retry later
> > > - */
> > > - if (atomic_read(ctx->mmap_changing)) {
> > > - err = -EAGAIN;
> > > - break;
> > > - }
> >
> > ... Okay, this is where things get confusing.
> >
> > How about this: Don't do this locking/boolean dance.
> >
> > Instead, do something like this:
> > In mm/memory.c, below lock_vma_under_rcu(), but something like this
> >
> > struct vm_area_struct *lock_vma(struct mm_struct *mm,
> > unsigned long addr)) /* or some better name.. */
> > {
> > struct vm_area_struct *vma;
> >
> > vma = lock_vma_under_rcu(mm, addr);
> >
> > if (vma)
> > return vma;
> >
> > mmap_read_lock(mm);
> > vma = lookup_vma(mm, addr);
> > if (vma)
> > vma_start_read(vma); /* Won't fail */
>
> Please don't assume vma_start_read() won't fail even when you have
> mmap_read_lock(). See the comment in vma_start_read() about the
> possibility of an overflow producing false negatives.
I did say something *like* this...
Thanks for catching my mistake.
>
> >
> > mmap_read_unlock(mm);
> > return vma;
> > }
> >
> > Now, we know we have a vma that's vma locked if there is a vma. The vma
> > won't go away - you have it locked. The mmap lock is held for even
> > less time for your worse case, and the code gets easier to follow.
> >
> > Once you are done with the vma do a vma_end_read(vma). Don't forget to
> > do this!
> >
> > Now the comment above such a function should state that the vma needs to
> > be vma_end_read(vma), or that could go undetected.. It might be worth
> > adding a unlock_vma() counterpart to vma_end_read(vma) even.
>
> Locking VMA while holding mmap_read_lock is an interesting usage
> pattern I haven't seen yet. I think this should work quite well!
What concerns me is this working too well - for instance someone *ahem*
binder *ahem* forever and always isolating their VMA, or someone
forgetting to unlock and never noticing.
vma->vm_lock->lock being locked should be caught by lockdep on exit
though.
...
Thanks,
Liam
