From patchwork Sun Mar 29 02:17:19 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Morton X-Patchwork-Id: 228733 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING, SIGNED_OFF_BY, SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0D1A0C43331 for ; Sun, 29 Mar 2020 02:17:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D86A120723 for ; Sun, 29 Mar 2020 02:17:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1585448241; bh=tRVsOAbOCbtHbQCO+U+9vXHkW9wblZ3fYRMFIzNbHHI=; h=Date:From:To:Subject:In-Reply-To:List-ID:From; b=rHJHU3vBn8yRbNI+ZDyGomwjsetkPOf2zurFrMEB7gEPGSAiGIYtXfI3w6OB7PHlx VlQF6g/LnOwwXW79s6YRDK2UXvWaBjEF7pfzduTmgw2ubBkxaf/TQeUZphy7T2PqJp TUJRl8Ym3DXnI/0XzhmnxkRMyyDYbMIS2QstjQTc= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726391AbgC2CRV (ORCPT ); Sat, 28 Mar 2020 22:17:21 -0400 Received: from mail.kernel.org ([198.145.29.99]:38724 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726315AbgC2CRV (ORCPT ); Sat, 28 Mar 2020 22:17:21 -0400 Received: from localhost.localdomain (c-73-231-172-41.hsd1.ca.comcast.net [73.231.172.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 6465D2071B; Sun, 29 Mar 2020 02:17:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1585448240; bh=tRVsOAbOCbtHbQCO+U+9vXHkW9wblZ3fYRMFIzNbHHI=; h=Date:From:To:Subject:In-Reply-To:From; b=rJz4TDimY6sVs+S6jMWra39mLJtK2j7/dzGnmVd9JNTeGVROYLUFmcLcVkKBwKyHD tAygmdI4ees3bIJDmEBMEcoXQvoX6RZOKpE9OQgu57+fnFDh/UXnOTnwsyiTOkRtgY iLdQ8s3N2IccFikp+SY93ONy64PUYYJPQmXYamFw= Date: Sat, 28 Mar 2020 19:17:19 -0700 From: Andrew Morton To: akpm@linux-foundation.org, dan.j.williams@intel.com, david@redhat.com, gregkh@linuxfoundation.org, heiko.carstens@de.ibm.com, kzak@redhat.com, linux-mm@kvack.org, mhocko@kernel.org, mhocko@suse.com, mm-commits@vger.kernel.org, ndfont@gmail.com, pbadari@us.ibm.com, rafael@kernel.org, rcj@linux.vnet.ibm.com, stable@vger.kernel.org, steve.scargall@intel.com, torvalds@linux-foundation.org Subject: [patch 2/5] drivers/base/memory.c: indicate all memory blocks as removable Message-ID: <20200329021719.MBKzW0xSl%akpm@linux-foundation.org> In-Reply-To: <20200328191456.4fc0b9ca86780f26c122399e@linux-foundation.org> User-Agent: s-nail v14.8.16 Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org From: David Hildenbrand Subject: drivers/base/memory.c: indicate all memory blocks as removable We see multiple issues with the implementation/interface to compute whether a memory block can be offlined (exposed via /sys/devices/system/memory/memoryX/removable) and would like to simplify it (remove the implementation). 1. It runs basically lockless. While this might be good for performance, we see possible races with memory offlining that will require at least some sort of locking to fix. 2. Nowadays, more false positives are possible. No arch-specific checks are performed that validate if memory offlining will not be denied right away (and such check will require locking). For example, arm64 won't allow to offline any memory block that was added during boot - which will imply a very high error rate. Other archs have other constraints. 3. The interface is inherently racy. E.g., if a memory block is detected to be removable (and was not a false positive at that time), there is still no guarantee that offlining will actually succeed. So any caller already has to deal with false positives. 4. It is unclear which performance benefit this interface actually provides. The introducing commit 5c755e9fd813 ("memory-hotplug: add sysfs removable attribute for hotplug memory remove") mentioned "A user-level agent must be able to identify which sections of memory are likely to be removable before attempting the potentially expensive operation." However, no actual performance comparison was included. Known users: - lsmem: Will group memory blocks based on the "removable" property. [1] - chmem: Indirect user. It has a RANGE mode where one can specify removable ranges identified via lsmem to be offlined. However, it also has a "SIZE" mode, which allows a sysadmin to skip the manual "identify removable blocks" step. [2] - powerpc-utils: Uses the "removable" attribute to skip some memory blocks right away when trying to find some to offline+remove. However, with ballooning enabled, it already skips this information completely (because it once resulted in many false negatives). Therefore, the implementation can deal with false positives properly already. [3] According to Nathan Fontenot, DLPAR on powerpc is nowadays no longer driven from userspace via the drmgr command (powerpc-utils). Nowadays it's managed in the kernel - including onlining/offlining of memory blocks - triggered by drmgr writing to /sys/kernel/dlpar. So the affected legacy userspace handling is only active on old kernels. Only ve= ry old versions of drmgr on a new kernel (unlikely) might execute slower - totally acceptable. With CONFIG_MEMORY_HOTREMOVE, always indicating "removable" should not break any user space tool. We implement a very bad heuristic now. Withou= t CONFIG_MEMORY_HOTREMOVE we cannot offline anything, so report "not removable" as before. Original discussion can be found in [4] ("[PATCH RFC v1] mm: is_mem_section_removable() overhaul"). Other users of is_mem_section_removable() will be removed next, so that we can remove is_mem_section_removable() completely. [1] http://man7.org/linux/man-pages/man1/lsmem.1.html [2] http://man7.org/linux/man-pages/man8/chmem.8.html [3] https://github.com/ibm-power-utilities/powerpc-utils [4] https://lkml.kernel.org/r/20200117105759.27905-1-david@redhat.com Also, this patch probably fixes a crash reported by Steve. http://lkml.kernel.org/r/CAPcyv4jpdaNvJ67SkjyUJLBnBnXXQv686BiVW042g03FUmWLXw@mail.gmail.com Link: http://lkml.kernel.org/r/20200128093542.6908-1-david@redhat.com Signed-off-by: David Hildenbrand Suggested-by: Michal Hocko Acked-by: Michal Hocko Reviewed-by: Nathan Fontenot Reported-by: "Scargall, Steve" Cc: Dan Williams Cc: Greg Kroah-Hartman Cc: "Rafael J. Wysocki" Cc: Badari Pulavarty Cc: Robert Jennings Cc: Heiko Carstens Cc: Karel Zak Cc: Signed-off-by: Andrew Morton --- drivers/base/memory.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) --- a/drivers/base/memory.c~drivers-base-memoryc-indicate-all-memory-blocks-as-removable +++ a/drivers/base/memory.c @@ -97,30 +97,13 @@ static ssize_t phys_index_show(struct de } /* - * Show whether the memory block is likely to be offlineable (or is already - * offline). Once offline, the memory block could be removed. The return - * value does, however, not indicate that there is a way to remove the - * memory block. + * Legacy interface that we cannot remove. Always indicate "removable" + * with CONFIG_MEMORY_HOTREMOVE - bad heuristic. */ static ssize_t removable_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct memory_block *mem = to_memory_block(dev); - unsigned long pfn; - int ret = 1, i; - - if (mem->state != MEM_ONLINE) - goto out; - - for (i = 0; i < sections_per_block; i++) { - if (!present_section_nr(mem->start_section_nr + i)) - continue; - pfn = section_nr_to_pfn(mem->start_section_nr + i); - ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION); - } - -out: - return sprintf(buf, "%d\n", ret); + return sprintf(buf, "%d\n", (int)IS_ENABLED(CONFIG_MEMORY_HOTREMOVE)); } /* From patchwork Sun Mar 29 02:17:29 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Morton X-Patchwork-Id: 228732 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI, SIGNED_OFF_BY, SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E277CC43331 for ; Sun, 29 Mar 2020 02:17:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B25F8206E6 for ; Sun, 29 Mar 2020 02:17:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1585448251; bh=ITZ+wyQPuPawCVfy3wFhamMcldvPSgTgHYy2EgbAR+4=; h=Date:From:To:Subject:In-Reply-To:List-ID:From; b=1z/SSM2jDKWX8Z/TZnRX+fnFZTHCuBdYXHEY0UC0SbS8HFFU0cQiirXp+8XQ/node FRqzxFdZ4ecisw8EnkAieT2//fJv/eTbfwDMwVdJ8nGLEtvH8vHxBeEYRHK1IMSJwg yYnNDe2R1nB//Fiz+u5Hdy3dPvGkn9jIj5FlPR70= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727118AbgC2CRb (ORCPT ); Sat, 28 Mar 2020 22:17:31 -0400 Received: from mail.kernel.org ([198.145.29.99]:38950 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726315AbgC2CRb (ORCPT ); Sat, 28 Mar 2020 22:17:31 -0400 Received: from localhost.localdomain (c-73-231-172-41.hsd1.ca.comcast.net [73.231.172.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 85A32206DB; Sun, 29 Mar 2020 02:17:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1585448250; bh=ITZ+wyQPuPawCVfy3wFhamMcldvPSgTgHYy2EgbAR+4=; h=Date:From:To:Subject:In-Reply-To:From; b=XHsIKKRSiAqmdy8RdHout75gO+y6sHZz+decLeVuDZgfa3uMzAxeloBCFXlUhVNKA NITV5EVU46TTBdeztzW1pBtU9XJJuGcrNiViQrYoceczeeIU9F0vMxqG7bvBUdlD/0 M+5hxZOBMICbvQJ9R2Kpdlc4yKzW3frW58w3faxI= Date: Sat, 28 Mar 2020 19:17:29 -0700 From: Andrew Morton To: akpm@linux-foundation.org, aneesh.kumar@linux.ibm.com, bhe@redhat.com, dan.j.williams@intel.com, david@redhat.com, linux-mm@kvack.org, mhocko@suse.com, mm-commits@vger.kernel.org, mpe@ellerman.id.au, osalvador@suse.de, pankaj.gupta.linux@gmail.com, richard.weiyang@gmail.com, rppt@linux.ibm.com, sachinp@linux.vnet.ibm.com, stable@vger.kernel.org, torvalds@linux-foundation.org Subject: [patch 5/5] mm/sparse: fix kernel crash with pfn_section_valid check Message-ID: <20200329021729.BYOHDz5tg%akpm@linux-foundation.org> In-Reply-To: <20200328191456.4fc0b9ca86780f26c122399e@linux-foundation.org> User-Agent: s-nail v14.8.16 Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org From: "Aneesh Kumar K.V" Subject: mm/sparse: fix kernel crash with pfn_section_valid check Fix the below crash BUG: Kernel NULL pointer dereference on read at 0x00000000 Faulting instruction address: 0xc000000000c3447c Oops: Kernel access of bad area, sig: 11 [#1] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries CPU: 11 PID: 7519 Comm: lt-ndctl Not tainted 5.6.0-rc7-autotest #1 ... NIP [c000000000c3447c] vmemmap_populated+0x98/0xc0 LR [c000000000088354] vmemmap_free+0x144/0x320 Call Trace: section_deactivate+0x220/0x240 __remove_pages+0x118/0x170 arch_remove_memory+0x3c/0x150 memunmap_pages+0x1cc/0x2f0 devm_action_release+0x30/0x50 release_nodes+0x2f8/0x3e0 device_release_driver_internal+0x168/0x270 unbind_store+0x130/0x170 drv_attr_store+0x44/0x60 sysfs_kf_write+0x68/0x80 kernfs_fop_write+0x100/0x290 __vfs_write+0x3c/0x70 vfs_write+0xcc/0x240 ksys_write+0x7c/0x140 system_call+0x5c/0x68 The crash is due to NULL dereference at test_bit(idx, ms->usage->subsection_map); due to ms->usage = NULL; in pfn_section_valid() With commit d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case") section_mem_map is set to NULL after depopulate_section_mem(). This was done so that pfn_page() can work correctly with kernel config that disables SPARSEMEM_VMEMMAP. With that config pfn_to_page does __section_mem_map_addr(__sec) + __pfn; where static inline struct page *__section_mem_map_addr(struct mem_section *section) { unsigned long map = section->section_mem_map; map &= SECTION_MAP_MASK; return (struct page *)map; } Now with SPASEMEM_VMEMAP enabled, mem_section->usage->subsection_map is used to check the pfn validity (pfn_valid()). Since section_deactivate release mem_section->usage if a section is fully deactivated, pfn_valid() check after a subsection_deactivate cause a kernel crash. static inline int pfn_valid(unsigned long pfn) { ... return early_section(ms) || pfn_section_valid(ms, pfn); } where static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn) { int idx = subsection_map_index(pfn); return test_bit(idx, ms->usage->subsection_map); } Avoid this by clearing SECTION_HAS_MEM_MAP when mem_section->usage is freed. For architectures like ppc64 where large pages are used for vmmemap mapping (16MB), a specific vmemmap mapping can cover multiple sections. Hence before a vmemmap mapping page can be freed, the kernel needs to make sure there are no valid sections within that mapping. Clearing the section valid bit before depopulate_section_memap enables this. [aneesh.kumar@linux.ibm.com: add comment] Link: http://lkml.kernel.org/r/20200326133235.343616-1-aneesh.kumar@linux.ibm.comLink: http://lkml.kernel.org/r/20200325031914.107660-1-aneesh.kumar@linux.ibm.com Fixes: d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case") Signed-off-by: Aneesh Kumar K.V Reported-by: Sachin Sant Tested-by: Sachin Sant Reviewed-by: Baoquan He Acked-by: Michal Hocko Acked-by: Pankaj Gupta Reviewed-by: Wei Yang Cc: Michael Ellerman Cc: Dan Williams Cc: David Hildenbrand Cc: Oscar Salvador Cc: Mike Rapoport Cc: Signed-off-by: Andrew Morton --- mm/sparse.c | 6 ++++++ 1 file changed, 6 insertions(+) --- a/mm/sparse.c~mm-sparse-fix-kernel-crash-with-pfn_section_valid-check +++ a/mm/sparse.c @@ -781,6 +781,12 @@ static void section_deactivate(unsigned ms->usage = NULL; } memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr); + /* + * Mark the section invalid so that valid_section() + * return false. This prevents code from dereferencing + * ms->usage array. + */ + ms->section_mem_map &= ~SECTION_HAS_MEM_MAP; } if (section_is_early && memmap)