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 D3C4BEE14D3 for ; Thu, 7 Sep 2023 09:23:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 58C6C440175; Thu, 7 Sep 2023 05:23:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 53C158E000F; Thu, 7 Sep 2023 05:23:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3DCAE440175; Thu, 7 Sep 2023 05:23:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 2E8538E000F for ; Thu, 7 Sep 2023 05:23:48 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id EEFAB410A2 for ; Thu, 7 Sep 2023 09:23:47 +0000 (UTC) X-FDA: 81209264094.29.0E1A6F7 Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by imf04.hostedemail.com (Postfix) with ESMTP id E756940017 for ; Thu, 7 Sep 2023 09:23:45 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="Wh/S4sdn"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf04.hostedemail.com: domain of urezki@gmail.com designates 209.85.167.41 as permitted sender) smtp.mailfrom=urezki@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694078626; 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=8532Pq0Hp+d+66El84szs8ANQmJzGPBQuAp6uqTUXPE=; b=INm3PruZ2JhaGO8uoH9UxRQ1A9RqU3pAutSjiDj9xzmm/SduEe6D0EqHqgKJYmL/LsCuEg loUdxdC6f9MmwlKdTXAB+IwFUbaIHpxVUnggRNnQVFLWDAPgC5pvGJ1EG4DzD4knQFtLM6 pQLPLJvyLpSg0FKYBQDL5H06pNdEJqQ= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="Wh/S4sdn"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf04.hostedemail.com: domain of urezki@gmail.com designates 209.85.167.41 as permitted sender) smtp.mailfrom=urezki@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694078626; a=rsa-sha256; cv=none; b=r4Pm6wymkguoBWR39FaqlnhfI9yYkXgJ4ePlCLsl4xORoElBGrN1Rse4Vn9zCIaJbI7WZd lWF4SVvD7Ps26m67aCsJfY6kfORFlxQ1Htiavocuw4zq6t0j+bsq5I20SbWf/m4OFxhNhL iZ4DGzHipErsf6nQK/8LoLRQUMWwORo= Received: by mail-lf1-f41.google.com with SMTP id 2adb3069b0e04-500c7796d8eso1176294e87.1 for ; Thu, 07 Sep 2023 02:23:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694078624; x=1694683424; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:from:to:cc:subject:date:message-id:reply-to; bh=8532Pq0Hp+d+66El84szs8ANQmJzGPBQuAp6uqTUXPE=; b=Wh/S4sdnXSiPf2z2yyqMbawS+o31AjJU/7kjxViDePQJsI8EecpyqXHxI5xpS1ky66 HPsS6pcEkpzkT1SoWdSVYLFVeR0lBikIXoeHEOqE/9hEdz4H0imHwNnTzGwM+4dwM18C uBFU3oPDYtOsGrhDFaUtmdlsHdG18XfTqLLD0L7DOPGUJN/8DHo3MEuR5Bm8s7S9A4ag EJVVai77otJUjt/Yk0eO8DQ7mkzuujbJR0rPJXFlZMz8hbUQYfvarQ3LhxfV6v1f9ZFF s69kQiInUuVUp6lmEQSKWnHfQaoGRAoD7Evtu5BUHz+Zmr1LbBBoZ/oo+GFWNlYwlRE7 73oA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1694078624; x=1694683424; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=8532Pq0Hp+d+66El84szs8ANQmJzGPBQuAp6uqTUXPE=; b=DC6aADwprSfKW9lzU5Dug/CS9t2G4BLMAQh52sn4o8ak1r/UeTN4J7HKFjzcwS4uWa h9XWDLh2G6tPLE5k20ah6iag6Cq2FGivLF78Thlss0xezLsIG4VIVmNQpHaJ7w7gai5/ mRhVRklpPBCbGFYmMC3JhSE/ozr1FjXYuzUTBdZX3z/2jSVt5FmVAD5UJqqC1q2ofZRP ybxkUiY7eUIw4k08KdR8LrwS6lFQamZj2OfStB8o7zcmTOhWOT5X6ICq4OeLEqfupm2K rTLaPsZHhEsojq2gB9m8eVS23Y9anbbgTE3tyKNk1Q7IUwBxd7ONlbSqljAN6oKvDPjs 3t/g== X-Gm-Message-State: AOJu0YyuurnICgIVqJMkky7fT1vFAvXWy1ALoIjCumSmjVH79O3exU30 ESE8eOTx4oxewJtEoJZsWl8= X-Google-Smtp-Source: AGHT+IE/TUE721ddUKqwc/3UzFPNOXH4kRo+bamMen9urGIi6QHbzH5Mta+NhM9DYk/d01WUxwXhQw== X-Received: by 2002:ac2:57c5:0:b0:4f8:77db:1d9e with SMTP id k5-20020ac257c5000000b004f877db1d9emr3692746lfo.12.1694078623634; Thu, 07 Sep 2023 02:23:43 -0700 (PDT) Received: from pc636 (host-90-235-20-237.mobileonline.telia.com. [90.235.20.237]) by smtp.gmail.com with ESMTPSA id c26-20020ac244ba000000b004fe432108aesm3062843lfm.261.2023.09.07.02.23.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Sep 2023 02:23:43 -0700 (PDT) From: Uladzislau Rezki X-Google-Original-From: Uladzislau Rezki Date: Thu, 7 Sep 2023 11:23:40 +0200 To: Lorenzo Stoakes Cc: Joel Fernandes , 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: References: <20230904180806.1002832-1-joel@joelfernandes.org> <571d4a4a-0674-4c84-b714-8e7582699e30@lucifer.local> <20230905114709.GA3881391@google.com> <20230906224608.GB1646335@google.com> <499537a7-3380-4355-ae34-df7f5c0f41bd@lucifer.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <499537a7-3380-4355-ae34-df7f5c0f41bd@lucifer.local> X-Rspamd-Queue-Id: E756940017 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: ts3ema67mcoa64pdcmappmo6a8rx4y36 X-HE-Tag: 1694078625-168141 X-HE-Meta: U2FsdGVkX19vtZT3qumdgYo9WFIsLjMIaaGdWhg4KHTo8SjalWskvpHbRMrELrKesJn3lKYSIWjtS0Gas+2kClR2IoD5bVyDoHcQYwogIgLvp+2nw8y1GmWa+ILNRnAInsw+GmiMnHG9TArnaEN9o7skJ0LEYfCS4Y8eogrVpl5dsP3/S/lDErezIlKDHm4R/POVMQmX/HzJSy1tXIinXHeCetJYl1P7W91gpUbGN2b9vlrRzDRqcFdzMxnp5zetYsbdxWXvCbWX3ev7q9mJQL2GrS6ih3CtXSqmHn9tKp4HX/5TtYYexFsExkByTmlnQpxoEWc7C3qpOpWHfkz9EBt0ZBnoitfL0oUwJZZcPNQzXtNTbNMmMokIUyFXK7JPbvYb1kxkij+t2kY+cyDJBY7uMV5mI4QRevsQelK+tsdM+17hg1T8fKo6X3txEADmwuOH/D7J4teRzMdgA7keDBo0KPRkqZbpIxdBv361d/UWiGGhr6oIEGDAbivuEEi//+7+Zr14TBpxaaqamh8dJ62c9W93JiL0KSl4oQs0xFyXNA9fnbf68ahQwk6I41UlV8BmBV0TNPwZEKTf8q4zpPaa23DVxLVNEj3Tf2EUY5uNSU4RkQ/TsMxNY/EXbrsi9iGKm2xEGyACKHC6mJtuihVtafUG5ul/H+8gH4JSnApGaualR63LRYoDFnddp1LXsbULhOQXG1ie0TchNEAz8GJWjL0wdDh477qhj5pzQ0oFWJNOiJp9vWcxH/h484qC6L/HoB2ZmhBrrJfob2yw8ayvFhCzGoYYksS8iFI539gDH3oOdnDXxuxkAsPHRksZPxA+FrhcvEAs/cqpe1vlxOrqr27fR5DfH+b7iiggiEGVlCrYWOmEkuVZajMyHNK86KwMZsKrnN0ZUdjesvtK4oACraOgpQioPeQtBWbCvRaZKApy8z5XMAFMp/vizpCY5N2KiDXGlI6Qx+bZZ8L opmZTe6T nPfavVBrDMMjn+KC221gE1WlkeYOp8weB+G35jwOnKIgZP5FLC1t0PBcaeBHS0/kf469FTQ/5FYfY9zN1mNsM7Hnms3uL4G1Bis1oJIstcddjf3LGSc1nSlJoHURuuneR76bdfm2vype/nsrhDXd8c6QTkEJFzizBeHilxoxeo3HpjIcIVf2lX9jRTDV6+ojT1W/rKCZkZeVu018xuSofKklHUe8tGkq9RQumprQSP/I0mUPvqV50XSBWsRSQy8XUrJKjtppGR55o9M0/2jlhJp2Em/kMWfPf2xDcBEs1WR5OjSkALD5Ml2xUleWEDvYyCVpbRNLdA98DowFItJ8vrVAVP5ICsSotO1a85syOg7TqOox9od6RFPGftJyFVEOTm1gAMvilr6lErgmyOwyQj/segAoyGxNn6P71fruG7uMRY8/l3pxOJy6K4CGpHbQgrCocT0S3xwJvmrz/ZybmbjwMeuYznYhaEVkYl7kBQXCaIjAuzqufMw3kc8nCcaSTbONFrbN/AX5FQrzVZBWwpAEzYjvElBrfz3lb6cqaNNZk641qxmukv+DNbXfD9n5SObT084IRm4FxA2KrwS5up5PI59ze3IR+oMpMI+KCv0MWr7Ag31Hnq3xS+KKrWo7veU5SOyVjoZak6W8d4Qshxx0Fl75PmaWViQAXKzp0M3GHwEwZuvXI5tuzgJe9VO3F874tbWdTizqbFkqtUljhcHSOgGkBqQ3poCNbmJrvLbwQma40WtLVWvkDvg== 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 Thu, Sep 07, 2023 at 08:11:48AM +0100, Lorenzo Stoakes wrote: > 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! > For PREEMPT_RT kernels we are not allowed to use "vmap parts" in non slepable context, this is just reality, because we use a sleep type of spinlock. I am not sure how urgent we need this fix. But to me it looks like debuging and corner case. Probably i am wrong and miss something. But if it is correct, i would just bailout for RT kernel and rework later in a more proper way. For example implement a safe way of RCU scan but this is also another story. -- Uladzislau Rezki