Re: [PATCH v3] mm: kfence: allocate kfence_metadata at runtime

[Peng Zhang]

在 2023/7/18 20:39, Marco Elver 写道:
> On Tue, 18 Jul 2023 at 09:30, Peng Zhang <zhangpeng.00@xxxxxxxxxxxxx> 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.
> >
> > Signed-off-by: Peng Zhang <zhangpeng.00@xxxxxxxxxxxxx>
>
> This looks good (minor nit below).
Andrew, if there is no need to update, can you help to add the
deleted blank line below? Thanks.
> Reviewed-by: Marco Elver <elver@xxxxxxxxxx>
Marco, Thank you for your review!
> Thanks!
>
> > ---
> > Changes since v2:
> >   - Fix missing renaming of kfence_alloc_pool.
> >   - Add __read_mostly for kfence_metadata and kfence_metadata_init.
> >   - Use smp_store_release() and smp_load_acquire() to access kfence_metadata.
> >   - Some tweaks to comments and git log.
> >
> > v1: https://lore.kernel.org/lkml/20230710032714.26200-1-zhangpeng.00@xxxxxxxxxxxxx/
> > v2: https://lore.kernel.org/lkml/20230712081616.45177-1-zhangpeng.00@xxxxxxxxxxxxx/
> >
> >   include/linux/kfence.h |  11 ++--
> >   mm/kfence/core.c       | 124 ++++++++++++++++++++++++++++-------------
> >   mm/kfence/kfence.h     |   5 +-
> >   mm/mm_init.c           |   2 +-
> >   4 files changed, 97 insertions(+), 45 deletions(-)
> >
> > diff --git a/include/linux/kfence.h b/include/linux/kfence.h
> > index 726857a4b680..401af4757514 100644
> > --- a/include/linux/kfence.h
> > +++ b/include/linux/kfence.h
> > @@ -59,15 +59,16 @@ static __always_inline bool is_kfence_address(const void *addr)
> >   }
> >
> >   /**
> > - * kfence_alloc_pool() - allocate the KFENCE pool via memblock
> > + * kfence_alloc_pool_and_metadata() - allocate the KFENCE pool and KFENCE
> > + * metadata via memblock
> >    */
> > -void __init kfence_alloc_pool(void);
> > +void __init kfence_alloc_pool_and_metadata(void);
> >
> >   /**
> >    * kfence_init() - perform KFENCE initialization at boot time
> >    *
> > - * Requires that kfence_alloc_pool() was called before. This sets up the
> > - * allocation gate timer, and requires that workqueues are available.
> > + * Requires that kfence_alloc_pool_and_metadata() was called before. This sets
> > + * up the allocation gate timer, and requires that workqueues are available.
> >    */
> >   void __init kfence_init(void);
> >
> > @@ -223,7 +224,7 @@ bool __kfence_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *sla
> >   #else /* CONFIG_KFENCE */
> >
> >   static inline bool is_kfence_address(const void *addr) { return false; }
> > -static inline void kfence_alloc_pool(void) { }
> > +static inline void kfence_alloc_pool_and_metadata(void) { }
> >   static inline void kfence_init(void) { }
> >   static inline void kfence_shutdown_cache(struct kmem_cache *s) { }
> >   static inline void *kfence_alloc(struct kmem_cache *s, size_t size, gfp_t flags) { return NULL; }
> > diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> > index dad3c0eb70a0..6b526435886c 100644
> > --- a/mm/kfence/core.c
> > +++ b/mm/kfence/core.c
> > @@ -116,7 +116,15 @@ 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 __read_mostly;
> > +
> > +/*
> > + * If kfence_metadata is not NULL, it may be accessed by kfence_shutdown_cache().
> > + * So introduce kfence_metadata_init to initialize metadata, and then make
> > + * kfence_metadata visible after initialization is successful. This prevents
> > + * potential UAF or access to uninitialized metadata.
> > + */
> > +static struct kfence_metadata *kfence_metadata_init __read_mostly;
> >
> >   /* Freelist with available objects. */
> >   static struct list_head kfence_freelist = LIST_HEAD_INIT(kfence_freelist);
> > @@ -591,7 +599,7 @@ static unsigned long kfence_init_pool(void)
> >
> >                  __folio_set_slab(slab_folio(slab));
> >   #ifdef CONFIG_MEMCG
> > -               slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 1].objcg |
> > +               slab->memcg_data = (unsigned long)&kfence_metadata_init[i / 2 - 1].objcg |
> >                                     MEMCG_DATA_OBJCGS;
> >   #endif
> >          }
> > @@ -610,7 +618,7 @@ static unsigned long kfence_init_pool(void)
> >          }
> >
> >          for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
> > -               struct kfence_metadata *meta = &kfence_metadata[i];
> > +               struct kfence_metadata *meta = &kfence_metadata_init[i];
> >
> >                  /* Initialize metadata. */
> >                  INIT_LIST_HEAD(&meta->list);
> > @@ -626,6 +634,12 @@ static unsigned long kfence_init_pool(void)
> >                  addr += 2 * PAGE_SIZE;
> >          }
> >
> > +       /*
> > +        * Make kfence_metadata visible only when initialization is successful.
> > +        * Otherwise, if the initialization fails and kfence_metadata is freed,
> > +        * it may cause UAF in kfence_shutdown_cache().
> > +        */
> > +       smp_store_release(&kfence_metadata, kfence_metadata_init);
> >          return 0;
> >
> >   reset_slab:
> > @@ -672,26 +686,10 @@ static bool __init kfence_init_pool_early(void)
> >           */
> >          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;
> > +       memblock_free_late(__pa(kfence_metadata_init), KFENCE_METADATA_SIZE);
> > +       kfence_metadata_init = NULL;
> >
> > -       /* 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;
> >   }
> >
> > @@ -841,19 +839,30 @@ static void toggle_allocation_gate(struct work_struct *work)
> >
> >   /* === Public interface ===================================================== */
> >
> > -void __init kfence_alloc_pool(void)
> > +void __init kfence_alloc_pool_and_metadata(void)
> >   {
> >          if (!kfence_sample_interval)
> >                  return;
> >
> > -       /* if the pool has already been initialized by arch, skip the below. */
> > -       if (__kfence_pool)
> > -               return;
> > -
> > -       __kfence_pool = memblock_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
> > -
> > +       /*
> > +        * If the pool has already been initialized by arch, there is no need to
> > +        * re-allocate the memory pool.
> > +        */
> >          if (!__kfence_pool)
> > +               __kfence_pool = memblock_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
> > +
> > +       if (!__kfence_pool) {
> >                  pr_err("failed to allocate pool\n");
> > +               return;
> > +       }
> > +
> > +       /* The memory allocated by memblock has been zeroed out. */
> > +       kfence_metadata_init = memblock_alloc(KFENCE_METADATA_SIZE, PAGE_SIZE);
> > +       if (!kfence_metadata_init) {
> > +               pr_err("failed to allocate metadata\n");
> > +               memblock_free(__kfence_pool, KFENCE_POOL_SIZE);
> > +               __kfence_pool = NULL;
> > +       }
> >   }
> >
> >   static void kfence_init_enable(void)
> > @@ -895,33 +904,68 @@ void __init kfence_init(void)
> >
> >   static int kfence_init_late(void)
> >   {
> > -       const unsigned long nr_pages = KFENCE_POOL_SIZE / PAGE_SIZE;
> > +       const unsigned long nr_pages_pool = KFENCE_POOL_SIZE / PAGE_SIZE;
> > +       const unsigned long nr_pages_meta = KFENCE_METADATA_SIZE / PAGE_SIZE;
> > +       unsigned long addr = (unsigned long)__kfence_pool;
> > +       unsigned long free_size = KFENCE_POOL_SIZE;
> > +       int err = -ENOMEM;
> > +
> >   #ifdef CONFIG_CONTIG_ALLOC
> >          struct page *pages;
> > -
>
> Unnecessary blank line removal (it looks worse now).
>
>
> > -       pages = alloc_contig_pages(nr_pages, GFP_KERNEL, first_online_node, NULL);
> > +       pages = alloc_contig_pages(nr_pages_pool, GFP_KERNEL, first_online_node,
> > +                                  NULL);
> >          if (!pages)
> >                  return -ENOMEM;
> > +
> >          __kfence_pool = page_to_virt(pages);
> > +       pages = alloc_contig_pages(nr_pages_meta, GFP_KERNEL, first_online_node,
> > +                                  NULL);
> > +       if (pages)
> > +               kfence_metadata_init = page_to_virt(pages);
> >   #else
> > -       if (nr_pages > MAX_ORDER_NR_PAGES) {
> > +       if (nr_pages_pool > MAX_ORDER_NR_PAGES ||
> > +           nr_pages_meta > MAX_ORDER_NR_PAGES) {
> >                  pr_warn("KFENCE_NUM_OBJECTS too large for buddy allocator\n");
> >                  return -EINVAL;
> >          }
> > +
> >          __kfence_pool = alloc_pages_exact(KFENCE_POOL_SIZE, GFP_KERNEL);
> >          if (!__kfence_pool)
> >                  return -ENOMEM;
> > +
> > +       kfence_metadata_init = alloc_pages_exact(KFENCE_METADATA_SIZE, GFP_KERNEL);
> >   #endif
> >
> > -       if (!kfence_init_pool_late()) {
> > -               pr_err("%s failed\n", __func__);
> > -               return -EBUSY;
> > +       if (!kfence_metadata_init)
> > +               goto free_pool;
> > +
> > +       memzero_explicit(kfence_metadata_init, KFENCE_METADATA_SIZE);
> > +       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__);
> > +       free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool);
> > +       err = -EBUSY;
> >
> > -       return 0;
> > +#ifdef CONFIG_CONTIG_ALLOC
> > +       free_contig_range(page_to_pfn(virt_to_page((void *)kfence_metadata_init)),
> > +                         nr_pages_meta);
> > +free_pool:
> > +       free_contig_range(page_to_pfn(virt_to_page((void *)addr)),
> > +                         free_size / PAGE_SIZE);
> > +#else
> > +       free_pages_exact((void *)kfence_metadata_init, KFENCE_METADATA_SIZE);
> > +free_pool:
> > +       free_pages_exact((void *)addr, free_size);
> > +#endif
> > +
> > +       kfence_metadata_init = NULL;
> > +       __kfence_pool = NULL;
> > +       return err;
> >   }
> >
> >   static int kfence_enable_late(void)
> > @@ -941,6 +985,10 @@ void kfence_shutdown_cache(struct kmem_cache *s)
> >          struct kfence_metadata *meta;
> >          int i;
> >
> > +       /* Pairs with release in kfence_init_pool(). */
> > +       if (!smp_load_acquire(&kfence_metadata))
> > +               return;
> > +
> >          for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
> >                  bool in_use;
> >
> > diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h
> > index 392fb273e7bd..f46fbb03062b 100644
> > --- a/mm/kfence/kfence.h
> > +++ b/mm/kfence/kfence.h
> > @@ -102,7 +102,10 @@ struct kfence_metadata {
> >   #endif
> >   };
> >
> > -extern struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS];
> > +#define KFENCE_METADATA_SIZE PAGE_ALIGN(sizeof(struct kfence_metadata) * \
> > +                                       CONFIG_KFENCE_NUM_OBJECTS)
> > +
> > +extern struct kfence_metadata *kfence_metadata;
> >
> >   static inline struct kfence_metadata *addr_to_metadata(unsigned long addr)
> >   {
> > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > index 7f7f9c677854..3d0a63c75829 100644
> > --- a/mm/mm_init.c
> > +++ b/mm/mm_init.c
> > @@ -2721,7 +2721,7 @@ void __init mm_core_init(void)
> >           */
> >          page_ext_init_flatmem();
> >          mem_debugging_and_hardening_init();
> > -       kfence_alloc_pool();
> > +       kfence_alloc_pool_and_metadata();
> >          report_meminit();
> >          kmsan_init_shadow();
> >          stack_depot_early_init();
> > --
> > 2.20.1