Messages in this thread | | | Date | Mon, 10 Jul 2023 18:52:27 +0800 | Subject | Re: [PATCH] mm: kfence: allocate kfence_metadata at runtime | From | Peng Zhang <> |
| |
在 2023/7/10 18:21, Marco Elver 写道: > On Mon, Jul 10, 2023 at 12:19PM +0200, Marco Elver wrote: >> On Mon, 10 Jul 2023 at 05:27, 'Peng Zhang' via kasan-dev >> <kasan-dev@googlegroups.com> wrote: >>> >>> kfence_metadata is currently a static array. For the purpose of >>> allocating scalable __kfence_pool, we first change it to runtime >>> allocation of metadata. Since the size of an object of kfence_metadata >>> is 1160 bytes, we can save at least 72 pages (with default 256 objects) >>> without enabling kfence. >>> >>> Below is the numbers obtained in qemu (with default 256 objects). >>> before: Memory: 8134692K/8388080K available (3668K bss) >>> after: Memory: 8136740K/8388080K available (1620K bss) >>> More than expected, it saves 2MB memory. >>> >>> Signed-off-by: Peng Zhang <zhangpeng.00@bytedance.com> >> >> Seems like a reasonable optimization, but see comments below. >> >> Also with this patch applied on top of v6.5-rc1, KFENCE just doesn't >> init at all anymore (early init). Please fix. > > Forgot to attach .config -- attached config. > > All I see is: > > [ 0.303465] rcu: RCU calculated value of scheduler-enlistment delay is 100 jiffies. > [ 0.304783] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=8 > [ 0.316800] NR_IRQS: 4352, nr_irqs: 488, preallocated irqs: 16 > [ 0.318140] rcu: srcu_init: Setting srcu_struct sizes based on contention. > [ 0.320001] kfence: kfence_init failed > [ 0.326880] Console: colour VGA+ 80x25 > [ 0.327585] printk: console [ttyS0] enabled > [ 0.327585] printk: console [ttyS0] enabled > > around KFENCE initialization. Thanks for your review and testing, I'll take a look at the issues later. > >>> --- >>> mm/kfence/core.c | 102 ++++++++++++++++++++++++++++++++------------- >>> mm/kfence/kfence.h | 5 ++- >>> 2 files changed, 78 insertions(+), 29 deletions(-) >>> >>> diff --git a/mm/kfence/core.c b/mm/kfence/core.c >>> index dad3c0eb70a0..b9fec1c46e3d 100644 >>> --- a/mm/kfence/core.c >>> +++ b/mm/kfence/core.c >>> @@ -116,7 +116,7 @@ EXPORT_SYMBOL(__kfence_pool); /* Export for test modules. */ >>> * backing pages (in __kfence_pool). >>> */ >>> static_assert(CONFIG_KFENCE_NUM_OBJECTS > 0); >>> -struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS]; >>> +struct kfence_metadata *kfence_metadata; >>> >>> /* Freelist with available objects. */ >>> static struct list_head kfence_freelist = LIST_HEAD_INIT(kfence_freelist); >>> @@ -643,13 +643,56 @@ static unsigned long kfence_init_pool(void) >>> return addr; >>> } >>> >>> +static int kfence_alloc_metadata(void) >>> +{ >>> + unsigned long nr_pages = KFENCE_METADATA_SIZE / PAGE_SIZE; >>> + >>> +#ifdef CONFIG_CONTIG_ALLOC >>> + struct page *pages; >>> + >>> + pages = alloc_contig_pages(nr_pages, GFP_KERNEL, first_online_node, >>> + NULL); >>> + if (pages) >>> + kfence_metadata = page_to_virt(pages); >>> +#else >>> + if (nr_pages > MAX_ORDER_NR_PAGES) { >>> + pr_warn("KFENCE_NUM_OBJECTS too large for buddy allocator\n"); >> >> Does this mean that KFENCE won't work at all if we can't allocate the >> metadata? I.e. it won't work either in early nor late init modes? >> >> I know we already have this limitation for _late init_ of the KFENCE pool. >> >> So I have one major question: when doing _early init_, what is the >> maximum size of the KFENCE pool (#objects) with this change? >> >>> + return -EINVAL; >>> + } >>> + kfence_metadata = alloc_pages_exact(KFENCE_METADATA_SIZE, >>> + GFP_KERNEL); >>> +#endif >>> + >>> + if (!kfence_metadata) >>> + return -ENOMEM; >>> + >>> + memset(kfence_metadata, 0, KFENCE_METADATA_SIZE); >> >> memzero_explicit, or pass __GFP_ZERO to alloc_pages? >> >>> + return 0; >>> +} >>> + >>> +static void kfence_free_metadata(void) >>> +{ >>> + if (WARN_ON(!kfence_metadata)) >>> + return; >>> +#ifdef CONFIG_CONTIG_ALLOC >>> + free_contig_range(page_to_pfn(virt_to_page((void *)kfence_metadata)), >>> + KFENCE_METADATA_SIZE / PAGE_SIZE); >>> +#else >>> + free_pages_exact((void *)kfence_metadata, KFENCE_METADATA_SIZE); >>> +#endif >>> + kfence_metadata = NULL; >>> +} >>> + >>> static bool __init kfence_init_pool_early(void) >>> { >>> - unsigned long addr; >>> + unsigned long addr = (unsigned long)__kfence_pool; >>> >>> if (!__kfence_pool) >>> return false; >>> >>> + if (!kfence_alloc_metadata()) >>> + goto free_pool; >>> + >>> addr = kfence_init_pool(); >>> >>> if (!addr) { >>> @@ -663,6 +706,7 @@ static bool __init kfence_init_pool_early(void) >>> return true; >>> } >>> >>> + kfence_free_metadata(); >>> /* >>> * Only release unprotected pages, and do not try to go back and change >>> * page attributes due to risk of failing to do so as well. If changing >>> @@ -670,31 +714,12 @@ static bool __init kfence_init_pool_early(void) >>> * fails for the first page, and therefore expect addr==__kfence_pool in >>> * most failure cases. >>> */ >>> +free_pool: >>> memblock_free_late(__pa(addr), KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool)); >>> __kfence_pool = NULL; >>> return false; >>> } >>> >>> -static bool kfence_init_pool_late(void) >>> -{ >>> - unsigned long addr, free_size; >>> - >>> - addr = kfence_init_pool(); >>> - >>> - if (!addr) >>> - return true; >>> - >>> - /* Same as above. */ >>> - free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool); >>> -#ifdef CONFIG_CONTIG_ALLOC >>> - free_contig_range(page_to_pfn(virt_to_page((void *)addr)), free_size / PAGE_SIZE); >>> -#else >>> - free_pages_exact((void *)addr, free_size); >>> -#endif >>> - __kfence_pool = NULL; >>> - return false; >>> -} >>> - >>> /* === DebugFS Interface ==================================================== */ >>> >>> static int stats_show(struct seq_file *seq, void *v) >>> @@ -896,6 +921,10 @@ void __init kfence_init(void) >>> static int kfence_init_late(void) >>> { >>> const unsigned long nr_pages = KFENCE_POOL_SIZE / PAGE_SIZE; >>> + unsigned long addr = (unsigned long)__kfence_pool; >>> + unsigned long free_size = KFENCE_POOL_SIZE; >>> + int ret; >>> + >>> #ifdef CONFIG_CONTIG_ALLOC >>> struct page *pages; >>> >>> @@ -913,15 +942,29 @@ static int kfence_init_late(void) >>> return -ENOMEM; >>> #endif >>> >>> - if (!kfence_init_pool_late()) { >>> - pr_err("%s failed\n", __func__); >>> - return -EBUSY; >>> + ret = kfence_alloc_metadata(); >>> + if (!ret) >>> + goto free_pool; >>> + >>> + addr = kfence_init_pool(); >>> + if (!addr) { >>> + kfence_init_enable(); >>> + kfence_debugfs_init(); >>> + return 0; >>> } >>> >>> - kfence_init_enable(); >>> - kfence_debugfs_init(); >>> + pr_err("%s failed\n", __func__); >>> + kfence_free_metadata(); >>> + free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool); >>> + ret = -EBUSY; >>> >>> - return 0; >>> +free_pool: >>> +#ifdef CONFIG_CONTIG_ALLOC >>> + free_contig_range(page_to_pfn(virt_to_page((void *)addr)), free_size / PAGE_SIZE); >>> +#else >>> + free_pages_exact((void *)addr, free_size); >>> +#endif >> >> You moved this from kfence_init_pool_late - that did "__kfence_pool = >> NULL" which is missing now.
| |