Message ID | 20230717115150.1806954-1-mawupeng1@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c923:0:b0:3e4:2afc:c1 with SMTP id j3csp1075664vqt; Mon, 17 Jul 2023 05:23:59 -0700 (PDT) X-Google-Smtp-Source: APBJJlHwxyTFf74XscIziPjAaKqofeyx/Rkgq1XRH6MC+guP/5aphN7QUYTz+CworwdaZH/inZ12 X-Received: by 2002:aa7:c90f:0:b0:51e:54ad:ba8f with SMTP id b15-20020aa7c90f000000b0051e54adba8fmr12979299edt.24.1689596639391; Mon, 17 Jul 2023 05:23:59 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689596639; cv=none; d=google.com; s=arc-20160816; b=FWLeg9TJJDvR81QxiYw1RUWnrMXld8g36VJeuCS8QZEwgOlQmkIgE6/YWtJw8a3cAT G1TfEK6iH8qkLN4+EFQPXngiPRTI34O7m3mwUm+b+v7u2Hd/x5TSaNfMYawoAUF7MQsN mxUY4OgJMGaFLA0s8w2nyaLJFo9VBPKnbigX6gZ7wQFB53uXyPYN59dFEX5/MpzqO1eW BpuO1JDBYt0Y0tND0wJ6p+6K5TrS4xFzQMX2cpp+VQ3+n4QFb9qzaKDAJu8ypBdKnmzt 6Y0ZTjgW+5bNGvHsH1YRAyfuWf82/vobh1tx38v74FsGBaq+5SKgkqqkCEB5K3UupB0W 2gmw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=jCHoCKapbskDxs00RFzUsH2Dc5z/Rza4VF/e2/6y96U=; fh=gYLHcGVH+7d4wa2WDl4n1tOhXyv6JiBTzDKd+sTfGgY=; b=uyHXxcbc/bOxBfKUZuUcX+zzQ6SbY6++MmwDCRpotunxQ3u5Tekabxy9TcRIPhPUIq CpmgdEF3jFz7W0Z8qVdMFDwFAgu9QwH8lGnd0PNJTtk6Ha7bvPOeYYa9OSyWwINnFr6R ml0+Typfg0tpe4MevGLnv8NGJD95QmYybqW8IKJI0KRuj+KGANK0BMZaJ3TD2cg6WUC6 iWUvnW6bexh90UcZyeEANrtKyFClzc9zHMLAaqviCHIVsJRaF/L0yQYJrjU8eHzQnWAI Hrm8L0DBQiw8NZwGyY3WT7LodK5iR5a89iR2EKujanQhF1BnUuk0lxoQPyIbWjoi2h5z JXOQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t14-20020a05640203ce00b005219817ce6asi818194edw.676.2023.07.17.05.23.35; Mon, 17 Jul 2023 05:23:59 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230017AbjGQLwB (ORCPT <rfc822;hadasmailinglist@gmail.com> + 99 others); Mon, 17 Jul 2023 07:52:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58944 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229564AbjGQLwA (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 17 Jul 2023 07:52:00 -0400 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 884B597 for <linux-kernel@vger.kernel.org>; Mon, 17 Jul 2023 04:51:57 -0700 (PDT) Received: from dggpemm500014.china.huawei.com (unknown [172.30.72.55]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4R4L5F6xJDz18LlC; Mon, 17 Jul 2023 19:51:13 +0800 (CST) Received: from localhost.localdomain (10.175.112.125) by dggpemm500014.china.huawei.com (7.185.36.153) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Mon, 17 Jul 2023 19:51:54 +0800 From: Wupeng Ma <mawupeng1@huawei.com> To: <catalin.marinas@arm.com>, <akpm@linux-foundation.org>, <sudaraja@codeaurora.org> CC: <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, <mawupeng1@huawei.com>, <wangkefeng.wang@huawei.com>, <will@kernel.org>, <linux-arm-kernel@lists.infradead.org>, <mark.rutland@arm.com>, <anshuman.khandual@arm.com> Subject: [RFC PATCH] arm64: mm: Fix kernel page tables incorrectly deleted during memory removal Date: Mon, 17 Jul 2023 19:51:50 +0800 Message-ID: <20230717115150.1806954-1-mawupeng1@huawei.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.175.112.125] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To dggpemm500014.china.huawei.com (7.185.36.153) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771670485602709147 X-GMAIL-MSGID: 1771670485602709147 |
Series |
[RFC] arm64: mm: Fix kernel page tables incorrectly deleted during memory removal
|
|
Commit Message
mawupeng
July 17, 2023, 11:51 a.m. UTC
From: Ma Wupeng <mawupeng1@huawei.com> During our test, we found that kernel page table may be unexpectedly cleared with rodata off. The root cause is that the kernel page is initialized with pud size(1G block mapping) while offline is memory block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, when offline a memory block, the call trace is shown below, offline_and_remove_memory try_remove_memory arch_remove_memory __remove_pgd_mapping unmap_hotplug_range unmap_hotplug_p4d_range unmap_hotplug_pud_range if (pud_sect(pud)) pud_clear(pudp); There is no issue for block mapping with pmd level(2M) because the memory block size is aligned with 2M. Commit f0b13ee23241 ("arm64/sparsemem: reduce SECTION_SIZE_BITS") reduces SECTION_SIZE_BITS from arm64, this make memory section size less than pud size possible. Since only hotadded memory can be removed for arm64 due to commit bbd6ec605c0f ("arm64/mm: Enable memory hot remove"), stop using pud size kernel page entry during memory hot join can fix this. Fixes: f0b13ee23241 ("arm64/sparsemem: reduce SECTION_SIZE_BITS") Signed-off-by: Ma Wupeng <mawupeng1@huawei.com> --- arch/arm64/mm/mmu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Comments
On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: > From: Ma Wupeng <mawupeng1@huawei.com> > > During our test, we found that kernel page table may be unexpectedly > cleared with rodata off. The root cause is that the kernel page is > initialized with pud size(1G block mapping) while offline is memory > block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, > when offline a memory block, the call trace is shown below, > > offline_and_remove_memory > try_remove_memory > arch_remove_memory > __remove_pgd_mapping > unmap_hotplug_range > unmap_hotplug_p4d_range > unmap_hotplug_pud_range > if (pud_sect(pud)) > pud_clear(pudp); Sorry, but I'm struggling to understand the problem here. If we're adding and removing a 2G memory region, why _wouldn't_ we want to use large 1GiB mappings? Or are you saying that only a subset of the memory is removed, but we then accidentally unmap the whole thing? > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 95d360805f8a..44c724ce4f70 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -44,6 +44,7 @@ > #define NO_BLOCK_MAPPINGS BIT(0) > #define NO_CONT_MAPPINGS BIT(1) > #define NO_EXEC_MAPPINGS BIT(2) /* assumes FEAT_HPDS is not used */ > +#define NO_PUD_MAPPINGS BIT(3) > > int idmap_t0sz __ro_after_init; > > @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end, > */ > if (pud_sect_supported() && > ((addr | next | phys) & ~PUD_MASK) == 0 && > - (flags & NO_BLOCK_MAPPINGS) == 0) { > + (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) { > pud_set_huge(pudp, phys, prot); > > /* > @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void) > int arch_add_memory(int nid, u64 start, u64 size, > struct mhp_params *params) > { > - int ret, flags = NO_EXEC_MAPPINGS; > + int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS; I think we should allow large mappings here and instead prevent partial removal of the block, if that's what is causing the issue. Will
On 2023/7/21 18:36, Will Deacon wrote: > On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >> From: Ma Wupeng <mawupeng1@huawei.com> >> >> During our test, we found that kernel page table may be unexpectedly >> cleared with rodata off. The root cause is that the kernel page is >> initialized with pud size(1G block mapping) while offline is memory >> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >> when offline a memory block, the call trace is shown below, >> >> offline_and_remove_memory >> try_remove_memory >> arch_remove_memory >> __remove_pgd_mapping >> unmap_hotplug_range >> unmap_hotplug_p4d_range >> unmap_hotplug_pud_range >> if (pud_sect(pud)) >> pud_clear(pudp); > > Sorry, but I'm struggling to understand the problem here. If we're adding > and removing a 2G memory region, why _wouldn't_ we want to use large 1GiB > mappings? > Or are you saying that only a subset of the memory is removed, > but we then accidentally unmap the whole thing? Yes, umap a subset but the whole thing page table entry is removed. > >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index 95d360805f8a..44c724ce4f70 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -44,6 +44,7 @@ >> #define NO_BLOCK_MAPPINGS BIT(0) >> #define NO_CONT_MAPPINGS BIT(1) >> #define NO_EXEC_MAPPINGS BIT(2) /* assumes FEAT_HPDS is not used */ >> +#define NO_PUD_MAPPINGS BIT(3) >> >> int idmap_t0sz __ro_after_init; >> >> @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end, >> */ >> if (pud_sect_supported() && >> ((addr | next | phys) & ~PUD_MASK) == 0 && >> - (flags & NO_BLOCK_MAPPINGS) == 0) { >> + (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) { >> pud_set_huge(pudp, phys, prot); >> >> /* >> @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void) >> int arch_add_memory(int nid, u64 start, u64 size, >> struct mhp_params *params) >> { >> - int ret, flags = NO_EXEC_MAPPINGS; >> + int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS; > > I think we should allow large mappings here and instead prevent partial > removal of the block, if that's what is causing the issue. This could solve this problem. Or we can prevent partial removal? Or rebulid page table entry which is not removed? > > Will
On 7/24/23 06:55, mawupeng wrote: > > On 2023/7/21 18:36, Will Deacon wrote: >> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >>> From: Ma Wupeng <mawupeng1@huawei.com> >>> >>> During our test, we found that kernel page table may be unexpectedly >>> cleared with rodata off. The root cause is that the kernel page is >>> initialized with pud size(1G block mapping) while offline is memory >>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >>> when offline a memory block, the call trace is shown below, >>> >>> offline_and_remove_memory >>> try_remove_memory >>> arch_remove_memory >>> __remove_pgd_mapping >>> unmap_hotplug_range >>> unmap_hotplug_p4d_range >>> unmap_hotplug_pud_range >>> if (pud_sect(pud)) >>> pud_clear(pudp); >> Sorry, but I'm struggling to understand the problem here. If we're adding >> and removing a 2G memory region, why _wouldn't_ we want to use large 1GiB >> mappings? > >> Or are you saying that only a subset of the memory is removed, >> but we then accidentally unmap the whole thing? > Yes, umap a subset but the whole thing page table entry is removed. > >>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >>> index 95d360805f8a..44c724ce4f70 100644 >>> --- a/arch/arm64/mm/mmu.c >>> +++ b/arch/arm64/mm/mmu.c >>> @@ -44,6 +44,7 @@ >>> #define NO_BLOCK_MAPPINGS BIT(0) >>> #define NO_CONT_MAPPINGS BIT(1) >>> #define NO_EXEC_MAPPINGS BIT(2) /* assumes FEAT_HPDS is not used */ >>> +#define NO_PUD_MAPPINGS BIT(3) >>> >>> int idmap_t0sz __ro_after_init; >>> >>> @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end, >>> */ >>> if (pud_sect_supported() && >>> ((addr | next | phys) & ~PUD_MASK) == 0 && >>> - (flags & NO_BLOCK_MAPPINGS) == 0) { >>> + (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) { >>> pud_set_huge(pudp, phys, prot); >>> >>> /* >>> @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void) >>> int arch_add_memory(int nid, u64 start, u64 size, >>> struct mhp_params *params) >>> { >>> - int ret, flags = NO_EXEC_MAPPINGS; >>> + int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS; >> I think we should allow large mappings here and instead prevent partial >> removal of the block, if that's what is causing the issue. > This could solve this problem. > Or we can prevent partial removal? Or rebulid page table entry which is not removed? + David Hildenbrand <david@redhat.com> Splitting the block mapping and rebuilding page table entry to reflect non-removed areas will require additional information such as flags and pgtable alloc function as in __create_pgd_mapping(), which need to be passed along, depending on whether it's tearing down vmemmap (would not have PUD block map) or linear mapping. But I am just wondering if we have to go in that direction at all or just prevent partial memory block removal as suggested by Will. - arch_remove_memory() does not have return type, core MM hotremove would not fail because arch_remove_memory() failed or warned - core MM hotremove does check_hotplug_memory_range() which ensures the range and start address are memory_block_size_bytes() aligned - Default memory_block_size_bytes() is dependent on SECTION_SIZE_BITS which on arm64 now can be less than PUD_SIZE triggering this problem. #define MIN_MEMORY_BLOCK_SIZE (1UL << SECTION_SIZE_BITS) unsigned long __weak memory_block_size_bytes(void) { return MIN_MEMORY_BLOCK_SIZE; } EXPORT_SYMBOL_GPL(memory_block_size_bytes); - We would need to override memory_block_size_bytes() on arm64 to accommodate such scenarios here Something like this might work (built but not tested) commit 2eb8dc0d08dfe0b2a3bb71df93b12f7bf74a2ca6 (HEAD) Author: Anshuman Khandual <anshuman.khandual@arm.com> Date: Mon Jul 24 06:45:34 2023 +0100 arm64/mm: Define memory_block_size_bytes() Define memory_block_size_bytes() on arm64 platforms to set minimum hot plug and remove granularity as PUD_SIZE in case where MIN_MEMORY_BLOCK_SIZE just falls below PUD_SIZE. Otherwise a complete PUD block mapping will be teared down while unmapping MIN_MEMORY_BLOCK_SIZE range. Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 95d360805f8a..1918459b3460 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -1157,6 +1157,17 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, } #ifdef CONFIG_MEMORY_HOTPLUG +unsigned long memory_block_size_bytes(void) +{ + /* + * Linear mappings might include PUD based block mappings which + * cannot be teared down in part during memory hotremove. Hence + * PUD_SIZE needs to be the minimum granularity, for memory hot + * removal in case MIN_MEMORY_BLOCK_SIZE falls below. + */ + return max_t(unsigned long, MIN_MEMORY_BLOCK_SIZE, PUD_SIZE); +} + void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *altmap) {
On 24.07.23 07:54, Anshuman Khandual wrote: > > > On 7/24/23 06:55, mawupeng wrote: >> >> On 2023/7/21 18:36, Will Deacon wrote: >>> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >>>> From: Ma Wupeng <mawupeng1@huawei.com> >>>> >>>> During our test, we found that kernel page table may be unexpectedly >>>> cleared with rodata off. The root cause is that the kernel page is >>>> initialized with pud size(1G block mapping) while offline is memory >>>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >>>> when offline a memory block, the call trace is shown below, Is someone adding memory in 2 GiB granularity and then removing parts of it in 128 MiB granularity? That would be against what we support using the add_memory() / offline_and_remove_memory() API and that driver should be fixed instead. Or does this trigger only when a hotplugged memory block falls into the same 2 GiB area as boot memory? >>>> >>>> offline_and_remove_memory >>>> try_remove_memory >>>> arch_remove_memory >>>> __remove_pgd_mapping >>>> unmap_hotplug_range >>>> unmap_hotplug_p4d_range >>>> unmap_hotplug_pud_range >>>> if (pud_sect(pud)) >>>> pud_clear(pudp); Which drivers triggers that? In-tree is only virtio-mem and dax/kmem. Both add and remove memory in the same granularity. For example, virtio-mem will only call add_memory(memory_block_size()) to then offline_and_remove_memory(memory_block_size()). Could that trigger it as well? >>> Sorry, but I'm struggling to understand the problem here. If we're adding >>> and removing a 2G memory region, why _wouldn't_ we want to use large 1GiB >>> mappings? >> >>> Or are you saying that only a subset of the memory is removed, >>> but we then accidentally unmap the whole thing? >> Yes, umap a subset but the whole thing page table entry is removed. >> Can we have some more details about the user and how to trigger it? >>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >>>> index 95d360805f8a..44c724ce4f70 100644 >>>> --- a/arch/arm64/mm/mmu.c >>>> +++ b/arch/arm64/mm/mmu.c >>>> @@ -44,6 +44,7 @@ >>>> #define NO_BLOCK_MAPPINGS BIT(0) >>>> #define NO_CONT_MAPPINGS BIT(1) >>>> #define NO_EXEC_MAPPINGS BIT(2) /* assumes FEAT_HPDS is not used */ >>>> +#define NO_PUD_MAPPINGS BIT(3) >>>> >>>> int idmap_t0sz __ro_after_init; >>>> >>>> @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end, >>>> */ >>>> if (pud_sect_supported() && >>>> ((addr | next | phys) & ~PUD_MASK) == 0 && >>>> - (flags & NO_BLOCK_MAPPINGS) == 0) { >>>> + (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) { >>>> pud_set_huge(pudp, phys, prot); >>>> >>>> /* >>>> @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void) >>>> int arch_add_memory(int nid, u64 start, u64 size, >>>> struct mhp_params *params) >>>> { >>>> - int ret, flags = NO_EXEC_MAPPINGS; >>>> + int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS; >>> I think we should allow large mappings here and instead prevent partial >>> removal of the block, if that's what is causing the issue. >> This could solve this problem. >> Or we can prevent partial removal? Or rebulid page table entry which is not removed? > > + David Hildenbrand <david@redhat.com> > > Splitting the block mapping and rebuilding page table entry to reflect non-removed > areas will require additional information such as flags and pgtable alloc function > as in __create_pgd_mapping(), which need to be passed along, depending on whether > it's tearing down vmemmap (would not have PUD block map) or linear mapping. But I > am just wondering if we have to go in that direction at all or just prevent partial > memory block removal as suggested by Will. > > - arch_remove_memory() does not have return type, core MM hotremove would not fail > because arch_remove_memory() failed or warned > > - core MM hotremove does check_hotplug_memory_range() which ensures the range and > start address are memory_block_size_bytes() aligned > > - Default memory_block_size_bytes() is dependent on SECTION_SIZE_BITS which on arm64 > now can be less than PUD_SIZE triggering this problem. > > #define MIN_MEMORY_BLOCK_SIZE (1UL << SECTION_SIZE_BITS) > > unsigned long __weak memory_block_size_bytes(void) > { > return MIN_MEMORY_BLOCK_SIZE; > } > EXPORT_SYMBOL_GPL(memory_block_size_bytes); > > - We would need to override memory_block_size_bytes() on arm64 to accommodate such > scenarios here > > Something like this might work (built but not tested) > > commit 2eb8dc0d08dfe0b2a3bb71df93b12f7bf74a2ca6 (HEAD) > Author: Anshuman Khandual <anshuman.khandual@arm.com> > Date: Mon Jul 24 06:45:34 2023 +0100 > > arm64/mm: Define memory_block_size_bytes() > > Define memory_block_size_bytes() on arm64 platforms to set minimum hot plug > and remove granularity as PUD_SIZE in case where MIN_MEMORY_BLOCK_SIZE just > falls below PUD_SIZE. Otherwise a complete PUD block mapping will be teared > down while unmapping MIN_MEMORY_BLOCK_SIZE range. > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 95d360805f8a..1918459b3460 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -1157,6 +1157,17 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, > } > > #ifdef CONFIG_MEMORY_HOTPLUG > +unsigned long memory_block_size_bytes(void) > +{ > + /* > + * Linear mappings might include PUD based block mappings which > + * cannot be teared down in part during memory hotremove. Hence > + * PUD_SIZE needs to be the minimum granularity, for memory hot > + * removal in case MIN_MEMORY_BLOCK_SIZE falls below. > + */ > + return max_t(unsigned long, MIN_MEMORY_BLOCK_SIZE, PUD_SIZE); > +} > + > void vmemmap_free(unsigned long start, unsigned long end, > struct vmem_altmap *altmap) > { > OH god no. That would seriously degrade memory hotplug capabilities in virtual environments (especially, virtio-mem and DIMMS). If someone adds memory in 128 MiB chunks and removes memory in 128 MiB chunks, that has to be working. Removing boot memory is blocked via register_memory_notifier(&prevent_bootmem_remove_nb);
On 2023/7/24 14:11, David Hildenbrand wrote: > On 24.07.23 07:54, Anshuman Khandual wrote: >> >> >> On 7/24/23 06:55, mawupeng wrote: >>> >>> On 2023/7/21 18:36, Will Deacon wrote: >>>> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >>>>> From: Ma Wupeng <mawupeng1@huawei.com> >>>>> >>>>> During our test, we found that kernel page table may be unexpectedly >>>>> cleared with rodata off. The root cause is that the kernel page is >>>>> initialized with pud size(1G block mapping) while offline is memory >>>>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >>>>> when offline a memory block, the call trace is shown below, > > Is someone adding memory in 2 GiB granularity and then removing parts of it in 128 MiB granularity? That would be against what we support using the add_memory() / offline_and_remove_memory() API and that driver should be fixed instead. Yes, this kind of situation. The problem occurs in the following scenarios: 1. use mem=xxG to reserve memory. 2. add_momory to online memory. 3. offline part of the memroy via offline_and_remove_memory. During my research, ACPI memory removal use memory_subsys_offline to offline memory section and this will not delete page table entry which do not trigger this kind of problem. So I understand what you are talking about. 1. 3rd-party driver shouldn't use add_memory/offline_and_remove_memory to online/offline memory. If it have to use, this can be achieved by driver. 2. memory_subsys_offline is perfered to do such thing. Should we update the doc to describe this kind of limitation? > > Or does this trigger only when a hotplugged memory block falls into the same 2 GiB area as boot memor> >>>>> >>>>> offline_and_remove_memory >>>>> try_remove_memory >>>>> arch_remove_memory >>>>> __remove_pgd_mapping >>>>> unmap_hotplug_range >>>>> unmap_hotplug_p4d_range >>>>> unmap_hotplug_pud_range >>>>> if (pud_sect(pud)) >>>>> pud_clear(pudp); > > Which drivers triggers that? In-tree is only virtio-mem and dax/kmem. Both add and remove memory in the same granularity. It is 3rd-party driver. which use try to offline part of(128M) movable memory and this lead to the problem. > > For example, virtio-mem will only call add_memory(memory_block_size()) to then offline_and_remove_memory(memory_block_size()). > > Could that trigger it as well? > >>>> Sorry, but I'm struggling to understand the problem here. If we're adding >>>> and removing a 2G memory region, why _wouldn't_ we want to use large 1GiB >>>> mappings? >>> >>>> Or are you saying that only a subset of the memory is removed, >>>> but we then accidentally unmap the whole thing? >>> Yes, umap a subset but the whole thing page table entry is removed. >>> > > Can we have some more details about the user and how to trigger it? > >>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >>>>> index 95d360805f8a..44c724ce4f70 100644 >>>>> --- a/arch/arm64/mm/mmu.c >>>>> +++ b/arch/arm64/mm/mmu.c >>>>> @@ -44,6 +44,7 @@ >>>>> #define NO_BLOCK_MAPPINGS BIT(0) >>>>> #define NO_CONT_MAPPINGS BIT(1) >>>>> #define NO_EXEC_MAPPINGS BIT(2) /* assumes FEAT_HPDS is not used */ >>>>> +#define NO_PUD_MAPPINGS BIT(3) >>>>> int idmap_t0sz __ro_after_init; >>>>> @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end, >>>>> */ >>>>> if (pud_sect_supported() && >>>>> ((addr | next | phys) & ~PUD_MASK) == 0 && >>>>> - (flags & NO_BLOCK_MAPPINGS) == 0) { >>>>> + (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) { >>>>> pud_set_huge(pudp, phys, prot); >>>>> /* >>>>> @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void) >>>>> int arch_add_memory(int nid, u64 start, u64 size, >>>>> struct mhp_params *params) >>>>> { >>>>> - int ret, flags = NO_EXEC_MAPPINGS; >>>>> + int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS; >>>> I think we should allow large mappings here and instead prevent partial >>>> removal of the block, if that's what is causing the issue. >>> This could solve this problem. >>> Or we can prevent partial removal? Or rebulid page table entry which is not removed? >> >> + David Hildenbrand <david@redhat.com> >> >> Splitting the block mapping and rebuilding page table entry to reflect non-removed >> areas will require additional information such as flags and pgtable alloc function >> as in __create_pgd_mapping(), which need to be passed along, depending on whether >> it's tearing down vmemmap (would not have PUD block map) or linear mapping. But I >> am just wondering if we have to go in that direction at all or just prevent partial >> memory block removal as suggested by Will. >> >> - arch_remove_memory() does not have return type, core MM hotremove would not fail >> because arch_remove_memory() failed or warned >> >> - core MM hotremove does check_hotplug_memory_range() which ensures the range and >> start address are memory_block_size_bytes() aligned >> >> - Default memory_block_size_bytes() is dependent on SECTION_SIZE_BITS which on arm64 >> now can be less than PUD_SIZE triggering this problem. >> >> #define MIN_MEMORY_BLOCK_SIZE (1UL << SECTION_SIZE_BITS) >> >> unsigned long __weak memory_block_size_bytes(void) >> { >> return MIN_MEMORY_BLOCK_SIZE; >> } >> EXPORT_SYMBOL_GPL(memory_block_size_bytes); >> >> - We would need to override memory_block_size_bytes() on arm64 to accommodate such >> scenarios here >> >> Something like this might work (built but not tested) >> >> commit 2eb8dc0d08dfe0b2a3bb71df93b12f7bf74a2ca6 (HEAD) >> Author: Anshuman Khandual <anshuman.khandual@arm.com> >> Date: Mon Jul 24 06:45:34 2023 +0100 >> >> arm64/mm: Define memory_block_size_bytes() >> Define memory_block_size_bytes() on arm64 platforms to set minimum hot plug >> and remove granularity as PUD_SIZE in case where MIN_MEMORY_BLOCK_SIZE just >> falls below PUD_SIZE. Otherwise a complete PUD block mapping will be teared >> down while unmapping MIN_MEMORY_BLOCK_SIZE range. >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index 95d360805f8a..1918459b3460 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -1157,6 +1157,17 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, >> } >> #ifdef CONFIG_MEMORY_HOTPLUG >> +unsigned long memory_block_size_bytes(void) >> +{ >> + /* >> + * Linear mappings might include PUD based block mappings which >> + * cannot be teared down in part during memory hotremove. Hence >> + * PUD_SIZE needs to be the minimum granularity, for memory hot >> + * removal in case MIN_MEMORY_BLOCK_SIZE falls below. >> + */ >> + return max_t(unsigned long, MIN_MEMORY_BLOCK_SIZE, PUD_SIZE); >> +} >> + >> void vmemmap_free(unsigned long start, unsigned long end, >> struct vmem_altmap *altmap) >> { >> > > OH god no. That would seriously degrade memory hotplug capabilities in virtual environments (especially, virtio-mem and DIMMS). > > If someone adds memory in 128 MiB chunks and removes memory in 128 MiB chunks, that has to be working. > > Removing boot memory is blocked via register_memory_notifier(&prevent_bootmem_remove_nb); >
On 26.07.23 08:20, mawupeng wrote: > > > On 2023/7/24 14:11, David Hildenbrand wrote: >> On 24.07.23 07:54, Anshuman Khandual wrote: >>> >>> >>> On 7/24/23 06:55, mawupeng wrote: >>>> >>>> On 2023/7/21 18:36, Will Deacon wrote: >>>>> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >>>>>> From: Ma Wupeng <mawupeng1@huawei.com> >>>>>> >>>>>> During our test, we found that kernel page table may be unexpectedly >>>>>> cleared with rodata off. The root cause is that the kernel page is >>>>>> initialized with pud size(1G block mapping) while offline is memory >>>>>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >>>>>> when offline a memory block, the call trace is shown below, >> >> Is someone adding memory in 2 GiB granularity and then removing parts of it in 128 MiB granularity? That would be against what we support using the add_memory() / offline_and_remove_memory() API and that driver should be fixed instead. > > Yes, this kind of situation. > > The problem occurs in the following scenarios: > 1. use mem=xxG to reserve memory. > 2. add_momory to online memory. > 3. offline part of the memroy via offline_and_remove_memory. > > During my research, ACPI memory removal use memory_subsys_offline to offline memory section and > this will not delete page table entry which do not trigger this kind of problem. > > So I understand what you are talking about. > 1. 3rd-party driver shouldn't use add_memory/offline_and_remove_memory to online/offline memory. > If it have to use, this can be achieved by driver. > 2. memory_subsys_offline is perfered to do such thing. No, my point is that 1) If you use add_memory() and offline_and_remove_memory() in the *same granularity* it has to be working, otherwise it has to be fixed. 2) If you use add_memory() and offline_and_remove_memory() in different granularity (especially, add_memory() in bigger granularity) , then change your code to do add_memory() in the same granularity. If you run into 1), then we populated a PUD for boot memory that also covers yet unpopulated physical memory ranges that are later populated by add_memory(). If that's the case, then we can either fix it by a) Not doing that. Use PMD tables instead for that piece of memory. b) Detecting that that PUD still covers memory and refusing to remove that PUD. c) Rejecting to hotadd memory in this situation at that location. We have mhp_get_pluggable_range() -> arch_get_mappable_range() to kind- of handle something like that.
On 2023/7/26 15:50, David Hildenbrand wrote: > On 26.07.23 08:20, mawupeng wrote: >> >> >> On 2023/7/24 14:11, David Hildenbrand wrote: >>> On 24.07.23 07:54, Anshuman Khandual wrote: >>>> >>>> >>>> On 7/24/23 06:55, mawupeng wrote: >>>>> >>>>> On 2023/7/21 18:36, Will Deacon wrote: >>>>>> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >>>>>>> From: Ma Wupeng <mawupeng1@huawei.com> >>>>>>> >>>>>>> During our test, we found that kernel page table may be unexpectedly >>>>>>> cleared with rodata off. The root cause is that the kernel page is >>>>>>> initialized with pud size(1G block mapping) while offline is memory >>>>>>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >>>>>>> when offline a memory block, the call trace is shown below, >>> >>> Is someone adding memory in 2 GiB granularity and then removing parts of it in 128 MiB granularity? That would be against what we support using the add_memory() / offline_and_remove_memory() API and that driver should be fixed instead. >> >> Yes, this kind of situation. >> >> The problem occurs in the following scenarios: >> 1. use mem=xxG to reserve memory. >> 2. add_momory to online memory. >> 3. offline part of the memroy via offline_and_remove_memory. >> >> During my research, ACPI memory removal use memory_subsys_offline to offline memory section and >> this will not delete page table entry which do not trigger this kind of problem. >> >> So I understand what you are talking about. >> 1. 3rd-party driver shouldn't use add_memory/offline_and_remove_memory to online/offline memory. >> If it have to use, this can be achieved by driver. >> 2. memory_subsys_offline is perfered to do such thing. > > No, my point is that > > 1) If you use add_memory() and offline_and_remove_memory() in the *same > granularity* it has to be working, otherwise it has to be fixed. > > 2) If you use add_memory() and offline_and_remove_memory() in different > granularity (especially, add_memory() in bigger granularity) , then > change your code to do add_memory() in the same granularity. > > > If you run into 1), then we populated a PUD for boot memory that also covers yet unpopulated physical memory ranges that are later populated by add_memory(). If that's the case, then we can either fix it by > > a) Not doing that. Use PMD tables instead for that piece of memory. > > b) Detecting that that PUD still covers memory and refusing to remove > that PUD. > > c) Rejecting to hotadd memory in this situation at that location. We > have mhp_get_pluggable_range() -> arch_get_mappable_range() to kind- > of handle something like that. Thank you for your patient answer. This I do understand and answer my question. >
On 7/26/23 13:20, David Hildenbrand wrote: > On 26.07.23 08:20, mawupeng wrote: >> >> >> On 2023/7/24 14:11, David Hildenbrand wrote: >>> On 24.07.23 07:54, Anshuman Khandual wrote: >>>> >>>> >>>> On 7/24/23 06:55, mawupeng wrote: >>>>> >>>>> On 2023/7/21 18:36, Will Deacon wrote: >>>>>> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >>>>>>> From: Ma Wupeng <mawupeng1@huawei.com> >>>>>>> >>>>>>> During our test, we found that kernel page table may be unexpectedly >>>>>>> cleared with rodata off. The root cause is that the kernel page is >>>>>>> initialized with pud size(1G block mapping) while offline is memory >>>>>>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >>>>>>> when offline a memory block, the call trace is shown below, >>> >>> Is someone adding memory in 2 GiB granularity and then removing parts of it in 128 MiB granularity? That would be against what we support using the add_memory() / offline_and_remove_memory() API and that driver should be fixed instead. >> >> Yes, this kind of situation. >> >> The problem occurs in the following scenarios: >> 1. use mem=xxG to reserve memory. >> 2. add_momory to online memory. >> 3. offline part of the memroy via offline_and_remove_memory. >> >> During my research, ACPI memory removal use memory_subsys_offline to offline memory section and >> this will not delete page table entry which do not trigger this kind of problem. >> >> So I understand what you are talking about. >> 1. 3rd-party driver shouldn't use add_memory/offline_and_remove_memory to online/offline memory. >> If it have to use, this can be achieved by driver. >> 2. memory_subsys_offline is perfered to do such thing. > > No, my point is that > > 1) If you use add_memory() and offline_and_remove_memory() in the *same > granularity* it has to be working, otherwise it has to be fixed. > > 2) If you use add_memory() and offline_and_remove_memory() in different > granularity (especially, add_memory() in bigger granularity) , then > change your code to do add_memory() in the same granularity. > > > If you run into 1), then we populated a PUD for boot memory that also covers yet unpopulated physical memory ranges that are later populated by add_memory(). If that's the case, then we can either fix it by Is that case possible ? __create_pgd_mapping() is called to create the mapping both in hotplug and boot memory cases. alloc_init_pud() ensures [1], that both virtual and physical address ranges are PUD_MASK aligned, before creating a huge or block page entry. (addr | next | phys) & ~PUD_MASK) == 0 > > a) Not doing that. Use PMD tables instead for that piece of memory. > > b) Detecting that that PUD still covers memory and refusing to remove > that PUD. > > c) Rejecting to hotadd memory in this situation at that location. We > have mhp_get_pluggable_range() -> arch_get_mappable_range() to kind- > of handle something like that. >
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 95d360805f8a..44c724ce4f70 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -44,6 +44,7 @@ #define NO_BLOCK_MAPPINGS BIT(0) #define NO_CONT_MAPPINGS BIT(1) #define NO_EXEC_MAPPINGS BIT(2) /* assumes FEAT_HPDS is not used */ +#define NO_PUD_MAPPINGS BIT(3) int idmap_t0sz __ro_after_init; @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end, */ if (pud_sect_supported() && ((addr | next | phys) & ~PUD_MASK) == 0 && - (flags & NO_BLOCK_MAPPINGS) == 0) { + (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) { pud_set_huge(pudp, phys, prot); /* @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void) int arch_add_memory(int nid, u64 start, u64 size, struct mhp_params *params) { - int ret, flags = NO_EXEC_MAPPINGS; + int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS; VM_BUG_ON(!mhp_range_allowed(start, size, true));