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 7502AEE14D3 for ; Wed, 6 Sep 2023 22:46:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5ACFD8D000A; Wed, 6 Sep 2023 18:46:13 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 55C768D0005; Wed, 6 Sep 2023 18:46:13 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4240E8D000A; Wed, 6 Sep 2023 18:46:13 -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 307758D0005 for ; Wed, 6 Sep 2023 18:46:13 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id EDEDD160ED6 for ; Wed, 6 Sep 2023 22:46:12 +0000 (UTC) X-FDA: 81207657384.02.286C981 Received: from mail-io1-f44.google.com (mail-io1-f44.google.com [209.85.166.44]) by imf25.hostedemail.com (Postfix) with ESMTP id 1F7E0A000C for ; Wed, 6 Sep 2023 22:46:10 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=joelfernandes.org header.s=google header.b=IvIKfiiP; spf=pass (imf25.hostedemail.com: domain of joel@joelfernandes.org designates 209.85.166.44 as permitted sender) smtp.mailfrom=joel@joelfernandes.org; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694040371; a=rsa-sha256; cv=none; b=d8lkruhdS8hAghmyq5klBXNzmifj/JDKcxPYrxQF5yP06nPjYS98TWc5dApOB/oZiz/Oid cNpLvQ9RbL2FO9Ofp0gezpAAY4dV55ojnvy2OlmZQZGSWCiGslCcdvQnPUbmGJCyYCHcJl f47xr5YsVOFtpWcRU5Cy1tHvLsURtmw= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=joelfernandes.org header.s=google header.b=IvIKfiiP; spf=pass (imf25.hostedemail.com: domain of joel@joelfernandes.org designates 209.85.166.44 as permitted sender) smtp.mailfrom=joel@joelfernandes.org; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694040371; 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=Omk8h6qAI8/QBggIcV1LJ4IashJ7NgP8xYw8aRJySEM=; b=zL9kDg8ERdRio4bJiY7hb92+SYuW8fqYZ2AQcW3oPvSjvh3pVnNX5YZP4Vypy5Wx6PBQT3 JzZuMTR2i7TwmYyjpSsg4W1b2N6Qb6bTaoLpSBadbLmxc68hPgRV4VDpJK37vUQXFq6tfI 3M0ZrJomHglLp75UQlSDr7ZeP2kEXDI= Received: by mail-io1-f44.google.com with SMTP id ca18e2360f4ac-79545e141c7so8716939f.0 for ; Wed, 06 Sep 2023 15:46:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=joelfernandes.org; s=google; t=1694040370; x=1694645170; 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=Omk8h6qAI8/QBggIcV1LJ4IashJ7NgP8xYw8aRJySEM=; b=IvIKfiiPltLvyvDr0Y3usAD/GJh0vBE1scPv2rtlgEXt8QtkXZKZqZVpOwXxfdHYl/ 25rkDz3xPfLuu9MfKJ1e2cG3Hp5UCQ8evdNOi79JnHzaoSxk+7DOXXvWGY1KHOjKPDTu gPUY4Bs20VlavzfGc0q98G4XolXzDM5oDJuJI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1694040370; x=1694645170; 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=Omk8h6qAI8/QBggIcV1LJ4IashJ7NgP8xYw8aRJySEM=; b=QlizrTXg+JQob+tNtOXMykv2oHxRZyakU9UatvkRunJIlpgi9GdAwnDBlyGfMyPI3s ygAwznWaZMmWIhZBquCGC719jR4rrHcmZ+mvdzMXWKd/84M8uG7cicEPwwOCtZAipCYc bdp1xSd3arHezwHrTR9PJYkm0tdO6oZnSmrnx/xo7bDkX59NDn0cTNLeenYOJEAKbw2w wfYXoUbaZBCebAvwIpiNkT59aK0aTux1sOGhF1rRk6ga1ApnAU3lcZYTqbptDB4VAFhg nR+KGYUcahI0iyZhpNkP2NjFw4IhOOsLkMILPNp4G0J76cQLhTw4LaVRZW/FGzXlrROT rXPw== X-Gm-Message-State: AOJu0YyzR1fw1ZBVfxYy1KxFFvVXkd9TCFN7VvacrfyHR36FEf0bZI5l 5bkXn2chYi4kRRHgD7kXhy7p5A== X-Google-Smtp-Source: AGHT+IHp8evXSLOC7rntt6cZ3sHD+jrdtq6uVSsSZMx3E8qry9hjqQhqNWKp8kFBoRxi1VB+KVux7w== X-Received: by 2002:a05:6602:70f:b0:790:aed5:d0b0 with SMTP id f15-20020a056602070f00b00790aed5d0b0mr1178474iox.0.1694040370078; Wed, 06 Sep 2023 15:46:10 -0700 (PDT) Received: from localhost (156.190.123.34.bc.googleusercontent.com. [34.123.190.156]) by smtp.gmail.com with ESMTPSA id s17-20020a6bd311000000b00790d72848efsm5210392iob.15.2023.09.06.15.46.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Sep 2023 15:46:09 -0700 (PDT) Date: Wed, 6 Sep 2023 22:46:08 +0000 From: Joel Fernandes To: Lorenzo Stoakes 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: <20230906224608.GB1646335@google.com> 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-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 1F7E0A000C X-Stat-Signature: kdzc68ckmhd1xpiyipn8ci99a1hb3p79 X-Rspam-User: X-HE-Tag: 1694040370-245422 X-HE-Meta: U2FsdGVkX1+psHg1uGKFVB8XLyQ/fUlsYTVnSrNCXVhTGTF3VrpMGuNnjtSHBCUfSVfXgLiURj3mFB5X9I/tWV7ODX7goPe6kt2TqohfHXFZ9CqSMx+rLxml3id/2rf7K02y5s5klof4Odn4ZMr6KJDhopXeDdiiMIlIPq5483/ODqruVNduQAB2UwvHUNJmAo26a6f+ZBj7+B67HTHvp3D7YDrE3FAZKXHAULHuglPNm4k+aE5F3tJ9OMe4tcOSQWDB0eYV9m0JFC9b2B5v5+bE7DSNhzLX5dY6n3ArIfxDbdqUgHiikKzRVJH3G5mbMMVNZxnEakCCGtLNR1UI9uRQpgBMCljoWfGwYeJa7J7i7FSOmyc4adOy+PpMRT0pjDEEXhzWk3djE+3hzECN+x7Y3IiH+Y0qtkPj5ENayJO5W5a8GRZqIkhxTR2x5ApVc09VicNCvvM9jh1LgsUuQ4GpeoO9OepIu84GmEgC7yONSOXTRgNlajfrHYjckFoS6/mvFiM6PYPUWY22sLMClFldBlha/JsSPLURIPoQJbvUavX6IBhEYiYsPZr4FS8CWyHBbPO8mGb80OhatUo1TO67xIjNrNuKlcunN9Z1LsSpQPB+53XTcAYEsvUskSxQKTGP5pHSm6zGoS6w8EnA92sldEbgHbnBbr35Ir0ifbanaUnFWquwkLP1fYSTsrp7h+Fbd15ZVN943f2uCddyocd99ng+YnYaZ5I/cmO+qCh6IWKPzUcpPJgUWKOlDI4/m+UDRsQ48ibciLR10QY8KSu/H/6yTsTODiKYPgS/flXnlp3GdSjBmFDc2OMr8xQE5MCbUSlvosg/bn50nbqiKazoz84GBskfBk0eiMj0ZZTjI8i8gpKmYEosPp+sX5tTtse0RoEwbRPotaE6Z6a/5u6CXaEH0YbdWXOezly6VBpqaLGjYxTJ2MPDKHaijbPoIzzNFXCfh7nbOhIVVh6 b7hW8XIx wiuufkqLqutneYRLEU6EZMz5zV39WnHNpOqE9IHMzH6tOxuNB8fsmYzuUPePXuIye+7h65BhW9nxDHOB3ATlgtbW6/NIEeiFy0bHbEAncFp/PihXHeNq2hh/yJmk4QFwU6qs2dcbTqkvNI/e6MIZ56bYGK4Ebd9EDZ86AY7t0kmsKbK7iOh2BQmNGjM32WuG/ZI6ZpWWhHDB+lBdtwaDtni7LXuP0DQ9LfCEcJmhuSQb4Qx3uAEb0/5ZrZV2FNwYIXd8zZ1f/Mw2RDJ5ieBQ0jRizY5fbZQHHh/LoPpcSK+v48/wakRM4jGhDvfi0T4UunzUEsWxYXPIbAALqVSKvvEWKmdHGvEiwzp+k0MQZuLguz34uB4gat/QU45z/PP3h4PGo+7vb/vSEqoB0RSrHfwa9Vju7vH8xag30a9RYqqL2fm3KnNEX22hebCmRtPhQmM863T45Uc7BPNOgm0F2AApyo7tJRqqA9/yk6OqJpwoDIKsvkQA+a2Lkyt16T4ORiNPNOzXczLV/MzBBiIjUeNQZnQDJNQ9DbqEjRDIS2LxdERVkGRJFxA/k0s1t4LxZWWF2psJ0QTp2E/NH9LdWAaQS+fjVLSBKlvkttCmRFRFctp1+tRRO1WhZfUchlMk/GNAW4HLk46s534M8Od6RPWUqF71IeRDs/wi7Jkb6zTBSMbsDJrN9HyeNxg== 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. 2/2 does this: - if (virt_addr_valid(object)) + if (is_vmalloc_addr(object)) + type = "vmalloc memory"; + else if (virt_addr_valid(object)) type = "non-slab/vmalloc memory"; This code is executed only if vmalloc_dump_obj() returns false. The is_vmalloc_addr() was added by 2/2 which is newly added right? You are right we are not differentiating between trylock failure and failure to find the vmalloc area. I was just saying, even though we don't differentiate, we do print "vmalloc memory" right? That wasn't being printed before. > > 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. This is a valid point, and I acknowledged it in last email. A code comment could indeed be useful. So I guess from an agreement standpoint, I agree: 1/2 could use an additional comment explaining why we need trylock (sighting the RT sleeping lock issue). 2/2 could update the existing code to convert "non-slab/vmalloc" to "non-slab/non-vmalloc". Note: that's an *existing* issue. The issue in 2/2 is not a new one so that can certainly be a separate patch. And while at it, we could update the comment in that patch as well. But the whole differentiating between trylock vs vmalloc area lookup failure is not that useful -- just my opinion fwiw! I honestly feel differentiating between trylock vs vmalloc area lookup failure complicates the code because it will require passing this information down from vmalloc_dump_obj() to the caller AFAICS and I am not sure if the person reading the debug will really care much. But I am OK with whatever the -mm community wants and I am happy to send out a new patch on top with the above that I agree on since Andrew took these 2 (but for the stuff I don't agree, I would appreciate if you could send a patch for review and I am happy to review it!). As you mentioned, this series is a stability fix and we can put touch-ups on top of it if needed, and there is also plenty of time till the next merge window. Allow me a few days and I'll do the new patch on top (I'd say dont bother to spend your time on it, I'll do it). thanks, - Joel > > > -- > Lorenzo Stoakes > https://ljs.io