On Tue, 5 Sept 2023 at 12:47, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > Cc: rcu@xxxxxxxxxxxxxxx
> > > Cc: Zqiang <qiang.zhang1211@xxxxxxxxx>
> > > Reviewed-by: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> > > Fixes: 98f180837a89 ("mm: Make mem_dump_obj() handle vmalloc() memory")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
