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 E285FEE14D3 for ; Wed, 6 Sep 2023 19:47:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0FF278E0016; Wed, 6 Sep 2023 15:47:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0B0918D0005; Wed, 6 Sep 2023 15:47:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E92548E0016; Wed, 6 Sep 2023 15:46:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id D91848D0005 for ; Wed, 6 Sep 2023 15:46:59 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 960A5C0ED8 for ; Wed, 6 Sep 2023 19:46:59 +0000 (UTC) X-FDA: 81207205758.10.ED90D65 Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by imf30.hostedemail.com (Postfix) with ESMTP id 64E8F8001F for ; Wed, 6 Sep 2023 19:46:56 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="A5fOmom/"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf30.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.46 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694029616; 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=XIUc2xK683d3Q/ZcNSmkTYY4rSdKAhFf7bFcO8REd1s=; b=o7FtCOVk4Xk9vyXdjkqpvO2rJHk5Hfc2vK9gYLZf59CGpQiTEgcR2ZD5tA+hA3M3j+7rFZ Fh33cWpVcsqgNgnYWz58Tn1VilDGov5tsxXfKweHbRH64sTX9n50+U4W7D195NxZEBFwUO 01D7DzE8xcZMv5jSsNqGrtAnVgMb5E0= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="A5fOmom/"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf30.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.46 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694029616; a=rsa-sha256; cv=none; b=VS7wP0MmSqYp3oYUOaaFmnNzMRNGbYS83OsN5sRf+3yMouWU9/axs0nmQVF7MfQ0snc9kt QGYEfsF7vyIuwdN3H7PJ9GjkamlLAlw7GfcNIQ4fEbGMlt8axC3DzA12vOn04x7Th94mxq eUlGq3tDvi2Ft12jQqaFV3KuzM54Y7I= Received: by mail-wm1-f46.google.com with SMTP id 5b1f17b1804b1-401d10e3e54so2416875e9.2 for ; Wed, 06 Sep 2023 12:46:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694029615; x=1694634415; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=XIUc2xK683d3Q/ZcNSmkTYY4rSdKAhFf7bFcO8REd1s=; b=A5fOmom/ieSRfJ1f8ol2t6gP7loQnqeonfBzrf0HOD0x51z/NlblEDbJ8L9xN75abH USvA5M1YiDgnVJz8jeo/RBooex77aFJ9vsmpuLwkEapycmHWglsxO/kC4+ifrjiq0ico /6aCHP05Ha7DIWcgKKRZmtqQj7hozBKX5Ot+JGMLdZ+7KvVjfSN2gutBuWfzdIaPKw8k P4IxmakM+TEh7N7EqltSe5Y9rnsJAgCmwXdl+9NJlEL/aXjNYGblOqonRErcuLtLazSE LMGbPkyf9qbiLVbR8CQX7wqJA4PDdcHKNCZs7ndOnX/ZHNHrXwdAs/ue/zVHSDXR8fkh ykjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1694029615; x=1694634415; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=XIUc2xK683d3Q/ZcNSmkTYY4rSdKAhFf7bFcO8REd1s=; b=buOX12gt/6twdH1q2dinrCKCJGAt4t1r7E2QCjOsbe3hO4O372DoY+4bUxRxNFxFBC qOq9250Qp0SibJJI/RuGzXAejTCNUmfmbCmSAa0authKRF2de19PvhyU1QbjNLUSSY9e e/f7dgrRP4x9rZzph4h/L57gAA5Y2C4IS0cBuUgFax+BXLKc7GsRGd8v4OCzM+HSDj+b eh1BnONrXpiLlFg0rgYtORzE1r/d4yWiAxGnT4pLrmFYbjkOeNQ7GdY383pFzC0mZR8+ TWBRy80plfNQmTLxo2x8huw6szwDc8AVYKfrf5KhUqy0/x6JIjGVHUCc6uxY8C4EtdrL QTgw== X-Gm-Message-State: AOJu0YwJOwZjQnxGYRUNkSNtayjbt02MS2tmXr+mwBaJmP7ARvEK9nYo rUHH3jvNlmJCe243a1AZr8Y= X-Google-Smtp-Source: AGHT+IHX/HTYarWHVUGZ8Kl0yIwsTgpFcNTDpxunqaE4hq8YLu00AaGgz+HCZH9+NHFr6Q/ekHWjZQ== X-Received: by 2002:a1c:770e:0:b0:401:cdc1:ac82 with SMTP id t14-20020a1c770e000000b00401cdc1ac82mr3387067wmi.9.1694029614483; Wed, 06 Sep 2023 12:46:54 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id o3-20020a5d4083000000b0031435731dfasm21167212wrp.35.2023.09.06.12.46.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Sep 2023 12:46:53 -0700 (PDT) Date: Wed, 6 Sep 2023 20:46:52 +0100 From: Lorenzo Stoakes 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 Subject: Re: [PATCH v3 1/2] mm/vmalloc: Add a safer version of find_vm_area() for debug Message-ID: <4f19f95a-e93c-4a30-8565-88e044960be0@lucifer.local> References: <20230904180806.1002832-1-joel@joelfernandes.org> <571d4a4a-0674-4c84-b714-8e7582699e30@lucifer.local> <20230905114709.GA3881391@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Stat-Signature: xr4w1c7y7byi8s4b3nhrbpfxbxijaoq6 X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 64E8F8001F X-HE-Tag: 1694029616-862282 X-HE-Meta: U2FsdGVkX1+ulDg7pAHFc3oA+MfFOsi/oHbVZlxBcl+ykCNGaPpCwd64WeTI9SjCG78DP3rHWZxoN5C7zYDvUoDcx40yE+w2WZleTXgRcdT+dkx36OZOP1Ys1CmltUeszSW9o3bPbZo9OicYyp0KssDSGFgsqXQH58Ue/3KyfOswCDRpvKAa4zPBwh9QIPihfV7wU1qqqSArr76nQyZEIjdEOyGLg8rtEYP1L2idkCpnw5pj1boyLM+4Y4GCPeeDGjHevi0SDPY0t3mnGN28IPLWpP/lSzLRTelDEF8YC9gyPSEZMRxQbE9tqB2C1COV+B5VpWzLbska0GjcpuLVM6vPHFsoQyHe3wy9DXCJpMRzp7qgxrgNB0z8qYFu1cTRSDUfG9iBzkn3vbAo2/tkpZfWbClg4AUOP39SIQy2cCY+7CLA7UVmOLlCjrv/8+Q0nywon4kW0IfVpEVFPlwJhcWMLYI/h/LUFGTsQuWNvM0+GuUYcnVl61BgmMr8pRYuegEyFYX3SyOlsZbkPdtbJUM554zvkwEbe6bjlELMauE2d6cHgXVBJqbAZxIISeh7fTQtTpT0sLk8v3IoTEexjDrNK/5Xs73hGmzBVV8Froa66dVwa3qPmmcfvj3jDitI4mkIE+7kupbE2yYde7wPTqLQov6YFR2ndJGFWSUEocBtVXbtGCxh4NhRxhsdWcFjIgtliQxyRXIwXOR7aw0Bglu4XcSOnJ/ALhX09GdRTAK4pTJ4IYVf6c1PFdQ/u0yaOBjh9oON3Vo8lU3Yqf6i8czNgd2PxVYkrstYKUBKrSiYe2/NUrcYGpEJWnUBdazLUAwrgzCotXvrIx88dWFlZy0IVwmLHMd6Sf/QcQ16iGuVP/NWNcQSIz2R32QuQ5St4Im9Gx/kKhR7aDAYmDQU/w1C0Y56Q9WRVVzGvYnl8JFoEDGJXlqSOtfXCAa/1U/Jv6U2bpVQXdu5LB95TxL cDp4xthk zXLM4f6lNZGKjteLFK1lMpTc15lDe9IORA/QBe3Ig7T86fAYsFG9H1H2//RM790TGlyWMbGLXvLCsvIlPmKuYa+Hdo8MSZjRS4wS2EkBCmr778O/uE7LCbj9jHdyh/kVCOMeW7x5yRa2GnxicVzhXONG9DKllIv4XdLUq5dB3FMOjIMUDUUOm3/YprhKt2SHajR8GT1EY8MMuwy1cnmntZ6xC1xBcud4qEGQPYqCUHJNrIasAaGo3BgyuS1gUYjXfkz8VEa/PZzpQQzG+DA4gny03IiyPK2PgDntlpBHMU0dn9O7TPu+a4j36KacPdmqgEC5vpx21U8iVogC/OhOlSL9eWVCf1rj165Tn2pLST3UuUm9YFxwUueE93c+F4nT7b9eaI32Hg2wQTnlPPZO+AgsE0qJVQe1D9RAZiHD31aRwldzhu9hrdEytL7rem8q8EmPMVXxwAaF1SZO1cZr48197b0lKvUDBosd1p9Tk4O5q98xZKwfu0qSbpiPTCoxyN0m/CuHaIFXr53lAmvDI7d2wd/IIidmp2MSWpADHGPW355VrXXPrDdNL3BiVMEkkY86rNHOP/ojw79WnhIk8GxPJoymv4tFgxJNIXOjuuWVpw8qaH5Z6djBDv2fKWZpVDb+uLk912sh360xMuwhBYyn5S4kxtgeu1akHww4nKxdXajEazLF5e3hNRv3OA+iHYz4rtbIUXqY37RCANAH93IDHkQ== 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 Wed, Sep 06, 2023 at 08:23:18PM +0100, Lorenzo Stoakes wrote: > 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!'... > 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!