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

[Lorenzo Stoakes <lstoakes@gmail.com>]

On Wed, Sep 06, 2023 at 08:23:18PM +0100, Lorenzo Stoakes wrote:
> 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!'...
>

Sorry I actually wrote this unkind comment in a moment of annoyance then
meant to delete it but of course forgot to :>) Disregard this bit.

Happy for pushback/disagreement, just feel like a few little touchups would
have helped improve documentation/clarity of what this series does.

Obviously stability matters more so perhaps touch-ups best as a follow up
series... though would be nice to have a comment to that effect.

Thanks!