From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B3927EE14D3 for ; Wed, 6 Sep 2023 19:23:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1638A8D001A; Wed, 6 Sep 2023 15:23:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 113728D0005; Wed, 6 Sep 2023 15:23:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F1D448D001A; Wed, 6 Sep 2023 15:23:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id E1E3B8D0005 for ; Wed, 6 Sep 2023 15:23:32 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id B2A04B42AF for ; Wed, 6 Sep 2023 19:23:32 +0000 (UTC) X-FDA: 81207146664.19.1364F91 Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by imf17.hostedemail.com (Postfix) with ESMTP id E30A840009 for ; Wed, 6 Sep 2023 19:23:30 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=RlB79NeE; spf=pass (imf17.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.216.45 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694028210; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ZikwxwV5OUPtPOXuowNsFclCwHU3dah79qe2K+yR5dQ=; b=40FcbXccRXrzmMRvJsU/SpqgeyDSGswS0fdx/C3272qm5voN/1eDhpYpOUa+uj2o09jePR Iklxax+ilZQAX6ToM+1U8E3TuBs878BytFIOQI+0R9YR0zj5lfJ48/Zs8+I+fq5TmvUPcp Tym09qHdyHC/9iqnxt/Uyb7dUlv8T90= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694028211; a=rsa-sha256; cv=none; b=msTF8kiUW2jP73/S3p6ZsMVGW/We7S4e0dK/PVQoso426eTy2jdrYXjC2g6cgIwlvbb7Ms M7hzdVQpueXt8nIZE7qW/zOi8xFAAXI8y7z0kxWS48KhOaBURK1fZ0J/dcXX9xHBswuAbW yEeD1x0TAwCGd2UJRBptIWzwJzY5jQM= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=RlB79NeE; spf=pass (imf17.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.216.45 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-26d1e5f2c35so121852a91.2 for ; Wed, 06 Sep 2023 12:23:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694028210; x=1694633010; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ZikwxwV5OUPtPOXuowNsFclCwHU3dah79qe2K+yR5dQ=; b=RlB79NeEISzXmmXxG9NZcpDxChywNXltow+3Z1P02ecSRQpBgMu3dcqGBxrtIBaHbT Vk4n8UlavEtIXPfTUG4efEI1gmRn1VWFP2ATjSLYOccxHI5slAFOpU4Df3ZKMtBP+w69 wUjEeDs1ADrHcpFx7VEUB0cbw8FwSY2K1qjaATHKiGIcPSU8LpC5sjJYdQ2mQB+NQ7wG kMpXAE8DsT1tKuex8RRHJPaGs8k15h+s89PmNZe3QAuW/9i6Q0JlQnaQlvw7Zw3tFu1S bWhh48bUa3Ro81yCqMJOFhylrnbQG5PkAzkcFpY+sO5buN2aHpOFoYzRCSGi3JJ7ZwRS D+rg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1694028210; x=1694633010; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ZikwxwV5OUPtPOXuowNsFclCwHU3dah79qe2K+yR5dQ=; b=l9YuR3ufMgKAlrOqge7vbFkbIUvrhT0BrOs94nlLtIZE6bjpnNn7b5SfXvBKOzdUom crny0jHzYN+OxeJi7w5Tw2NQDmhHZPbuvpKZLwgY38JkB6WIguK6H3zW4tSnbNCxrbM2 jNTG0+N+GgtlGA8gIIc0cEExgdBrNMKD6iKaR2phJeqlHR+mOhJ9ERK2QsovJ2vrTCX0 PCKTPECVrnbZt2G/hVUylBTpzAHbfcAyzTdNHvIds5BhImHYstivrFnWCz3Cf7ViTHOi 75+IGvD1b16szJhiaoxTKgcwdHpuoRhPqjxF3gXxI3eJmTIDj4fMWZiBZoJe6vr5WCfl YBAg== X-Gm-Message-State: AOJu0Yxiwo+GZ8TN9+rhJ3Cd2FSn4PbmytjJvtEw/D0RQX7N0mS7fmQU VOk4w/ld/x2FSwQboTDScvVntXzpDjh6v9kNc68= X-Google-Smtp-Source: AGHT+IGnviK2d0b1xWwIrS7FexAWR6yZvrg5Gplwuz5Tq21PdCFksCodt7+vh6io+9iy1mO1TT9B7BRlZKXqfR5VFok= X-Received: by 2002:a17:90a:d58d:b0:268:b7a2:62e8 with SMTP id v13-20020a17090ad58d00b00268b7a262e8mr16476118pju.7.1694028209835; Wed, 06 Sep 2023 12:23:29 -0700 (PDT) MIME-Version: 1.0 References: <20230904180806.1002832-1-joel@joelfernandes.org> <571d4a4a-0674-4c84-b714-8e7582699e30@lucifer.local> <20230905114709.GA3881391@google.com> In-Reply-To: <20230905114709.GA3881391@google.com> From: Lorenzo Stoakes Date: Wed, 6 Sep 2023 20:23:18 +0100 Message-ID: Subject: Re: [PATCH v3 1/2] mm/vmalloc: Add a safer version of find_vm_area() for debug To: Joel Fernandes Cc: linux-kernel@vger.kernel.org, Andrew Morton , Uladzislau Rezki , Christoph Hellwig , "Paul E. McKenney" , Vlastimil Babka , Zhen Lei , rcu@vger.kernel.org, Zqiang , stable@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: E30A840009 X-Rspam-User: X-Stat-Signature: o99chgb1pfq3qz77fd6ywncjmh33p9uk X-Rspamd-Server: rspam03 X-HE-Tag: 1694028210-527597 X-HE-Meta: U2FsdGVkX1/fdRpsT3qbLPKO08e9HFyVKkcXEmJzChNvZMRgsV0qSKKjg8hFjMDuHk9+q4IgHn8AOHCTOc6ZLA2rTOAYul0nJbBORedIUlnmUk4Hy7U5D70qro7nPxORwpZqpU6y4CX2SshX3W/tmbuajuLL0EiKixD3oSTz0X8fR4dID1rm79GXihyDVUOkYDKa5GBo4heNlZ+oC7RrHGaaj3kpImFLZkskaY/445J6XLlXmdyudvlDmt48y00DbmvIuZeVTnj2CNp2O3LkRpBHw8aaYrIkKxRG6TkEgwfOoPQcU3yV2PM2WzhuRWRM8XavO04qEHruFlgiIfOo0XUomXHyiU82QqgVp09OIxJzxCUMxQEb3MN4TX1ynvKYx12x+tnrC1Xp28hATxQTvXQNGSvp7Pw4lTr/D2tlMsROUTEo7J9BqWAalJLR5M9e3riblkBC+cHrJ4xHKml4lI4SGNKYJSo3+9XQJdavjL6I4T+b9JeKhZzJqrpi0WKymUCDW/Q7uvRrFaRMZYOipAPp/ayW9Cet8j2yV1h5qDb1He6xOihxyOk4wq8papa0q1zIJpzEuJ4zUq43F5iwU1YzZKx0MjlKI18lpRi07nW3qPWxvfbrEp6mmqP1glD6FXMARPCFPwjN6drwvp1jW1qKBIcCVSvLIBeydgRbXUGUaqIpYDnob5CmXykdB7WiIk8v+1CiXPObzvUuUqm+7H7Mb2j2W6J13GD5on9Ton/yf0fcR7/2bVvPl2tGP8AFy8VqBDUAjMsZ7kVE3RK/EBwzPgA4GJXPrgmVsnBSEb1r6C1Sz2xYlGfkQs7YmpBf1nvQ4+2euOES3rza33o66BRgApGxz73lF+MruEDVf4945sxXf7vXv6MK2xBsAJJkVvDH+XP4qqGxrxmk1VzfOZnqo+tdsvWO6Yp6s0GdiVdpzJT0MGkFFCjgRycn93eLj0XbrTZNDcEiggNqgc5 v7gepY9l Ktfyv+kYiFA4LvTRWsqiVc2ED2qIOoWGJFH8taXRwtAuwwR1xCQxI+6hCYnxWa4tVYDBESQoJNWSLSt9cp9Iv85zRhTloZhjPKAi6FZtvasXmRws9Q7FKEmuobrGne3/m2KrXqtgF2HjPMxVRP0aliDCq2esy89oYvxJ/4J2rESCbWzLOjREg5WPKamWJeL/XdONxCL5iSTzlXf3b5D3aKakthO7Kc7l6zx0sldoypjc7x4UfQCfwTNMoo1CrqdKCf4utndat4crZ3qtdIp/h1H6k0wrF4Yf2KAYh9YYxBNNddIDn35JVOFZQc4mrykV1IjOLkIA0VT1yiSBGEFpT845hIv46Fg9JNrMBdxwiW30JWIsGF99jvPBq0sn9VDTYJHJ316N7w8AA0UX070YRJshABtsJs43i03sMY6Mjd+pmYoFZLdqfUH6IUHFVAjD6fRq2VbIeaRxGkdJxuRM0up92nhWNtoGqyuRYAC+tNyOt3+vE2YZbPYuxD/r9J0K6gF6it7mF+T2eCKpBiJFJDLf72iRa1t9A3dIYJEF2BD/vDrjeLyrXLs6uy3jXOWljdnsZuuyb8kH7tQ/qcZeE4HeKPDoY25mIvorSY9B5UyKZnXYIlHOyndUVskfjEEuDVA20ZTJARHNBZPMMZNEDtlglHMlNug+KWHW/tgGhdC6ongSzBeUwlxjUGvR+ocrI0IWYxPzwytfdWgBAA13vrgjrnkr627qFtelJ X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, 5 Sept 2023 at 12:47, Joel Fernandes 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 > > > Cc: Paul E. McKenney > > > Cc: rcu@vger.kernel.org > > > Cc: Zqiang > > > Reviewed-by: Uladzislau Rezki (Sony) > > > Fixes: 98f180837a89 ("mm: Make mem_dump_obj() handle vmalloc() memory") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Joel Fernandes (Google) > > > --- > > > 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