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;
Message-ID: <ed82e6b9-3d9f-259a-82bc-cc51f9131f29@bytedance.com>
Date:   Wed, 12 Jul 2023 16:37:10 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.13.0
Subject: Re: [PATCH] mm: kfence: allocate kfence_metadata at runtime
To:     Peng Zhang <zhangpeng.00@bytedance.com>
Cc:     glider@google.com, dvyukov@google.com, akpm@linux-foundation.org,
        kasan-dev@googlegroups.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, muchun.song@linux.dev,
        Marco Elver <elver@google.com>
References: <20230710032714.26200-1-zhangpeng.00@bytedance.com>
 <CANpmjNOHz+dRbJsAyg29nksPMcd2P6109iPxTem_-b2qfUvXtw@mail.gmail.com>
 <2a16a76c-506c-f325-6792-4fb58e8da531@bytedance.com>
From:   Peng Zhang <zhangpeng.00@bytedance.com>
In-Reply-To: <2a16a76c-506c-f325-6792-4fb58e8da531@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2023/7/12 16:28, Peng Zhang 写道:
> 
> 
> 在 2023/7/10 18:19, Marco Elver 写道:
>> 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.
> I'm very sorry because I made a slight modification before sending the
> patch but it has not been tested, which caused it to not work properly.
> I fixed some of the issues you mentioned in v2[1].
> 
> [1] 
> https://lore.kernel.org/lkml/20230712081616.45177-1-zhangpeng.00@bytedance.com/
> 
>>
>>> ---
>>>   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?
> It will be limited to 2^10/sizeof(struct kfence_metadata) by buddy
                 Sorry,  2^10*PAGE_SIZE/sizeof(struct kfence_metadata)
> system, so I used memblock to allocate kfence_metadata in v2.
>>
>>> +               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?
> Unfortunately, __GFP_ZERO does not work successfully in
> alloc_contig_pages(), so I used memzero_explicit() in v2.
> Even though I don't know if memzero_explicit() is necessary
> (it just uses the barrier).
>>
>>> +       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.
> Thanks for spotting this, I added it in v2.
>