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 0EEA9EE14D0 for ; Thu, 7 Sep 2023 07:11:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 94752280025; Thu, 7 Sep 2023 03:11:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8F7338E000F; Thu, 7 Sep 2023 03:11:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7C06C280025; Thu, 7 Sep 2023 03:11:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 6A2698E000F for ; Thu, 7 Sep 2023 03:11:54 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 3C26DB452B for ; Thu, 7 Sep 2023 07:11:54 +0000 (UTC) X-FDA: 81208931748.04.D58F6F1 Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com [209.85.128.53]) by imf15.hostedemail.com (Postfix) with ESMTP id 4A237A0007 for ; Thu, 7 Sep 2023 07:11:52 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=WlbqYDb4; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.53 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=1694070712; 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=W8ZbMBxHSKUFb4LWza138ts8LT1OgHugv/+Ktfy9rpw=; b=SAH2ZjiytP+HbckUepOe5Q3MNHvOS51AuRoAm2tJ7JTYKeutW87HQ4uvDIuiQwBtC/Mtaq Mg5uLmptWNDS5MfH+DPsQJ3eLPW0zUjpVWoi8CIHvvrGcPNWs/EAeguUkSz2RP9Hpa5wK0 wLaA2gN4/IV+VH7R22FOp+cRt7hI2mU= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=WlbqYDb4; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.53 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694070712; a=rsa-sha256; cv=none; b=ut3WeuC7QhG20twRvgnpaojtEPzFRzxDFG7v9Q7xfRcGQH1ycLxtr6XAeRutFk4606J2+5 y6YnZ2wgMSmeJCGEgD9TYCoBCV0BGSUZR9Kl9up+LntJFmfosGY4rPzwQT0nKbVk3QSGGm O6QsU3E9uZFl24p3X2WiT7r+Pb6Dof0= Received: by mail-wm1-f53.google.com with SMTP id 5b1f17b1804b1-401d67434daso6901935e9.2 for ; Thu, 07 Sep 2023 00:11:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694070711; x=1694675511; 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=W8ZbMBxHSKUFb4LWza138ts8LT1OgHugv/+Ktfy9rpw=; b=WlbqYDb4RLiFxOaE2Gzs+yYyQkK26FTdDbetpSKVCjDAN3nhs37WuT9yrW6YJdRf7j r0vbGSpLXsJ1x+KE6R/+n6EOMkNIGaXnWMSoDuku4aDo26bYG5cfR6tdnNsEDUyIEpcg 0uzmgW/g+C4UkBbPu9KVcZ9JiS+2GxQOwGP7Y0oEWO5dQaHPtUxAUB6cj1eAUHEXqCt7 hf/kaNU57EVMwQMk6zbRp7H1K6FfogLAqMrVwp1Rt/t+Jgm3O4gKUYtepquPUcwO1YJH aOo+D0tXwNTPC3RtozwpFiNuTCJK39Vv3+Klm75cNZOjZk72IuixfWjNpS4ainDuLmxW qUlA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1694070711; x=1694675511; 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=W8ZbMBxHSKUFb4LWza138ts8LT1OgHugv/+Ktfy9rpw=; b=X9u0jtyfZNAXTP3I5S3sW3LtXYz+TB3EXsK2U1Kx1pNfDnZ8JMGHfzi6TDcIDKpdPP uisF01SReibHKgbHZQXJu1aU05HZOrk3nNKqWbHsINycvZlveMEhqqCjLT8TjcstWtfo R655ICTshQbC64cHKCE3OFc+R/s7xjBHL9B8DmG2MopM2c71dTIwhxjjXr0PTxudKwQX 3srDvcQLBJxAFLKKhpslniluyGU+KSCMoQgLzUv/wOX14x5F4Ok70/YI6HYJhgFAYm6u z3wCmWgnaP34QYcmTJAXxRa/1ILfz6GD+gJ5MjAt9mXEY/zZxUYzGy9AkeCINKgwQYVh reKg== X-Gm-Message-State: AOJu0Yx0LiM8IguztK3wp7/bi+WSCOwkbGCLcSVhSzsxVM3lyGg1MDTt esgJwn07Fy2XYna2LSeXOGw= X-Google-Smtp-Source: AGHT+IEQDQdq3XESEPBE/V7+6OKrbU5Om9XjvaZzCyVN3J1Y2iN1rOvZqR+qIlxLMKY937YvMDtj0g== X-Received: by 2002:a05:600c:b57:b0:401:b908:85a2 with SMTP id k23-20020a05600c0b5700b00401b90885a2mr3624279wmr.23.1694070710479; Thu, 07 Sep 2023 00:11:50 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id f13-20020a7bcd0d000000b00402bda974ddsm1616156wmj.6.2023.09.07.00.11.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Sep 2023 00:11:49 -0700 (PDT) Date: Thu, 7 Sep 2023 08:11:48 +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: <499537a7-3380-4355-ae34-df7f5c0f41bd@lucifer.local> References: <20230904180806.1002832-1-joel@joelfernandes.org> <571d4a4a-0674-4c84-b714-8e7582699e30@lucifer.local> <20230905114709.GA3881391@google.com> <20230906224608.GB1646335@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230906224608.GB1646335@google.com> X-Rspamd-Queue-Id: 4A237A0007 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: ft1cppx1hnroejx68gr1g7xuzxg1gqot X-HE-Tag: 1694070712-418133 X-HE-Meta: U2FsdGVkX19sDn9jaZdPS6A4Sv4OfT413gfg73y1DWK4xTTjtPyK2a3jRS5ZniEVsYCVtlXlXDOSbCC21glxC1cUusRX3SJWaepRg0adXKqCPfm8wITGmevDc6z6L3tYZ3anJD1MzaQQcwINkkYTb72OJPJCiFXZdkQuA7cAEoIUeKmopEudIZlk8rJFFR9md7ber97Y8jEBT+Twq3GtN/TlZfdPfyRJ/7pQ4zUs1rlTy48m1X2vTeYBiWPXPw+3GQbivSEEc+xAWgDGwemN9VwDKYuaBeyuiGPXiEBCrkW057CTutI3OMgGPT2CBuAyiB0ysQYWQB+a4OIyFoDuzTyyrECzQg5XqLmgRiw39BGajM1dfXtxk1KNiKBCF1duf4DZsPP1ML/vRA8J+6kv739oewyKm9yr20PIx1PD1rhPKW3/biLrYaKzx6B3NGtV5dKxPKZIYrIu6kTPVvDzMOif8vqruInW51KJb8DTP4gXgWL08UWkWfVyRLPcmfB0NZlyvlkQ/shkUs+VBR/DQaMedAIPsIl6NC/1BEQHRdE74Pd2wSYdYLCl/UB/OY4D7aFC5IldunrYqVOyNB3YS/xEBtZPeHpsJGwU4AAk9Wa6lMPrXptmTrUQN20r5CFsUpXq5H6CaTvkwE03LEUowAskBe3nSa5Z5r7P/EMVLJTo9CZ4IyfSxum8g4rKCY+Aee/OwG4PgflQoxlCKdHhcLrkRBG+fnThL73vjeZ0KS4jGiHzHjuLHkfVhI2MLigzdX9qPUHWx+fkrBl8DwMOV2d5zD1Pa4+jYRc4pm0S5Y9QNXsHpblTquIliglQiAxTAvCUxzufI3RcuOzaOZxzElz6hQuYHcdjGBxYk05DqFUuqSYzHZmaAZoUyRb2je2vmYXPUwuqZFaM+DhT5G2UNm1O1K4IWrYrezZ0JG9GmPwytGbvjTBSXBB4VtnfgmmAAbAnIJrR15j3B4sGjQn J/j5wx4V yzv45MYV9xIr2nsnBBPHbtubyb4GofsSt7G+8wBOPxU9wPVnKwNB2gj6S5IHiDWGJM5E8qg1O+5Y4l+EvauCwMGjgFAU9Yu2MPOG6jXwudBUXQwq8/RARSGZ2Rn8ykImSj2pEhlBLmQhqIIdoGP2SSV4j1tO9lavUOf/kUbQ9q63LNjlrT/9IIFGSyKwlgl4Rs44hc+g//kfWnMlADAmOqJgT9J/21basTPMIPdgzco54PHbjJmZnhrEicYEx5LYXNBMfSDJZZQ7QqlqVG+1VfSducW0ulOlKacGvNQGODFZArXRqmpFxKFhOAjlpH4KYVhLvbLi/WM6U+iILG1nnV/Lp7utBBkWkb3QhvrWpYyennPx9dUlwElW/y3kUy4vvwnonBJz7NN6smXLLxrzMtIcjboYf2PbrDVSYPTL3qXlyFXny5iiQvNoeMd19/b2cP9TLEm06xnhWYPX5wGQTZIe0J/r/Oy2ndRQtvtYx6ob2W4lGT/rHibRr4oNXVeww+/wabRbh6Y4Mrav2R2xCe7y6X71r0Yrx7gcmqvcyA6IUQvRdACMklEAJdq7Cl+KLNJybUOtkPyQuD3A7VCiGsCS2i660lGAvKVLcG8+PnZ+EKWPSjQ9CZcbQVyd28eL/GZoGgpJMKTubvtXr8yMVMdDzPboy1gjdB7YMVVMRpXJYhy+ORoxhcLg+ak81/laCcSI8 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 10:46:08PM +0000, Joel Fernandes wrote: > 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. Thanks, yeah this may seem trivial, but I am quite sensitive about things being added to the code base that are neither described in commit msg nor in a comment or elsewhere and become 'implicit' in a sense. So just a simple comment here would be helpful, and I'm glad we're in agreement on that, will leave to you to do a follow up patch. > > 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. Yeah sorry this whole thing was rather confusing, it did indeed (unclearly) specify non-/non- in the past (on assumption dumping function would work), addition of vmalloc check now makes that correct again, the phrasing is the issue. You can leave this as-is as yeah, you're right, this was a pre-existing issue. virt_addr_valid() returns true for a slab addr, but kmem_valid_obj() is checked above so already been ruled out, now you ruled out vmalloc. Just a bit tricksy. > > 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!). Ah right, I think maybe I wasn't clear, all I meant to suggest is to output log output rather than feed anything back to caller, something like:- if (!spin_trylock(&vmap_area_lock)) { pr_cont(" [couldn't acquire vmap lock]\n"); ... } My concern is that in the past this function would only return false if it couldn't find the address in a VA, now it returns false also if you happen to call it when the spinlock is locked, which might be confusing for somebody debugging this. HOWEVER, since you now indicate that the address is vmalloc anyway, and you _absolutely cannot_ give any further details safely, perhaps this additional information is indeed not that usful. My concern was just feeling antsy that we suddenly don't do something because a lock happens to be applied but as you say that cannot be helped in certain contexts. So actually, leave this. > > 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). Ack, I was just a little frustrated we didn't reach a resolution on review (either deciding things could be deferred or having changes) before merge. Obviously fine to prioritise, but would be good to have that explicitly stated. > > thanks, > > - Joel > > > > > > Anyway, so TL;DR:- 1. As we both agree, add a comment to explain why you need the spin trylock. (there are no further steps :P) And I don't believe this actually needs any further changes after this discussion*, so if you fancy doing a follow up to that effect that will suffice for me thanks! * Though I strongly feel vmalloc as a whole needs top-to-bottom refactoring, but that's another story...