Re: [PATCH v3 1/2] mm/vmalloc: Add a safer version of find_vm_area() for debug

[Lorenzo Stoakes <lstoakes@gmail.com>]

On Tue, 5 Sept 2023 at 12:47, Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Tue, Sep 05, 2023 at 08:09:16AM +0100, Lorenzo Stoakes wrote:
> > On Mon, Sep 04, 2023 at 06:08:04PM +0000, Joel Fernandes (Google) wrote:
> > > It is unsafe to dump vmalloc area information when trying to do so from
> > > some contexts. Add a safer trylock version of the same function to do a
> > > best-effort VMA finding and use it from vmalloc_dump_obj().
> >
> > It'd be nice to have more details as to precisely which contexts and what this
> > resolves.
>
> True. I was hoping the 'trylock' mention would be sufficient (example hardirq
> context interrupting a lock-held region) but you're right.
>
> > > [applied test robot feedback on unused function fix.]
> > > [applied Uladzislau feedback on locking.]
> > >
> > > Reported-by: Zhen Lei <thunder.leizhen@huaweicloud.com>
> > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > Cc: rcu@vger.kernel.org
> > > Cc: Zqiang <qiang.zhang1211@gmail.com>
> > > Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > Fixes: 98f180837a89 ("mm: Make mem_dump_obj() handle vmalloc() memory")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > >  mm/vmalloc.c | 26 ++++++++++++++++++++++----
> > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index 93cf99aba335..2c6a0e2ff404 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -4274,14 +4274,32 @@ void pcpu_free_vm_areas(struct vm_struct **vms, int nr_vms)
> > >  #ifdef CONFIG_PRINTK
> > >  bool vmalloc_dump_obj(void *object)
> > >  {
> > > -   struct vm_struct *vm;
> > >     void *objp = (void *)PAGE_ALIGN((unsigned long)object);
> > > +   const void *caller;
> > > +   struct vm_struct *vm;
> > > +   struct vmap_area *va;
> > > +   unsigned long addr;
> > > +   unsigned int nr_pages;
> > >
> > > -   vm = find_vm_area(objp);
> > > -   if (!vm)
> > > +   if (!spin_trylock(&vmap_area_lock))
> > > +           return false;
> >
> > It'd be good to have a comment here explaining why we must trylock here. I am
> > also concerned that in the past this function would return false only if the
> > address was not a vmalloc one, but now it might just return false due to lock
> > contention and the user has no idea which it is?
> >
> > I'd want to at least output "vmalloc region cannot lookup lock contention"
> > vs. the below cannot find case.
>
> In the patch 2/2 we do print if the address looks like a vmalloc address even
> if the vmalloc look up fails.

No, you output exactly what was output before, only changing what it
means and in no way differentiating between couldn't find vmalloc
area/couldn't get lock.

>
> Also the reporter's usecase is not a common one. We only attempt to dump
> information if there was a debug objects failure (example if somebody did a
> double call_rcu). In such a situation, the patch will prevent a deadlock and
> still print something about the address.

Right, but the function still purports to do X but does Y.

>
> > Under heavy lock contention aren't you potentially breaking the ability to
> > introspect vmalloc addresses? Wouldn't it be better to explicitly detect the
> > contexts under which acquiring this spinlock is not appropriate?
>
> Yes this is a good point, but there's another case as well: PREEMPT_RT can
> sleep on lock contention (as spinlocks are sleeping) and we can't sleep from
> call_rcu() as it may be called in contexts that cannot sleep. So we handle
> that also using trylock.

Right so somebody now has to find this email to realise that. I hate
implicit knowledge like this, it needs a comment. It also furthers the
point that it'd be useful to differentiate between the two.

>
> Thanks for the review!

This got merged despite my outstanding comments so I guess I'll have
to follow up with a patch.

>
>  - Joel
>
>
> >
> > > +   va = __find_vmap_area((unsigned long)objp, &vmap_area_root);
> > > +   if (!va) {
> > > +           spin_unlock(&vmap_area_lock);
> > >             return false;
> > > +   }
> > > +
> > > +   vm = va->vm;
> > > +   if (!vm) {
> > > +           spin_unlock(&vmap_area_lock);
> > > +           return false;
> > > +   }
> > > +   addr = (unsigned long)vm->addr;
> > > +   caller = vm->caller;
> > > +   nr_pages = vm->nr_pages;
> > > +   spin_unlock(&vmap_area_lock);
> > >     pr_cont(" %u-page vmalloc region starting at %#lx allocated at %pS\n",
> > > -           vm->nr_pages, (unsigned long)vm->addr, vm->caller);
> > > +           nr_pages, addr, caller);
> > >     return true;
> > >  }
> > >  #endif
> > > --
> > > 2.42.0.283.g2d96d420d3-goog
> > >

This reads like another 'nice review and I agree but I won't change
anything!'...


-- 
Lorenzo Stoakes
https://ljs.io