Re: [PATCH v3 4/9] fs/procfs: use per-VMA RCU-protected locking in PROCMAP_QUERY API
From: Liam R. Howlett
Date: Thu Jun 06 2024 - 14:08:30 EST
* Suren Baghdasaryan <surenb@xxxxxxxxxx> [240606 13:33]:
> On Thu, Jun 6, 2024 at 10:15 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> >
> > * Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [240606 12:52]:
> > > On Wed, Jun 5, 2024 at 4:16 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Jun 4, 2024 at 5:25 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > > > >
> > > > > Attempt to use RCU-protected per-VMA lock when looking up requested VMA
> > > > > as much as possible, only falling back to mmap_lock if per-VMA lock
> > > > > failed. This is done so that querying of VMAs doesn't interfere with
> > > > > other critical tasks, like page fault handling.
> > > > >
> > > > > This has been suggested by mm folks, and we make use of a newly added
> > > > > internal API that works like find_vma(), but tries to use per-VMA lock.
> > > > >
> > > > > We have two sets of setup/query/teardown helper functions with different
> > > > > implementations depending on availability of per-VMA lock (conditioned
> > > > > on CONFIG_PER_VMA_LOCK) to abstract per-VMA lock subtleties.
> > > > >
> > > > > When per-VMA lock is available, lookup is done under RCU, attempting to
> > > > > take a per-VMA lock. If that fails, we fallback to mmap_lock, but then
> > > > > proceed to unconditionally grab per-VMA lock again, dropping mmap_lock
> > > > > immediately. In this configuration mmap_lock is never helf for long,
> > > > > minimizing disruptions while querying.
> > > > >
> > > > > When per-VMA lock is compiled out, we take mmap_lock once, query VMAs
> > > > > using find_vma() API, and then unlock mmap_lock at the very end once as
> > > > > well. In this setup we avoid locking/unlocking mmap_lock on every looked
> > > > > up VMA (depending on query parameters we might need to iterate a few of
> > > > > them).
> > > > >
> > > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > > > ---
> > > > > fs/proc/task_mmu.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 46 insertions(+)
> > > > >
> > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > > > index 614fbe5d0667..140032ffc551 100644
> > > > > --- a/fs/proc/task_mmu.c
> > > > > +++ b/fs/proc/task_mmu.c
> > > > > @@ -388,6 +388,49 @@ static int pid_maps_open(struct inode *inode, struct file *file)
> > > > > PROCMAP_QUERY_VMA_FLAGS \
> > > > > )
> > > > >
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > +static int query_vma_setup(struct mm_struct *mm)
> > > > > +{
> > > > > + /* in the presence of per-VMA lock we don't need any setup/teardown */
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static void query_vma_teardown(struct mm_struct *mm, struct vm_area_struct *vma)
> > > > > +{
> > > > > + /* in the presence of per-VMA lock we need to unlock vma, if present */
> > > > > + if (vma)
> > > > > + vma_end_read(vma);
> > > > > +}
> > > > > +
> > > > > +static struct vm_area_struct *query_vma_find_by_addr(struct mm_struct *mm, unsigned long addr)
> > > > > +{
> > > > > + struct vm_area_struct *vma;
> > > > > +
> > > > > + /* try to use less disruptive per-VMA lock */
> > > > > + vma = find_and_lock_vma_rcu(mm, addr);
> > > > > + if (IS_ERR(vma)) {
> > > > > + /* failed to take per-VMA lock, fallback to mmap_lock */
> > > > > + if (mmap_read_lock_killable(mm))
> > > > > + return ERR_PTR(-EINTR);
> > > > > +
> > > > > + vma = find_vma(mm, addr);
> > > > > + if (vma) {
> > > > > + /*
> > > > > + * We cannot use vma_start_read() as it may fail due to
> > > > > + * false locked (see comment in vma_start_read()). We
> > > > > + * can avoid that by directly locking vm_lock under
> > > > > + * mmap_lock, which guarantees that nobody can lock the
> > > > > + * vma for write (vma_start_write()) under us.
> > > > > + */
> > > > > + down_read(&vma->vm_lock->lock);
> > > >
> > > > Hi Andrii,
> > > > The above pattern of locking VMA under mmap_lock and then dropping
> > > > mmap_lock is becoming more common. Matthew had an RFC proposal for an
> > > > API to do this here:
> > > > https://lore.kernel.org/all/ZivhG0yrbpFqORDw@xxxxxxxxxxxxxxxxxxxx/. It
> > > > might be worth reviving that discussion.
> > >
> > > Sure, it would be nice to have generic and blessed primitives to use
> > > here. But the good news is that once this is all figured out by you mm
> > > folks, it should be easy to make use of those primitives here, right?
> > >
> > > >
> > > > > + }
> > > > > +
> > > > > + mmap_read_unlock(mm);
> > > >
> > > > Later on in your code you are calling get_vma_name() which might call
> > > > anon_vma_name() to retrieve user-defined VMA name. After this patch
> > > > this operation will be done without holding mmap_lock, however per
> > > > https://elixir.bootlin.com/linux/latest/source/include/linux/mm_types.h#L582
> > > > this function has to be called with mmap_lock held for read. Indeed
> > > > with debug flags enabled you should hit this assertion:
> > > > https://elixir.bootlin.com/linux/latest/source/mm/madvise.c#L96.
> >
> > The documentation on the first link says to hold the lock or take a
> > reference, but then we assert the lock. If you take a reference to the
> > anon vma name, then we will trigger the assert. Either the
> > documentation needs changing or the assert is incorrect - or I'm missing
> > something?
>
> I think the documentation is correct. It says that at the time of
> calling anon_vma_name() the mmap_lock should be locked (hence the
> assertion). Then the user can raise anon_vma_name refcount, drop
> mmap_lock and safely continue using anon_vma_name object. IOW this is
> fine:
>
> mmap_read_lock(vma->mm);
> anon_name = anon_vma_name(vma);
> anon_vma_name_get(anon_name);
> mmap_read_unlock(vma->mm);
> // keep using anon_name
> anon_vma_name_put(anon_name);
>
I consider that an optimistic view of what will happen, but okay.