| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <96a737e5-5545-04ed-e533-e322ffe20fce@bytedance.com>
Date: Mon, 10 Jul 2023 18:52:27 +0800
From: Peng Zhang <zhangpeng.00@...edance.com>
To: Marco Elver <elver@...gle.com>
Cc: glider@...gle.com, dvyukov@...gle.com, akpm@...ux-foundation.org,
kasan-dev@...glegroups.com, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, muchun.song@...ux.dev,
Peng Zhang <zhangpeng.00@...edance.com>
Subject: Re: [PATCH] mm: kfence: allocate kfence_metadata at runtime
在 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@...glegroups.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@...edance.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.
Powered by blists - more mailing lists