[PATCH 1/2] mm: kfence: fix missing objcg housekeeping for SLAB

[PATCH 1/2] mm: kfence: fix missing objcg housekeeping for SLAB

[Muchun Song]

The objcg is not cleared and put for kfence object when it is freed, which
could lead to memory leak for struct obj_cgroup and wrong statistics of
NR_SLAB_RECLAIMABLE_B or NR_SLAB_UNRECLAIMABLE_B.  Since the last freed
object's objcg is not cleared, mem_cgroup_from_obj() could return the wrong
memcg when this kfence object, which is not charged to any objcgs, is
reallocated to other users.  A real word issue [1] is caused by this bug.

[1] https://groups.google.com/g/syzkaller-bugs/c/BBQFy2QraoY/m/HtBd5gbyAQAJ
Reported-by: syzbot+f8c45ccc7d5d45fc5965@syzkaller.appspotmail.com
Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/slab.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/slab.c b/mm/slab.c
index d9dec7a8fd79..b04e40078bdf 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3422,6 +3422,7 @@ static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
 
 	if (is_kfence_address(objp)) {
 		kmemleak_free_recursive(objp, cachep->flags);
+		memcg_slab_free_hook(cachep, &objp, 1);
 		__kfence_free(objp);
 		return;
 	}
-- 
2.11.0

[PATCH 2/2] mm: kfence: fix objcgs vector allocation

[Muchun Song]

If the kfence object is allocated to be used for objects vector, then
this slot of the pool eventually being occupied permanently since
the vector is never freed.  The solutions could be 1) freeing vector
when the kfence object is freed or 2) allocating all vectors statically.
Since the memory consumption of object vectors is low, it is better to
chose 2) to fix the issue and it is also can reduce overhead of vectors
allocating in the future.

Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/kfence/core.c   | 3 +++
 mm/kfence/kfence.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 13128fa13062..9976b3f0d097 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -579,9 +579,11 @@ static bool __init kfence_init_pool(void)
 	}
 
 	for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
+		struct slab *slab = virt_to_slab(addr);
 		struct kfence_metadata *meta = &kfence_metadata[i];
 
 		/* Initialize metadata. */
+		slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
 		INIT_LIST_HEAD(&meta->list);
 		raw_spin_lock_init(&meta->lock);
 		meta->state = KFENCE_OBJECT_UNUSED;
@@ -938,6 +940,7 @@ void __kfence_free(void *addr)
 {
 	struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
 
+	KFENCE_WARN_ON(meta->objcg);
 	/*
 	 * If the objects of the cache are SLAB_TYPESAFE_BY_RCU, defer freeing
 	 * the object, as the object page may be recycled for other-typed
diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h
index 2a2d5de9d379..6f0e1aece3f8 100644
--- a/mm/kfence/kfence.h
+++ b/mm/kfence/kfence.h
@@ -89,6 +89,7 @@ struct kfence_metadata {
 	struct kfence_track free_track;
 	/* For updating alloc_covered on frees. */
 	u32 alloc_stack_hash;
+	struct obj_cgroup *objcg;
 };
 
 extern struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS];
-- 
2.11.0

Re: [PATCH 2/2] mm: kfence: fix objcgs vector allocation

[Muchun Song]

On Sun, Mar 27, 2022 at 1:19 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> If the kfence object is allocated to be used for objects vector, then
> this slot of the pool eventually being occupied permanently since
> the vector is never freed.  The solutions could be 1) freeing vector
> when the kfence object is freed or 2) allocating all vectors statically.
> Since the memory consumption of object vectors is low, it is better to
> chose 2) to fix the issue and it is also can reduce overhead of vectors
> allocating in the future.
>
> Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

Since it cannot be compiled successfully when !CONFIG_MEMCG
(The following patch should be applied), I'll update this in the next
version if anyone agrees with this change.

Thanks.

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 9976b3f0d097..b5c4b62b5d2c 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -583,7 +583,9 @@ static bool __init kfence_init_pool(void)
                struct kfence_metadata *meta = &kfence_metadata[i];

                /* Initialize metadata. */
+#ifdef CONFIG_MEMCG
                slab->memcg_data = (unsigned long)&meta->objcg |
MEMCG_DATA_OBJCGS;
+#endif
                INIT_LIST_HEAD(&meta->list);
                raw_spin_lock_init(&meta->lock);
                meta->state = KFENCE_OBJECT_UNUSED;
@@ -940,7 +942,9 @@ void __kfence_free(void *addr)
 {
        struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);

+#ifdef CONFIG_MEMCG
        KFENCE_WARN_ON(meta->objcg);
+#endif
        /*
         * If the objects of the cache are SLAB_TYPESAFE_BY_RCU, defer freeing
         * the object, as the object page may be recycled for other-typed
diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h
index 6f0e1aece3f8..9a6c4b1b12a8 100644
--- a/mm/kfence/kfence.h
+++ b/mm/kfence/kfence.h
@@ -89,7 +89,9 @@ struct kfence_metadata {
        struct kfence_track free_track;
        /* For updating alloc_covered on frees. */
        u32 alloc_stack_hash;
+#ifdef CONFIG_MEMCG
        struct obj_cgroup *objcg;
+#endif
 };

Re: [PATCH 2/2] mm: kfence: fix objcgs vector allocation

[kernel test robot]

Hi Muchun,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hnaz-mm/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Muchun-Song/mm-kfence-fix-missing-objcg-housekeeping-for-SLAB/20220327-132038
base:   https://github.com/hnaz/linux-mm master
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220327/202203271634.QymsHESG-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0f6d9501cf49ce02937099350d08f20c4af86f3d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a33cf78311711db98d9f77541d0a4b50bc466875
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Muchun-Song/mm-kfence-fix-missing-objcg-housekeeping-for-SLAB/20220327-132038
        git checkout a33cf78311711db98d9f77541d0a4b50bc466875
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash mm/kfence/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> mm/kfence/core.c:593:36: warning: incompatible integer to pointer conversion passing 'unsigned long' to parameter of type 'const void *' [-Wint-conversion]
                   struct slab *slab = virt_to_slab(addr);
                                                    ^~~~
   mm/kfence/../slab.h:173:53: note: passing argument to parameter 'addr' here
   static inline struct slab *virt_to_slab(const void *addr)
                                                       ^
   mm/kfence/core.c:597:9: error: no member named 'memcg_data' in 'struct slab'
                   slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
                   ~~~~  ^
   mm/kfence/core.c:597:52: error: use of undeclared identifier 'MEMCG_DATA_OBJCGS'
                   slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
                                                                    ^
   1 warning and 2 errors generated.


vim +593 mm/kfence/core.c

   543	
   544	/*
   545	 * Initialization of the KFENCE pool after its allocation.
   546	 * Returns 0 on success; otherwise returns the address up to
   547	 * which partial initialization succeeded.
   548	 */
   549	static unsigned long kfence_init_pool(void)
   550	{
   551		unsigned long addr = (unsigned long)__kfence_pool;
   552		struct page *pages;
   553		int i;
   554	
   555		if (!arch_kfence_init_pool())
   556			return addr;
   557	
   558		pages = virt_to_page(addr);
   559	
   560		/*
   561		 * Set up object pages: they must have PG_slab set, to avoid freeing
   562		 * these as real pages.
   563		 *
   564		 * We also want to avoid inserting kfence_free() in the kfree()
   565		 * fast-path in SLUB, and therefore need to ensure kfree() correctly
   566		 * enters __slab_free() slow-path.
   567		 */
   568		for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
   569			if (!i || (i % 2))
   570				continue;
   571	
   572			/* Verify we do not have a compound head page. */
   573			if (WARN_ON(compound_head(&pages[i]) != &pages[i]))
   574				return addr;
   575	
   576			__SetPageSlab(&pages[i]);
   577		}
   578	
   579		/*
   580		 * Protect the first 2 pages. The first page is mostly unnecessary, and
   581		 * merely serves as an extended guard page. However, adding one
   582		 * additional page in the beginning gives us an even number of pages,
   583		 * which simplifies the mapping of address to metadata index.
   584		 */
   585		for (i = 0; i < 2; i++) {
   586			if (unlikely(!kfence_protect(addr)))
   587				return addr;
   588	
   589			addr += PAGE_SIZE;
   590		}
   591	
   592		for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
 > 593			struct slab *slab = virt_to_slab(addr);
   594			struct kfence_metadata *meta = &kfence_metadata[i];
   595	
   596			/* Initialize metadata. */
   597			slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
   598			INIT_LIST_HEAD(&meta->list);
   599			raw_spin_lock_init(&meta->lock);
   600			meta->state = KFENCE_OBJECT_UNUSED;
   601			meta->addr = addr; /* Initialize for validation in metadata_to_pageaddr(). */
   602			list_add_tail(&meta->list, &kfence_freelist);
   603	
   604			/* Protect the right redzone. */
   605			if (unlikely(!kfence_protect(addr + PAGE_SIZE)))
   606				return addr;
   607	
   608			addr += 2 * PAGE_SIZE;
   609		}
   610	
   611		/*
   612		 * The pool is live and will never be deallocated from this point on.
   613		 * Remove the pool object from the kmemleak object tree, as it would
   614		 * otherwise overlap with allocations returned by kfence_alloc(), which
   615		 * are registered with kmemleak through the slab post-alloc hook.
   616		 */
   617		kmemleak_free(__kfence_pool);
   618	
   619		return 0;
   620	}
   621	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/2] mm: kfence: fix objcgs vector allocation

[kernel test robot]

Hi Muchun,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hnaz-mm/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Muchun-Song/mm-kfence-fix-missing-objcg-housekeeping-for-SLAB/20220327-132038
base:   https://github.com/hnaz/linux-mm master
config: x86_64-randconfig-c022 (https://download.01.org/0day-ci/archive/20220327/202203271619.Ni4lY7Mc-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/a33cf78311711db98d9f77541d0a4b50bc466875
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Muchun-Song/mm-kfence-fix-missing-objcg-housekeeping-for-SLAB/20220327-132038
        git checkout a33cf78311711db98d9f77541d0a4b50bc466875
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash mm/kfence/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   mm/kfence/core.c: In function 'kfence_init_pool':
>> mm/kfence/core.c:593:36: warning: passing argument 1 of 'virt_to_slab' makes pointer from integer without a cast [-Wint-conversion]
     593 |   struct slab *slab = virt_to_slab(addr);
         |                                    ^~~~
         |                                    |
         |                                    long unsigned int
   In file included from mm/kfence/kfence.h:17,
                    from mm/kfence/core.c:35:
   mm/kfence/../slab.h:173:53: note: expected 'const void *' but argument is of type 'long unsigned int'
     173 | static inline struct slab *virt_to_slab(const void *addr)
         |                                         ~~~~~~~~~~~~^~~~
   mm/kfence/core.c:597:7: error: 'struct slab' has no member named 'memcg_data'
     597 |   slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
         |       ^~
   mm/kfence/core.c:597:52: error: 'MEMCG_DATA_OBJCGS' undeclared (first use in this function)
     597 |   slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
         |                                                    ^~~~~~~~~~~~~~~~~
   mm/kfence/core.c:597:52: note: each undeclared identifier is reported only once for each function it appears in


vim +/virt_to_slab +593 mm/kfence/core.c

   543	
   544	/*
   545	 * Initialization of the KFENCE pool after its allocation.
   546	 * Returns 0 on success; otherwise returns the address up to
   547	 * which partial initialization succeeded.
   548	 */
   549	static unsigned long kfence_init_pool(void)
   550	{
   551		unsigned long addr = (unsigned long)__kfence_pool;
   552		struct page *pages;
   553		int i;
   554	
   555		if (!arch_kfence_init_pool())
   556			return addr;
   557	
   558		pages = virt_to_page(addr);
   559	
   560		/*
   561		 * Set up object pages: they must have PG_slab set, to avoid freeing
   562		 * these as real pages.
   563		 *
   564		 * We also want to avoid inserting kfence_free() in the kfree()
   565		 * fast-path in SLUB, and therefore need to ensure kfree() correctly
   566		 * enters __slab_free() slow-path.
   567		 */
   568		for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
   569			if (!i || (i % 2))
   570				continue;
   571	
   572			/* Verify we do not have a compound head page. */
   573			if (WARN_ON(compound_head(&pages[i]) != &pages[i]))
   574				return addr;
   575	
   576			__SetPageSlab(&pages[i]);
   577		}
   578	
   579		/*
   580		 * Protect the first 2 pages. The first page is mostly unnecessary, and
   581		 * merely serves as an extended guard page. However, adding one
   582		 * additional page in the beginning gives us an even number of pages,
   583		 * which simplifies the mapping of address to metadata index.
   584		 */
   585		for (i = 0; i < 2; i++) {
   586			if (unlikely(!kfence_protect(addr)))
   587				return addr;
   588	
   589			addr += PAGE_SIZE;
   590		}
   591	
   592		for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
 > 593			struct slab *slab = virt_to_slab(addr);
   594			struct kfence_metadata *meta = &kfence_metadata[i];
   595	
   596			/* Initialize metadata. */
   597			slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
   598			INIT_LIST_HEAD(&meta->list);
   599			raw_spin_lock_init(&meta->lock);
   600			meta->state = KFENCE_OBJECT_UNUSED;
   601			meta->addr = addr; /* Initialize for validation in metadata_to_pageaddr(). */
   602			list_add_tail(&meta->list, &kfence_freelist);
   603	
   604			/* Protect the right redzone. */
   605			if (unlikely(!kfence_protect(addr + PAGE_SIZE)))
   606				return addr;
   607	
   608			addr += 2 * PAGE_SIZE;
   609		}
   610	
   611		/*
   612		 * The pool is live and will never be deallocated from this point on.
   613		 * Remove the pool object from the kmemleak object tree, as it would
   614		 * otherwise overlap with allocations returned by kfence_alloc(), which
   615		 * are registered with kmemleak through the slab post-alloc hook.
   616		 */
   617		kmemleak_free(__kfence_pool);
   618	
   619		return 0;
   620	}
   621	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/2] mm: kfence: fix objcgs vector allocation

[Marco Elver]

On Sun, 27 Mar 2022 at 07:19, Muchun Song <songmuchun@bytedance.com> wrote:
>
> If the kfence object is allocated to be used for objects vector, then
> this slot of the pool eventually being occupied permanently since
> the vector is never freed.  The solutions could be 1) freeing vector
> when the kfence object is freed or 2) allocating all vectors statically.
> Since the memory consumption of object vectors is low, it is better to
> chose 2) to fix the issue and it is also can reduce overhead of vectors
> allocating in the future.
>
> Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/kfence/core.c   | 3 +++
>  mm/kfence/kfence.h | 1 +
>  2 files changed, 4 insertions(+)

Thanks for this -- mostly looks good. Minor comments below + also
please fix what the test robot reported.

> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 13128fa13062..9976b3f0d097 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -579,9 +579,11 @@ static bool __init kfence_init_pool(void)
>         }
>
>         for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
> +               struct slab *slab = virt_to_slab(addr);
>                 struct kfence_metadata *meta = &kfence_metadata[i];
>
>                 /* Initialize metadata. */
> +               slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;

Maybe just move it to kfence_guarded_alloc(), see "/* Set required
slab fields */", where similar initialization on slab is done.

>                 INIT_LIST_HEAD(&meta->list);
>                 raw_spin_lock_init(&meta->lock);
>                 meta->state = KFENCE_OBJECT_UNUSED;
> @@ -938,6 +940,7 @@ void __kfence_free(void *addr)
>  {
>         struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
>
> +       KFENCE_WARN_ON(meta->objcg);

This holds true for both SLAB and SLUB, right? (I think it does, but
just double-checking.)

>         /*
>          * If the objects of the cache are SLAB_TYPESAFE_BY_RCU, defer freeing
>          * the object, as the object page may be recycled for other-typed
> diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h
> index 2a2d5de9d379..6f0e1aece3f8 100644
> --- a/mm/kfence/kfence.h
> +++ b/mm/kfence/kfence.h
> @@ -89,6 +89,7 @@ struct kfence_metadata {
>         struct kfence_track free_track;
>         /* For updating alloc_covered on frees. */
>         u32 alloc_stack_hash;
> +       struct obj_cgroup *objcg;
>  };
>
>  extern struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS];
> --
> 2.11.0
>

Re: [PATCH 2/2] mm: kfence: fix objcgs vector allocation

[Muchun Song]

On Mon, Mar 28, 2022 at 1:31 AM Marco Elver <elver@google.com> wrote:
>
> On Sun, 27 Mar 2022 at 07:19, Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > If the kfence object is allocated to be used for objects vector, then
> > this slot of the pool eventually being occupied permanently since
> > the vector is never freed.  The solutions could be 1) freeing vector
> > when the kfence object is freed or 2) allocating all vectors statically.
> > Since the memory consumption of object vectors is low, it is better to
> > chose 2) to fix the issue and it is also can reduce overhead of vectors
> > allocating in the future.
> >
> > Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/kfence/core.c   | 3 +++
> >  mm/kfence/kfence.h | 1 +
> >  2 files changed, 4 insertions(+)
>
> Thanks for this -- mostly looks good. Minor comments below + also
> please fix what the test robot reported.

Will do.

>
> > diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> > index 13128fa13062..9976b3f0d097 100644
> > --- a/mm/kfence/core.c
> > +++ b/mm/kfence/core.c
> > @@ -579,9 +579,11 @@ static bool __init kfence_init_pool(void)
> >         }
> >
> >         for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
> > +               struct slab *slab = virt_to_slab(addr);
> >                 struct kfence_metadata *meta = &kfence_metadata[i];
> >
> >                 /* Initialize metadata. */
> > +               slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
>
> Maybe just move it to kfence_guarded_alloc(), see "/* Set required
> slab fields */", where similar initialization on slab is done.

But slab->memcg_data is special since it is only needed to be
initialized once.  I think it is better move it to the place where
__SetPageSlab(&pages[i]) is.  What do you think?

>
> >                 INIT_LIST_HEAD(&meta->list);
> >                 raw_spin_lock_init(&meta->lock);
> >                 meta->state = KFENCE_OBJECT_UNUSED;
> > @@ -938,6 +940,7 @@ void __kfence_free(void *addr)
> >  {
> >         struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
> >
> > +       KFENCE_WARN_ON(meta->objcg);
>
> This holds true for both SLAB and SLUB, right? (I think it does, but
> just double-checking.)

Right.

Thanks.

Re: [PATCH 2/2] mm: kfence: fix objcgs vector allocation

[Marco Elver]

On Mon, 28 Mar 2022 at 03:53, Muchun Song <songmuchun@bytedance.com> wrote:
>
> On Mon, Mar 28, 2022 at 1:31 AM Marco Elver <elver@google.com> wrote:
> >
> > On Sun, 27 Mar 2022 at 07:19, Muchun Song <songmuchun@bytedance.com> wrote:
> > >
> > > If the kfence object is allocated to be used for objects vector, then
> > > this slot of the pool eventually being occupied permanently since
> > > the vector is never freed.  The solutions could be 1) freeing vector
> > > when the kfence object is freed or 2) allocating all vectors statically.
> > > Since the memory consumption of object vectors is low, it is better to
> > > chose 2) to fix the issue and it is also can reduce overhead of vectors
> > > allocating in the future.
> > >
> > > Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
> > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > ---
> > >  mm/kfence/core.c   | 3 +++
> > >  mm/kfence/kfence.h | 1 +
> > >  2 files changed, 4 insertions(+)
> >
> > Thanks for this -- mostly looks good. Minor comments below + also
> > please fix what the test robot reported.
>
> Will do.
>
> >
> > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> > > index 13128fa13062..9976b3f0d097 100644
> > > --- a/mm/kfence/core.c
> > > +++ b/mm/kfence/core.c
> > > @@ -579,9 +579,11 @@ static bool __init kfence_init_pool(void)
> > >         }
> > >
> > >         for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
> > > +               struct slab *slab = virt_to_slab(addr);
> > >                 struct kfence_metadata *meta = &kfence_metadata[i];
> > >
> > >                 /* Initialize metadata. */
> > > +               slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
> >
> > Maybe just move it to kfence_guarded_alloc(), see "/* Set required
> > slab fields */", where similar initialization on slab is done.
>
> But slab->memcg_data is special since it is only needed to be
> initialized once.  I think it is better move it to the place where
> __SetPageSlab(&pages[i]) is.  What do you think?

That's fair.

Re: [PATCH 1/2] mm: kfence: fix missing objcg housekeeping for SLAB

[Linus Torvalds]

On Sat, Mar 26, 2022 at 10:19 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The objcg is not cleared and put for kfence object when it is freed, which
> could lead to memory leak for struct obj_cgroup and wrong statistics of
> NR_SLAB_RECLAIMABLE_B or NR_SLAB_UNRECLAIMABLE_B.  Since the last freed
> object's objcg is not cleared, mem_cgroup_from_obj() could return the wrong
> memcg when this kfence object, which is not charged to any objcgs, is
> reallocated to other users.  A real word issue [1] is caused by this bug.

Good that this looks sorted out.

Patch 2/2 seems to still be up in the air. The patch not only causes
build errors, but it looks really very odd to me.

In particular, you do that loop with

                __SetPageSlab(&pages[i]);

in kfence_init_pool(), but that is *not* where you set the
MEMCG_DATA_OBJCGS, and instead do that virt_to_slab(addr) dance later.

That looks very odd to me. I think the two should go hand-in-hand,
since that __SetPageSlab() really is what makes it a slab thing, and I
think it should go together with setting the slab state correctly.

Finally, is there a syzbot report for that second problem?

Anyway, should I apply this PATCH 1/2 now directly as the solution for
the dentry issue, or should I wait for that second patch? They seem to
be related only indirectly, in that the problems were both introduced
by the same commit.

                     Linus

Re: [PATCH 1/2] mm: kfence: fix missing objcg housekeeping for SLAB

[Muchun Song]

On Mon, Mar 28, 2022 at 5:08 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Mar 26, 2022 at 10:19 PM Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > The objcg is not cleared and put for kfence object when it is freed, which
> > could lead to memory leak for struct obj_cgroup and wrong statistics of
> > NR_SLAB_RECLAIMABLE_B or NR_SLAB_UNRECLAIMABLE_B.  Since the last freed
> > object's objcg is not cleared, mem_cgroup_from_obj() could return the wrong
> > memcg when this kfence object, which is not charged to any objcgs, is
> > reallocated to other users.  A real word issue [1] is caused by this bug.
>
> Good that this looks sorted out.
>
> Patch 2/2 seems to still be up in the air. The patch not only causes
> build errors, but it looks really very odd to me.
>
> In particular, you do that loop with
>
>                 __SetPageSlab(&pages[i]);
>
> in kfence_init_pool(), but that is *not* where you set the
> MEMCG_DATA_OBJCGS, and instead do that virt_to_slab(addr) dance later.
>
> That looks very odd to me. I think the two should go hand-in-hand,
> since that __SetPageSlab() really is what makes it a slab thing, and I
> think it should go together with setting the slab state correctly.

Right. It is a little odd. I'll improve it in the next version.

>
> Finally, is there a syzbot report for that second problem?

No. The second bug does not trigger any oops, so it is hard to be seen.
It is just my code review.

>
> Anyway, should I apply this PATCH 1/2 now directly as the solution for
> the dentry issue, or should I wait for that second patch? They seem to
> be related only indirectly, in that the problems were both introduced
> by the same commit.
>

I think you could apply PATCH 1/2 now.  PATCH 2/2 is another issue not
related to dentry issue.

Thanks.