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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2630BC6FD19 for ; Thu, 16 Mar 2023 10:09:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=iT7AZ+45vf85N8VStUR4hlHasQfgj8xKoLDHmI4y5w0=; b=q9VWaI7u05NEy6 Lq9OnjlnIDhJMBqeSYaeD43Whq5zG7sBsOKrkseUm2hSRe4kZNCDr1JUFNTKBGaddoUS7YB7auP19 SKQo3wC7zO/1FDqd5TQ+4eTEBhZSNSRoWF8zDMFoOpzDgR0CPWn9w8J+ymNSUMa8s3p/yUiLZM2ab 1LSx8wSBDUY2IGwn85NKzQjO+ivs1X4nbhivDjco4AE4SJcsXvvFrxj4c0KNglLnRGwPSjp7szw1t vb8NjUGMt0Z8YYFw/RIBm+Sglvt2wrZCuB5n922rNeB7OrpQeqpV5qeL2rFJSGJgqF+NyCDzXJ9M6 PxTN5RS5FbgTVWZ0yD9A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pckVr-00FyWS-3B; Thu, 16 Mar 2023 10:07:12 +0000 Received: from mail-wm1-x32f.google.com ([2a00:1450:4864:20::32f]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pckVn-00FyTQ-2H for linux-arm-kernel@lists.infradead.org; Thu, 16 Mar 2023 10:07:10 +0000 Received: by mail-wm1-x32f.google.com with SMTP id x22so808054wmj.3 for ; Thu, 16 Mar 2023 03:07:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678961220; h=user-agent: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=EMq7tDc/0X3ajktZnr96EPs4WhVyPi++3wTjhaux/UU=; b=k1reqwA70CPAT5jyiJke9fY7VmX9ej5IIvfmqQszumAaoG39kggtE4Eu9Np6YUrkEP h8vtkbCg4JNzLPKnOQAlFDpwXoIXgGVKisb9pbGKOCBvdGubF1GlY2QLzc+2gU4kWmVn TovyE+xUzbeRmaAY57LN6fg9GVGPQqLj4L8WnEETlgHIrj05BEg96iO8M/tLoGoE4xvS VEB6voLzOf8K3BEizOLkerOLGtJlogYUuy20+glzmMRW7CPEQo8EQd7oqmEt8CmQCyAT gTjKxijtvolrIGrmZfUEM1EuUra0zfheEVjm+3EFbmmAZYkD6w/agWgjK4aUD462Qc8X 9R8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678961220; h=user-agent: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=EMq7tDc/0X3ajktZnr96EPs4WhVyPi++3wTjhaux/UU=; b=ZGGNeOFgfXRDIhl/QlB7DaF96dxq/UsOLLNB3+7Rv7BmQQkGrnocMi0XJIXWy9jOo0 yJQ7HdRZvxjSVyOlDKgQ+S+D67S9ifenP160ilQYcZYk68Ft+rhJtj89cERf93cN1J/D ZTnvw5kuzeg7a8FcW5oN7633TVFUam6opzapPNXkjiZtUactdj1nbqZ8WFHWa5IK5ym1 inPa6tQmFWUOVv9ZH5wdq8eyAHURFNSELIluWkALDptVuAX1zF8dxvyZkU/G42rlILn0 k6KV0IFPb24RL0ik+RwbHDLx7IMifPwhArKrlrL3dhgr8CyEM1KH+1Z7osQIeWYbWkGR rlmQ== X-Gm-Message-State: AO0yUKXoNm4ENYLoZZ4rgi4X3E7KVuaaZm+Y6avhMKw8V1fv6IFJYdgz AX3m9xyow/V5uhoUyzpqdcOBKg== X-Google-Smtp-Source: AK7set/rr4NnDKcZ0jy/awSc5nqEZCaEa1JqTpS5wMrVYhoxHFFWaAK+QK9wUl7utDgGovGclLnOSQ== X-Received: by 2002:a05:600c:4ed0:b0:3e2:20c7:6553 with SMTP id g16-20020a05600c4ed000b003e220c76553mr20455044wmq.13.1678961219905; Thu, 16 Mar 2023 03:06:59 -0700 (PDT) Received: from elver.google.com ([2a00:79e0:9c:201:f359:6b95:96e:1317]) by smtp.gmail.com with ESMTPSA id bd20-20020a05600c1f1400b003e21dcccf9fsm4470397wmb.16.2023.03.16.03.06.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Mar 2023 03:06:57 -0700 (PDT) Date: Thu, 16 Mar 2023 11:06:50 +0100 From: Marco Elver To: Zhenhua Huang Cc: catalin.marinas@arm.com, will@kernel.org, glider@google.com, dvyukov@google.com, akpm@linux-foundation.org, robin.murphy@arm.com, mark.rutland@arm.com, jianyong.wu@arm.com, james.morse@arm.com, wangkefeng.wang@huawei.com, linux-arm-kernel@lists.infradead.org, kasan-dev@googlegroups.com, linux-mm@kvack.org, quic_pkondeti@quicinc.com, quic_guptap@quicinc.com, quic_tingweiz@quicinc.com Subject: Re: [PATCH v9] mm,kfence: decouple kfence from page granularity mapping judgement Message-ID: References: <1678956620-26103-1-git-send-email-quic_zhenhuah@quicinc.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1678956620-26103-1-git-send-email-quic_zhenhuah@quicinc.com> User-Agent: Mutt/2.2.9 (2022-11-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230316_030707_745147_2346CC1E X-CRM114-Status: GOOD ( 41.25 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Mar 16, 2023 at 04:50PM +0800, Zhenhua Huang wrote: > Kfence only needs its pool to be mapped as page granularity, if it is > inited early. Previous judgement was a bit over protected. From [1], Mark > suggested to "just map the KFENCE region a page granularity". So I > decouple it from judgement and do page granularity mapping for kfence > pool only. Need to be noticed that late init of kfence pool still requires > page granularity mapping. > > Page granularity mapping in theory cost more(2M per 1GB) memory on arm64 > platform. Like what I've tested on QEMU(emulated 1GB RAM) with > gki_defconfig, also turning off rodata protection: > Before: > [root@liebao ]# cat /proc/meminfo > MemTotal: 999484 kB > After: > [root@liebao ]# cat /proc/meminfo > MemTotal: 1001480 kB > > To implement this, also relocate the kfence pool allocation before the > linear mapping setting up, arm64_kfence_alloc_pool is to allocate phys > addr, __kfence_pool is to be set after linear mapping set up. > > LINK: [1] https://lore.kernel.org/linux-arm-kernel/Y+IsdrvDNILA59UN@FVFF77S0Q05N/ > Suggested-by: Mark Rutland > Signed-off-by: Zhenhua Huang > --- > arch/arm64/include/asm/kfence.h | 16 +++++++++++ > arch/arm64/mm/mmu.c | 59 +++++++++++++++++++++++++++++++++++++++++ > arch/arm64/mm/pageattr.c | 9 +++++-- > include/linux/kfence.h | 1 + > mm/kfence/core.c | 4 +++ > 5 files changed, 87 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/kfence.h b/arch/arm64/include/asm/kfence.h > index aa855c6..8143c91 100644 > --- a/arch/arm64/include/asm/kfence.h > +++ b/arch/arm64/include/asm/kfence.h > @@ -10,6 +10,22 @@ > > #include > > +extern phys_addr_t early_kfence_pool; This should not be accessible if !CONFIG_KFENCE. > +#ifdef CONFIG_KFENCE > + > +extern char *__kfence_pool; > +static inline void kfence_set_pool(phys_addr_t addr) > +{ > + __kfence_pool = phys_to_virt(addr); > +} kfence_set_pool() is redundant if it's for arm64 only, because we know where it's needed, and there you could just access __kfence_pool directly. So let's just remove this function. (Initially I thought you want to provide it generally, also for other architectures.) > +#else > + > +static inline void kfence_set_pool(phys_addr_t addr) { } > + > +#endif > + > static inline bool arch_kfence_init_pool(void) { return true; } [...] > +#endif > + > +phys_addr_t early_kfence_pool; This variable now exists in non-KFENCE builds, which is wrong. > static void __init map_mem(pgd_t *pgdp) > { > static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN); > @@ -543,6 +587,10 @@ static void __init map_mem(pgd_t *pgdp) > */ > BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end)); > > + early_kfence_pool = arm64_kfence_alloc_pool(); > + if (early_kfence_pool) > + memblock_mark_nomap(early_kfence_pool, KFENCE_POOL_SIZE); > + > if (can_set_direct_map()) > flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS; > > @@ -608,6 +656,17 @@ static void __init map_mem(pgd_t *pgdp) > } > } > #endif > + > + /* Kfence pool needs page-level mapping */ > + if (early_kfence_pool) { > + __map_memblock(pgdp, early_kfence_pool, > + early_kfence_pool + KFENCE_POOL_SIZE, > + pgprot_tagged(PAGE_KERNEL), > + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS); > + memblock_clear_nomap(early_kfence_pool, KFENCE_POOL_SIZE); > + /* kfence_pool really mapped now */ > + kfence_set_pool(early_kfence_pool); > + } This whole piece of code could also be wrapped in another function, which becomes a no-op if !CONFIG_KFENCE. Then you also don't need to provide the KFENCE_POOL_SIZE define for 0 if !CONFIG_KFENCE. [...] > + * > + * Kfence pool requires page granularity mapping also if we init it > + * late. > */ > return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() || > - IS_ENABLED(CONFIG_KFENCE); > + (IS_ENABLED(CONFIG_KFENCE) && !early_kfence_pool); Accessing a non-existent variable if !CONFIG_KFENCE works because the compiler optimizes out the access, but is generally bad style. I think the only issue that I have is that the separation between KFENCE and non-KFENCE builds is not great. At the end of the email are is a diff against your patch which would be my suggested changes (while at it, I fixed up a bunch of other issues). Untested, so if you decide to adopt these changes, please test. Thanks, -- Marco ------ >8 ------ diff --git a/arch/arm64/include/asm/kfence.h b/arch/arm64/include/asm/kfence.h index 8143c91854e1..a81937fae9f6 100644 --- a/arch/arm64/include/asm/kfence.h +++ b/arch/arm64/include/asm/kfence.h @@ -10,22 +10,6 @@ #include -extern phys_addr_t early_kfence_pool; - -#ifdef CONFIG_KFENCE - -extern char *__kfence_pool; -static inline void kfence_set_pool(phys_addr_t addr) -{ - __kfence_pool = phys_to_virt(addr); -} - -#else - -static inline void kfence_set_pool(phys_addr_t addr) { } - -#endif - static inline bool arch_kfence_init_pool(void) { return true; } static inline bool kfence_protect_page(unsigned long addr, bool protect) @@ -35,4 +19,14 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect) return true; } +#ifdef CONFIG_KFENCE +extern bool kfence_early_init; +static inline bool arm64_kfence_can_set_direct_map(void) +{ + return !kfence_early_init; +} +#else /* CONFIG_KFENCE */ +static inline bool arm64_kfence_can_set_direct_map(void) { return false; } +#endif /* CONFIG_KFENCE */ + #endif /* __ASM_KFENCE_H */ diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 61944c7091f0..683958616ac1 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -528,17 +528,14 @@ static int __init enable_crash_mem_map(char *arg) early_param("crashkernel", enable_crash_mem_map); #ifdef CONFIG_KFENCE +bool kfence_early_init = !!CONFIG_KFENCE_SAMPLE_INTERVAL; -static bool kfence_early_init __initdata = !!CONFIG_KFENCE_SAMPLE_INTERVAL; -/* - * early_param can be parsed before linear mapping - * set up - */ -static int __init parse_kfence_early_init(char *p) +/* early_param() will be parsed before map_mem() below. */ +static int __init parse_kfence_early_init(char *arg) { int val; - if (get_option(&p, &val)) + if (get_option(&arg, &val)) kfence_early_init = !!val; return 0; } @@ -552,22 +549,34 @@ static phys_addr_t arm64_kfence_alloc_pool(void) return 0; kfence_pool = memblock_phys_alloc(KFENCE_POOL_SIZE, PAGE_SIZE); - if (!kfence_pool) + if (!kfence_pool) { pr_err("failed to allocate kfence pool\n"); + kfence_early_init = false; + return 0; + } + + /* Temporarily mark as NOMAP. */ + memblock_mark_nomap(kfence_pool, KFENCE_POOL_SIZE); return kfence_pool; } -#else - -static phys_addr_t arm64_kfence_alloc_pool(void) +static void arm64_kfence_map_pool(phys_addr_t kfence_pool, pgd_t *pgdp) { - return 0; -} - -#endif + if (!kfence_pool) + return; -phys_addr_t early_kfence_pool; + /* KFENCE pool needs page-level mapping. */ + __map_memblock(pgdp, kfence_pool, kfence_pool + KFENCE_POOL_SIZE, + pgprot_tagged(PAGE_KERNEL), + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS); + memblock_clear_nomap(kfence_pool, KFENCE_POOL_SIZE); + __kfence_pool = phys_to_virt(kfence_pool); +} +#else /* CONFIG_KFENCE */ +static inline phys_addr_t arm64_kfence_alloc_pool(void) { return 0; } +static inline void arm64_kfence_map_pool(phys_addr_t kfence_pool, pgd_t *pgdp) { } +#endif /* CONFIG_KFENCE */ static void __init map_mem(pgd_t *pgdp) { @@ -575,6 +584,7 @@ static void __init map_mem(pgd_t *pgdp) phys_addr_t kernel_start = __pa_symbol(_stext); phys_addr_t kernel_end = __pa_symbol(__init_begin); phys_addr_t start, end; + phys_addr_t early_kfence_pool; int flags = NO_EXEC_MAPPINGS; u64 i; @@ -588,8 +598,6 @@ static void __init map_mem(pgd_t *pgdp) BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end)); early_kfence_pool = arm64_kfence_alloc_pool(); - if (early_kfence_pool) - memblock_mark_nomap(early_kfence_pool, KFENCE_POOL_SIZE); if (can_set_direct_map()) flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS; @@ -656,17 +664,7 @@ static void __init map_mem(pgd_t *pgdp) } } #endif - - /* Kfence pool needs page-level mapping */ - if (early_kfence_pool) { - __map_memblock(pgdp, early_kfence_pool, - early_kfence_pool + KFENCE_POOL_SIZE, - pgprot_tagged(PAGE_KERNEL), - NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS); - memblock_clear_nomap(early_kfence_pool, KFENCE_POOL_SIZE); - /* kfence_pool really mapped now */ - kfence_set_pool(early_kfence_pool); - } + arm64_kfence_map_pool(early_kfence_pool, pgdp); } void mark_rodata_ro(void) diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c index 7ce5295cc6fb..aa8fd12cc96f 100644 --- a/arch/arm64/mm/pageattr.c +++ b/arch/arm64/mm/pageattr.c @@ -7,7 +7,6 @@ #include #include #include -#include #include #include @@ -28,11 +27,10 @@ bool can_set_direct_map(void) * mapped at page granularity, so that it is possible to * protect/unprotect single pages. * - * Kfence pool requires page granularity mapping also if we init it - * late. + * KFENCE pool requires page-granular mapping if initialized late. */ return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() || - (IS_ENABLED(CONFIG_KFENCE) && !early_kfence_pool); + arm64_kfence_can_set_direct_map(); } static int change_page_range(pte_t *ptep, unsigned long addr, void *data) diff --git a/include/linux/kfence.h b/include/linux/kfence.h index 91cbcc98e293..726857a4b680 100644 --- a/include/linux/kfence.h +++ b/include/linux/kfence.h @@ -222,7 +222,6 @@ bool __kfence_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *sla #else /* CONFIG_KFENCE */ -#define KFENCE_POOL_SIZE 0 static inline bool is_kfence_address(const void *addr) { return false; } static inline void kfence_alloc_pool(void) { } static inline void kfence_init(void) { } diff --git a/mm/kfence/core.c b/mm/kfence/core.c index fab087d39633..e7f22af5e710 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -818,7 +818,7 @@ void __init kfence_alloc_pool(void) if (!kfence_sample_interval) return; - /* if the pool has already been initialized by arch, skip the below */ + /* If the pool has already been initialized by arch, skip the below. */ if (__kfence_pool) return; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel