| Message ID | 20220328132843.16624-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm: kfence: fix objcgs vector allocation | expand |
On Mon, 28 Mar 2022 at 15:28, 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> Reviewed-by: Marco Elver <elver@google.com> Btw, how did you test this? Thanks, -- Marco > --- > v2: > - Fix compiler error reported by kernel test robot <lkp@intel.com>. > > mm/kfence/core.c | 11 ++++++++++- > mm/kfence/kfence.h | 3 +++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index 13128fa13062..d4c7978cd75e 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -555,6 +555,8 @@ static bool __init kfence_init_pool(void) > * enters __slab_free() slow-path. > */ > for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) { > + struct slab *slab = page_slab(&pages[i]); > + > if (!i || (i % 2)) > continue; > > @@ -562,7 +564,11 @@ static bool __init kfence_init_pool(void) > if (WARN_ON(compound_head(&pages[i]) != &pages[i])) > goto err; > > - __SetPageSlab(&pages[i]); > + __folio_set_slab(slab_folio(slab)); > +#ifdef CONFIG_MEMCG > + slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 1].objcg | > + MEMCG_DATA_OBJCGS; > +#endif > } > > /* > @@ -938,6 +944,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 2a2d5de9d379..9a6c4b1b12a8 100644 > --- a/mm/kfence/kfence.h > +++ b/mm/kfence/kfence.h > @@ -89,6 +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 > }; > > extern struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS]; > -- > 2.11.0 >
On Mon, Mar 28, 2022 at 11:43 PM Marco Elver <elver@google.com> wrote: > > On Mon, 28 Mar 2022 at 15:28, 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> > > Reviewed-by: Marco Elver <elver@google.com> Thanks. > > Btw, how did you test this? > Yeah. No problem.
On Mon, Mar 28, 2022 at 11:51 PM Muchun Song <songmuchun@bytedance.com> wrote: > > On Mon, Mar 28, 2022 at 11:43 PM Marco Elver <elver@google.com> wrote: > > > > On Mon, 28 Mar 2022 at 15:28, 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> > > > > Reviewed-by: Marco Elver <elver@google.com> > > Thanks. > > > > > Btw, how did you test this? > > I have tested it with syzkaller with the following configs. And I didn't find any issues. CONFIG_KFENCE=y CONFIG_KFENCE_SAMPLE_INTERVAL=10 CONFIG_KFENCE_NUM_OBJECTS=2550 CONFIG_KFENCE_DEFERRABLE=n CONFIG_KFENCE_STATIC_KEYS=y CONFIG_KFENCE_STRESS_TEST_FAULTS=0
On Mon, 28 Mar 2022 at 17:54, Muchun Song <songmuchun@bytedance.com> wrote: [...] > > > > > > Btw, how did you test this? > > > > > I have tested it with syzkaller with the following configs. > And I didn't find any issues. > > CONFIG_KFENCE=y > CONFIG_KFENCE_SAMPLE_INTERVAL=10 > CONFIG_KFENCE_NUM_OBJECTS=2550 > CONFIG_KFENCE_DEFERRABLE=n > CONFIG_KFENCE_STATIC_KEYS=y > CONFIG_KFENCE_STRESS_TEST_FAULTS=0 Hmm, I would have expected that you have some definitive test case that shows the issue, and with the patch the issue is gone. Were there issues triggered by syzkaller w/o this patch? Thanks, -- Marco
On Mon, Mar 28, 2022 at 09:28:43PM +0800, Muchun Song 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> > --- > v2: > - Fix compiler error reported by kernel test robot <lkp@intel.com>. Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> LGTM, thanks! > > mm/kfence/core.c | 11 ++++++++++- > mm/kfence/kfence.h | 3 +++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index 13128fa13062..d4c7978cd75e 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -555,6 +555,8 @@ static bool __init kfence_init_pool(void) > * enters __slab_free() slow-path. > */ > for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) { > + struct slab *slab = page_slab(&pages[i]); > + > if (!i || (i % 2)) > continue; > > @@ -562,7 +564,11 @@ static bool __init kfence_init_pool(void) > if (WARN_ON(compound_head(&pages[i]) != &pages[i])) > goto err; > > - __SetPageSlab(&pages[i]); > + __folio_set_slab(slab_folio(slab)); > +#ifdef CONFIG_MEMCG > + slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 1].objcg | > + MEMCG_DATA_OBJCGS; > +#endif We can probably put CONFIG_MEMCG_KMEM here, but it doesn't matter that much. In the long run we should get rid of CONFIG_MEMCG_KMEM anyway.
On Tue, Mar 29, 2022 at 2:58 AM Marco Elver <elver@google.com> wrote: > > On Mon, 28 Mar 2022 at 17:54, Muchun Song <songmuchun@bytedance.com> wrote: > [...] > > > > > > > > Btw, how did you test this? > > > > > > > > I have tested it with syzkaller with the following configs. > > And I didn't find any issues. > > > > CONFIG_KFENCE=y > > CONFIG_KFENCE_SAMPLE_INTERVAL=10 > > CONFIG_KFENCE_NUM_OBJECTS=2550 > > CONFIG_KFENCE_DEFERRABLE=n > > CONFIG_KFENCE_STATIC_KEYS=y > > CONFIG_KFENCE_STRESS_TEST_FAULTS=0 > > Hmm, I would have expected that you have some definitive test case > that shows the issue, and with the patch the issue is gone. Were there > issues triggered by syzkaller w/o this patch? > I have tested this patch with the following patch and without this patch. Then we'll see the BUG_ON meaning both objcg vector and object are allocated from kfence pool. diff --git a/mm/slab.h b/mm/slab.h index c7f2abc2b154..1d8d15522a2e 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -519,6 +519,8 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s, continue; } + BUG_ON(is_kfence_address(p[i]) && is_kfence_address(slab_objcgs(slab))); + off = obj_to_index(s, slab, p[i]); obj_cgroup_get(objcg); slab_objcgs(slab)[off] = objcg;
diff --git a/mm/kfence/core.c b/mm/kfence/core.c index 13128fa13062..d4c7978cd75e 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -555,6 +555,8 @@ static bool __init kfence_init_pool(void) * enters __slab_free() slow-path. */ for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) { + struct slab *slab = page_slab(&pages[i]); + if (!i || (i % 2)) continue; @@ -562,7 +564,11 @@ static bool __init kfence_init_pool(void) if (WARN_ON(compound_head(&pages[i]) != &pages[i])) goto err; - __SetPageSlab(&pages[i]); + __folio_set_slab(slab_folio(slab)); +#ifdef CONFIG_MEMCG + slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 1].objcg | + MEMCG_DATA_OBJCGS; +#endif } /* @@ -938,6 +944,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 2a2d5de9d379..9a6c4b1b12a8 100644 --- a/mm/kfence/kfence.h +++ b/mm/kfence/kfence.h @@ -89,6 +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 }; extern struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS];
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> --- v2: - Fix compiler error reported by kernel test robot <lkp@intel.com>. mm/kfence/core.c | 11 ++++++++++- mm/kfence/kfence.h | 3 +++ 2 files changed, 13 insertions(+), 1 deletion(-)