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 E4600CA0FED for ; Fri, 1 Sep 2023 16:41:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 300D8440147; Fri, 1 Sep 2023 12:41:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2B0DC8D0002; Fri, 1 Sep 2023 12:41:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 178DF440147; Fri, 1 Sep 2023 12:41:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 093958D0002 for ; Fri, 1 Sep 2023 12:41:39 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id C5F5E120379 for ; Fri, 1 Sep 2023 16:41:38 +0000 (UTC) X-FDA: 81188594676.14.C6C651D Received: from mail-vs1-f47.google.com (mail-vs1-f47.google.com [209.85.217.47]) by imf22.hostedemail.com (Postfix) with ESMTP id D92C4C0003 for ; Fri, 1 Sep 2023 16:41:36 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=joelfernandes.org header.s=google header.b=u6UuYDPU; dmarc=none; spf=pass (imf22.hostedemail.com: domain of joel@joelfernandes.org designates 209.85.217.47 as permitted sender) smtp.mailfrom=joel@joelfernandes.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1693586496; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=VmUhOcfuWLkaVOCEhVcPAHsPP8uUFstrB5IczQnTPM4=; b=LeBmSUgaeK6ErgNamkGWpjPYb2r0DHmJF4JmwbyUOAHIt/KJbgDDj+GlzxMsBL2rve8VdT QAbnEooAsBN4fY3vCLCz5b1oA+ycG9UeKkAZATbKAczSR8ntskRhdiTUA3NKdtDSuYwSzI R8nyrUV6u+fRtJoT2tvD3XS4oN1l0ng= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=joelfernandes.org header.s=google header.b=u6UuYDPU; dmarc=none; spf=pass (imf22.hostedemail.com: domain of joel@joelfernandes.org designates 209.85.217.47 as permitted sender) smtp.mailfrom=joel@joelfernandes.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1693586496; a=rsa-sha256; cv=none; b=Yl9a5zqfIjduh5eqB8zA8zm7FqCvqAKuRokmNXNdgsC1gqit/qypmWYwwfIyAgUNqI2Dq5 A6OWoLoRkp1OQROLn6mbhG06NbPZ6XpxxLEwQabEaeADgp2ueaYthtQczI/kqTgQvhLpuw PKYHYFA7M0/jOC2PlRAmeZu9JwEyGbI= Received: by mail-vs1-f47.google.com with SMTP id ada2fe7eead31-44d4c3fa5beso982007137.3 for ; Fri, 01 Sep 2023 09:41:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=joelfernandes.org; s=google; t=1693586496; x=1694191296; darn=kvack.org; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:from:to:cc:subject:date:message-id :reply-to; bh=VmUhOcfuWLkaVOCEhVcPAHsPP8uUFstrB5IczQnTPM4=; b=u6UuYDPUjZWvvI3yId1HKeFkOVuz387iEgYEWF+shOjlExq5hxTwUMKaSU0ney2ghr WgZH+DpBIS1SMAvUG+nAK5dOifO/orYcOagN/DKV/kBOPwKKMQSh5RcKPF1HZ+wnLVhK c8XFam29peNqBpN4+1taVySqirRH5SapYxbAo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693586496; x=1694191296; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=VmUhOcfuWLkaVOCEhVcPAHsPP8uUFstrB5IczQnTPM4=; b=a0srhTW+fm2DlLPIvmPsnNpt2GIR6tT8DTOV8i6vek8LdA+JDU54yLgRQNyJVTWVwc C6dRveaVdQgQILxRguk2rIED6M5LFi5ha1+/eXDS8qEU7vnkn1hq/BhpgOK3GLCzTxcC 8UxL5tgUD9NHwNUaRBwIP0jdybIjMBfC5z2VxqXxqMUF1d4WKrnqPfZ9NVwMQSar/TVv qnjgoJ/fFSiUuMma8ExkccXo7G3P16Jjv8tiUTl2fZmEPxmSnvzdoXActJPUSWUrwvbM LU3Wg65USFiAn1cYCqAvBb9qOy1NF7MzSuo8OBtx8C8zSzBJ1YwLMaleCtVPgjbFXn/6 AaYw== X-Gm-Message-State: AOJu0YzKw/Ggq7XXA183QZoINtc1gFDMgYhPYaXlZQKFbjGNFtxkFgZC 6mkMV4scHgJfvE3wV2ufUhco9A== X-Google-Smtp-Source: AGHT+IE1mXepgLrLtiIO7fYm6yoVwWRhVpPWTOGAcZsgOri3Xrt0joCy91s3n/cmvixC72/fggnv4g== X-Received: by 2002:a05:6102:3d8:b0:44d:5c61:e473 with SMTP id n24-20020a05610203d800b0044d5c61e473mr3255557vsq.22.1693586495810; Fri, 01 Sep 2023 09:41:35 -0700 (PDT) Received: from smtpclient.apple ([192.145.116.44]) by smtp.gmail.com with ESMTPSA id c6-20020ae9e206000000b00767e98535b7sm1509977qkc.67.2023.09.01.09.41.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 01 Sep 2023 09:41:35 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Joel Fernandes Mime-Version: 1.0 (1.0) Subject: Re: [PATCH v2 1/2] mm/vmalloc: Add a safer version of find_vm_area() for debug Date: Fri, 1 Sep 2023 12:41:24 -0400 Message-Id: References: Cc: linux-kernel@vger.kernel.org, Andrew Morton , Christoph Hellwig , Lorenzo Stoakes , Zhen Lei , "Paul E . McKenney" , rcu@vger.kernel.org, Zqiang , Matthew Wilcox , linux-mm@kvack.org In-Reply-To: To: Uladzislau Rezki X-Mailer: iPhone Mail (20B101) X-Rspam-User: X-Stat-Signature: bkx7hrnibic6kfnehhtcy54b8e3yszmk X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: D92C4C0003 X-HE-Tag: 1693586496-280960 X-HE-Meta: U2FsdGVkX1+biG1rUxoPfUdxi/4CbNR9f7bdupuTEducraerVaqvcIFhro/Y7LZjJURZtHqeCBWKXSKnZ4vmHsRaGh/xBurE7/I2YAgyyvWb3KkVhqgs0BR9Yu3AdydpUCProm4VSkSbJR8eOsgjKYM0S9KmYscBuEzUhUXoiDG+cGZX7Yu9vFiHXQQEhAOFFqwZ68bvmJqVVIFZxaa5ErlUCNRs0Th5/oH/PIaVn0kjFG7I/vqstd3lkCFrmREtaoX2IQAela4SnyYzEvoibUBzJPL3mnrC49YkOvgkqT1utV1QVRf7ik6mUpkKeheyid98XYuR1HbT/tseMfcdhjERaiHw20Wg4nkyaKz8pfJc1JDMtG3PRORXrnzfcCPyrIau/VkEHpVzZOuYja/74AYT/+VnHUlnT1BFM8HHvp078Tnidom2BZ5czpMXLzsjhvLkaPk/FRrzAgv50I5EiRaOnL9PtWYLtdGLe4i9bYNPmHg/jBmL4ACqnksP1xY4283kYArC+ja4TV68IJ6KJexOqh4toB8kgsPy6omp0lwkic8bCXdGkcHOZZyuOcfOylSwWuSg12fkOFmzqkAJWNizKhfyA4CMprHvG8Mi4bqrK+qM8zEBVzhH733Ompnkhnaxr2U1K+xkIIcOyznR0QwvQY60VyVgHjhloaeU9dzIm4QDrzqIMUFR5c5KwPMB8O9mDYeztPpOLmt7QC4j00LK6u7eazo0fgk/0kw6IE1he4eYFtMKrRtLmMwQukS6Xg3vLzsiOp4vHW591aToXbmk+d1PZvC3cHL0Yq00cUbQi7EA3cEgoFttbPaPEleC6Amx9yUDuJ7/jw6WpD4CcszlOOPNpMfD1HUjLdHplI092em6BlMaZtru1E6qePleQ20Mfy7tF0dSex3PGZXgnxn4Ty+Mq5sHFpuehNKErnOzEfDWhJV9iJHMCo9Oik/seJsMCqFRTvBJm/pzSl1 eQtHl7yX t3cNyszDCVhjO5rfyxWQbax1Jh4F4XWNIT6MYIs40F5dWCMuFHC8a5qArnnUo+NSG6lJHzw7XjLrXY96wB6rphWrRGKzT9QxC2YuqJUBa4+UN+Se23Q4b2vSKVQevn/MJf09y67KQQXagm88fNhwooiO2fp+fPZJC1O2LO+Ogeyf8Lqvc3bLnrMW8XcKG+W+wAiv3hfIBNMA+CCJ0YcTomnrJYb3aE4jHvHL9HmYecWzdO6RIZZr3sA7i20BymcTrOqcVYR5/CG9+0QHezqlOsOfFJPRjnx46uTd/S/VbTRdEG0DWdvJPPRukzMxN7VxJGmPlRGLXYKSK0JbNB3eKhoR3syMABxhMRRLBRFj/PvCsz2qLBf8Q/gKrkjJycEHvQyfYL2M5xSuj5mhkvBE4UBoNDgeUeKSXBTmtXnAMOXijZl3xXQ1+tVWeZ5+jjkwIMmG6zZzF1crj69M6H/pWEigwkX7FwqvkxKWjrPAEdaTRlKWBhlhRyR7AF9LidWXZF3kIj0Xz7fPdVhep/swv/24Pn6FZY48Fs9p5EVrS68c0BOaa8iMRBKSvV7ICjKdFYJlYg8BkeJ8b43L71OIcOS/79EDB7jiZsrxphK2QyiQH1aqcnOyikCkkWAgsDpdE2YaUpoiIlKqsE6mAlv8m0q64PgQqtkhmDNFhKeR+DD/6XED2LzoWKPK8nAgMy5gxpDzJOdCkZPp+FXJSnCrsKJo2AS1bDkuJNkStZS1cxAN+zQ40RNxdvA2ryGqoFYGB5r5a 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 Sep 1, 2023, at 8:48 AM, Uladzislau Rezki wrote: >=20 > =EF=BB=BFOn Fri, Sep 01, 2023 at 12:33:21AM +0000, Joel Fernandes wrote: >>> On Fri, Sep 01, 2023 at 12:19:17AM +0000, Joel Fernandes wrote: >>> On Thu, Aug 31, 2023 at 09:47:52PM +0200, Uladzislau Rezki wrote: >>>> On Thu, Aug 31, 2023 at 05:18:25PM +0000, Joel Fernandes (Google) wrote= : >>>>> It is unsafe to dump vmalloc area information when trying to do so fro= m >>>>> 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(). >>>>>=20 >>>>> [apply test robot feedback on unused function fix.] >>>>>=20 >>>>> Reported-by: Zhen Lei >>>>> Cc: Paul E. McKenney >>>>> Cc: rcu@vger.kernel.org >>>>> Cc: Zqiang >>>>> Reviewed-by: Matthew Wilcox (Oracle) >>>>> Signed-off-by: Joel Fernandes (Google) >>>>> --- >>>>> v1->v2: Apply review tags and test robot feedback. >>>>>=20 >>>>> mm/vmalloc.c | 39 ++++++++++++++++++++++++++++++++++++++- >>>>> 1 file changed, 38 insertions(+), 1 deletion(-) >>>>>=20 >>>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >>>>> index 93cf99aba335..f09e882ae3b8 100644 >>>>> --- a/mm/vmalloc.c >>>>> +++ b/mm/vmalloc.c >>>>> @@ -1865,6 +1865,20 @@ struct vmap_area *find_vmap_area(unsigned long a= ddr) >>>>> return va; >>>>> } >>>>>=20 >>>>> +#ifdef CONFIG_PRINTK >>>>> +static struct vmap_area *find_vmap_area_trylock(unsigned long addr) >>>>> +{ >>>>> + struct vmap_area *va; >>>>> + >>>>> + if (!spin_trylock(&vmap_area_lock)) >>>>> + return NULL; >>>>> + va =3D __find_vmap_area(addr, &vmap_area_root); >>>>> + spin_unlock(&vmap_area_lock); >>>>> + >>>>> + return va; >>>>> +} >>>>> +#endif >>>>> + >>>>> static struct vmap_area *find_unlink_vmap_area(unsigned long addr) >>>>> { >>>>> struct vmap_area *va; >>>>> @@ -2671,6 +2685,29 @@ struct vm_struct *find_vm_area(const void *addr= ) >>>>> return va->vm; >>>>> } >>>>>=20 >>>>> +/** >>>>> + * try_to_find_vm_area - find a continuous kernel virtual area >>>>> + * @addr: base address >>>>> + * >>>>> + * This function is the same as find_vm_area() except that it is >>>>> + * safe to call if vmap_area_lock is already held and returns NULL >>>>> + * if it is. See comments in find_vmap_area() for other details. >>>>> + * >>>>> + * Return: the area descriptor on success or %NULL on failure. >>>>> + */ >>>>> +#ifdef CONFIG_PRINTK >>>>> +static struct vm_struct *try_to_find_vm_area(const void *addr) >>>>> +{ >>>>> + struct vmap_area *va; >>>>> + >>>>> + va =3D find_vmap_area_trylock((unsigned long)addr); >>>>> + if (!va) >>>>> + return NULL; >>>>> + >>>>> + return va->vm; >>>>> +} >>>>> +#endif >>>>> + >>>>> /** >>>>> * remove_vm_area - find and remove a continuous kernel virtual area >>>>> * @addr: base address >>>>> @@ -4277,7 +4314,7 @@ bool vmalloc_dump_obj(void *object) >>>>> struct vm_struct *vm; >>>>> void *objp =3D (void *)PAGE_ALIGN((unsigned long)object); >>>>>=20 >>>>> - vm =3D find_vm_area(objp); >>>>> + vm =3D try_to_find_vm_area(objp); >>>>> if (!vm) >>>>> return false; >>>>> pr_cont(" %u-page vmalloc region starting at %#lx allocated at %pS\= n", >>>=20 >>> Hi Vlad, >>> Thanks for taking a look. >>>=20 >>>> I am not sure if this patch makes a lot of sense. I agree, this is a >>>> problem and it mitigates it. But it is broken in terms of once you drop= >>>> the lock, the VA should not be accessed. >>>=20 >>> Just to note the lockless-access issue you are referring to is not intro= duced >>> by this patch but is rather in the existing code. Also just to note this= is >>> debug code. >>>=20 >>>> Is that a real issue or it gets triggered due to some syntetic test cas= e? >>>=20 >>> It is a real issue. See 2/2. >>>=20 >>>> If i were you, i would go with open-coded version of trylock. Because >>>> there is only one user so far. >>>=20 >>> Taking your open coding and locking suggestions, I came up with the belo= w >>> which actually results in a smaller patch. Does it look good to you? >>>=20 >>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >>> index 93cf99aba335..aaf6bad997a7 100644 >>=20 >> And with some trivial compiler errors fixed (sorry should have build test= ed >> but wanted to just share the idea earlier): >>=20 >> 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, i= nt nr_vms) >> #ifdef CONFIG_PRINTK >> bool vmalloc_dump_obj(void *object) >> { >> - struct vm_struct *vm; >> void *objp =3D (void *)PAGE_ALIGN((unsigned long)object); >> + const void *caller; >> + struct vm_struct *vm; >> + struct vmap_area *va; >> + unsigned long addr; >> + unsigned int nr_pages; >>=20 >> - vm =3D find_vm_area(objp); >> - if (!vm) >> + if (!spin_trylock(&vmap_area_lock)) >> + return false; >> + va =3D __find_vmap_area((unsigned long)objp, &vmap_area_root); >> + if (!va) { >> + spin_unlock(&vmap_area_lock); >> return false; >> + } >> + >> + vm =3D va->vm; >> + if (!vm) { >> + spin_unlock(&vmap_area_lock); >> + return false; >> + } >> + addr =3D (unsigned long)vm->addr; >> + caller =3D vm->caller; >> + nr_pages =3D 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 >>=20 > Looks good to me and thank you for fixing a locking issue :) > I think you will re-spin and resend it one more time? Yes. May I add your Reviewed-by tag to both patches after re-spinning as men= tioned above? thanks! - Joel >=20 > -- > Uladzislau Rezki