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: [email protected]
Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
Signed-off-by: Muchun Song <[email protected]>
---
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
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 <[email protected]>
---
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
On Sun, 27 Mar 2022 at 07:19, Muchun Song <[email protected]> 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 <[email protected]>
> ---
> 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
>