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.
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.
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.
Thanks for the review!
- Joel
- va = __find_vmap_area((unsigned long)objp, &vmap_area_root);
- if (!va) {
spin_unlock(&vmap_area_lock);- }
- 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);}
#endif
2.42.0.283.g2d96d420d3-goog