| Message ID | 20200610163135.17364-10-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | slub_debug fixes and improvements | expand |
On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote: > The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: > always get the cache from its page in kmem_cache_free()") to support kmemcg, > where per-memcg cache can be different from the root one, so we can't use > the kmem_cache pointer given to kmem_cache_free(). > > Prior to that commit, SLUB already had debugging check+warning that could be > enabled to compare the given kmem_cache pointer to one referenced by the slab > page where the object-to-be-freed resides. This check was moved to > cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED > configs by commit 598a0717a816 ("mm/slab: validate cache membership under > freelist hardening"). > > These checks and warnings can be useful especially for the debugging, which can > be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to > WARN_ONCE() so only the first hit is now reported, others are silent. This > patch changes it to WARN() so that all errors are reported. > > It's also useful to print SLUB allocation/free tracking info for the offending > object, if tracking is enabled. We could export the SLUB print_tracking() > function and provide an empty one for SLAB, or realize that both the debugging > and hardening cases in cache_from_obj() are only supported by SLUB anyway. So > this patch moves cache_from_obj() from slab.h to separate instances in slab.c > and slub.c, where the SLAB version only does the kmemcg lookup and even could > be completely removed once the kmemcg rework [1] is merged. The SLUB version > can thus easily use the print_tracking() function. It can also use the > kmem_cache_debug_flags() static key check for improved performance in kernels > without the hardening and with debugging not enabled on boot. > > [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/slab.c | 8 ++++++++ > mm/slab.h | 23 ----------------------- > mm/slub.c | 21 +++++++++++++++++++++ > 3 files changed, 29 insertions(+), 23 deletions(-) > > diff --git a/mm/slab.c b/mm/slab.c > index 9350062ffc1a..6134c4c36d4c 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller) > } > EXPORT_SYMBOL(__kmalloc_track_caller); > > +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > +{ > + if (memcg_kmem_enabled()) > + return virt_to_cache(x); > + else > + return s; > +} Hm, it looks like all the SLAB version doesn't perform any sanity checks anymore. Is it intended? Also, Is it ever possible that s != virt_to_cache(x) if there are no bugs? kmem_cache_free_bulk() in slab.c does contain the following: if (!orig_s) /* called via kfree_bulk */ s = virt_to_cache(objp); else s = cache_from_obj(orig_s, objp); which looks a bit strange with the version above. > + > /** > * kmem_cache_free - Deallocate an object > * @cachep: The cache the allocation was from. > diff --git a/mm/slab.h b/mm/slab.h > index 815e4e9a94cd..c0c4244f75da 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -503,29 +503,6 @@ static __always_inline void uncharge_slab_page(struct page *page, int order, > memcg_uncharge_slab(page, order, s); > } > > -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > -{ > - struct kmem_cache *cachep; > - > - /* > - * When kmemcg is not being used, both assignments should return the > - * same value. but we don't want to pay the assignment price in that > - * case. If it is not compiled in, the compiler should be smart enough > - * to not do even the assignment. In that case, slab_equal_or_root > - * will also be a constant. > - */ > - if (!memcg_kmem_enabled() && > - !IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > - !unlikely(s->flags & SLAB_CONSISTENCY_CHECKS)) > - return s; > - > - cachep = virt_to_cache(x); > - WARN_ONCE(cachep && !slab_equal_or_root(cachep, s), > - "%s: Wrong slab cache. %s but object is from %s\n", > - __func__, s->name, cachep->name); > - return cachep; > -} > - > static inline size_t slab_ksize(const struct kmem_cache *s) > { > #ifndef CONFIG_SLUB > diff --git a/mm/slub.c b/mm/slub.c > index efb08f2e9c66..f7a1d8537674 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -1524,6 +1524,10 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, > { > return false; > } > + > +static void print_tracking(struct kmem_cache *s, void *object) > +{ > +} > #endif /* CONFIG_SLUB_DEBUG */ > > /* > @@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr) > } > #endif > > +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > +{ > + struct kmem_cache *cachep; > + > + if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > + !memcg_kmem_enabled() && > + !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > + return s; > + > + cachep = virt_to_cache(x); > + if (WARN(cachep && !slab_equal_or_root(cachep, s), > + "%s: Wrong slab cache. %s but object is from %s\n", > + __func__, s->name, cachep->name)) > + print_tracking(cachep, x); > + return cachep; > +} Maybe we can define a trivial SLAB version of kmem_cache_debug_flags() and keep a single version of cache_from_obj()? Thank you!
On 6/11/20 12:46 AM, Roman Gushchin wrote: > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote: >> @@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller) >> } >> EXPORT_SYMBOL(__kmalloc_track_caller); >> >> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) >> +{ >> + if (memcg_kmem_enabled()) >> + return virt_to_cache(x); >> + else >> + return s; >> +} > > Hm, it looks like all the SLAB version doesn't perform any sanity checks anymore. > Is it intended? Yes, it was the same before commit b9ce5ef49f00. The commit could have been more precise - kmemcg needs virt_to_cache(), but not the sanity check. The SLUB version also shouldn't really be doing the sanity check if only memcg_kmem_enabled() is true (and not the debugging/hardening), but the code then looks ugly and I hope this will just fix itself with your kmemcg slab rework. > Also, Is it ever possible that s != virt_to_cache(x) if there are no bugs? Well, only in the kmemcg case it should be possible. > kmem_cache_free_bulk() in slab.c does contain the following: > if (!orig_s) /* called via kfree_bulk */ > s = virt_to_cache(objp); > else > s = cache_from_obj(orig_s, objp); > which looks a bit strange with the version above. Looks fine to me. If we are called with non-NULL s, and kmemcg is not enabled, we can just trust s. If we are called with NULL s (via kfree_bulk()) we need to get cache from the object, even if kmemcg is not enabled, so we do virt_to_cache() unconditionally. Once your series is fully accepted, we can remove SLAB's cache_from_obj() and the whole 'else' part in the hunk above. Or am I missing something? >> @@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr) >> } >> #endif >> >> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) >> +{ >> + struct kmem_cache *cachep; >> + >> + if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && >> + !memcg_kmem_enabled() && >> + !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) >> + return s; >> + >> + cachep = virt_to_cache(x); >> + if (WARN(cachep && !slab_equal_or_root(cachep, s), >> + "%s: Wrong slab cache. %s but object is from %s\n", >> + __func__, s->name, cachep->name)) >> + print_tracking(cachep, x); >> + return cachep; >> +} > > Maybe we can define a trivial SLAB version of kmem_cache_debug_flags() > and keep a single version of cache_from_obj()? I think the result would be more obfuscated than just making it plain that SLAB doesn't have those SLUB features. And I still hope SLAB's version will go away completely. If your series is accepted first, then this patch based in that will not introduce slab.c cache_from_obj() at all. > Thank you! >
On Thu, Jun 11, 2020 at 11:56:53AM +0200, Vlastimil Babka wrote: > On 6/11/20 12:46 AM, Roman Gushchin wrote: > > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote: > >> @@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller) > >> } > >> EXPORT_SYMBOL(__kmalloc_track_caller); > >> > >> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > >> +{ > >> + if (memcg_kmem_enabled()) > >> + return virt_to_cache(x); > >> + else > >> + return s; > >> +} > > > > Hm, it looks like all the SLAB version doesn't perform any sanity checks anymore. > > Is it intended? > > Yes, it was the same before commit b9ce5ef49f00. The commit could have been more > precise - kmemcg needs virt_to_cache(), but not the sanity check. The SLUB > version also shouldn't really be doing the sanity check if only > memcg_kmem_enabled() is true (and not the debugging/hardening), but the code > then looks ugly and I hope this will just fix itself with your kmemcg slab rework. Got it. > > > Also, Is it ever possible that s != virt_to_cache(x) if there are no bugs? > > Well, only in the kmemcg case it should be possible. > > > kmem_cache_free_bulk() in slab.c does contain the following: > > if (!orig_s) /* called via kfree_bulk */ > > s = virt_to_cache(objp); > > else > > s = cache_from_obj(orig_s, objp); > > which looks a bit strange with the version above. > > Looks fine to me. If we are called with non-NULL s, and kmemcg is not enabled, > we can just trust s. If we are called with NULL s (via kfree_bulk()) we need to > get cache from the object, even if kmemcg is not enabled, so we do > virt_to_cache() unconditionally. > Once your series is fully accepted, we can remove SLAB's cache_from_obj() and > the whole 'else' part in the hunk above. Or am I missing something? Right. I guess there will be even more cleanups possible, let's see where we'll end up. It looks like nothing prevents it from being queued for 5.9 after 5.8-rc1 will be out, right? > > > >> @@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr) > >> } > >> #endif > >> > >> +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > >> +{ > >> + struct kmem_cache *cachep; > >> + > >> + if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > >> + !memcg_kmem_enabled() && > >> + !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > >> + return s; > >> + > >> + cachep = virt_to_cache(x); > >> + if (WARN(cachep && !slab_equal_or_root(cachep, s), > >> + "%s: Wrong slab cache. %s but object is from %s\n", > >> + __func__, s->name, cachep->name)) > >> + print_tracking(cachep, x); > >> + return cachep; > >> +} > > > > Maybe we can define a trivial SLAB version of kmem_cache_debug_flags() > > and keep a single version of cache_from_obj()? > > I think the result would be more obfuscated than just making it plain that SLAB > doesn't have those SLUB features. And I still hope SLAB's version will go away > completely. If your series is accepted first, then this patch based in that will > not introduce slab.c cache_from_obj() at all. Ok, makes sense to me. Thank you!
On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote: > The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: > always get the cache from its page in kmem_cache_free()") to support kmemcg, > where per-memcg cache can be different from the root one, so we can't use > the kmem_cache pointer given to kmem_cache_free(). > > Prior to that commit, SLUB already had debugging check+warning that could be > enabled to compare the given kmem_cache pointer to one referenced by the slab > page where the object-to-be-freed resides. This check was moved to > cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED > configs by commit 598a0717a816 ("mm/slab: validate cache membership under > freelist hardening"). > > These checks and warnings can be useful especially for the debugging, which can > be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to > WARN_ONCE() so only the first hit is now reported, others are silent. This > patch changes it to WARN() so that all errors are reported. > > It's also useful to print SLUB allocation/free tracking info for the offending > object, if tracking is enabled. We could export the SLUB print_tracking() > function and provide an empty one for SLAB, or realize that both the debugging > and hardening cases in cache_from_obj() are only supported by SLUB anyway. So > this patch moves cache_from_obj() from slab.h to separate instances in slab.c > and slub.c, where the SLAB version only does the kmemcg lookup and even could Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB), and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to cover SLAB and SLOB. The point being: I still want the sanity check for the SLAB case under hardening. This needs to stay a common function. The whole point is to catch corruption from the wrong kmem_cache * being associated with an object, and that's agnostic of slab/slub/slob. So, I'll send a follow-up to this patch to actually do what I had originally intended for 598a0717a816 ("mm/slab: validate cache membership under freelist hardening"), which wasn't intended to be SLUB-specific.
On 6/17/20 7:49 PM, Kees Cook wrote: > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote: >> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: >> always get the cache from its page in kmem_cache_free()") to support kmemcg, >> where per-memcg cache can be different from the root one, so we can't use >> the kmem_cache pointer given to kmem_cache_free(). >> >> Prior to that commit, SLUB already had debugging check+warning that could be >> enabled to compare the given kmem_cache pointer to one referenced by the slab >> page where the object-to-be-freed resides. This check was moved to >> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED >> configs by commit 598a0717a816 ("mm/slab: validate cache membership under >> freelist hardening"). >> >> These checks and warnings can be useful especially for the debugging, which can >> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to >> WARN_ONCE() so only the first hit is now reported, others are silent. This >> patch changes it to WARN() so that all errors are reported. >> >> It's also useful to print SLUB allocation/free tracking info for the offending >> object, if tracking is enabled. We could export the SLUB print_tracking() >> function and provide an empty one for SLAB, or realize that both the debugging >> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So >> this patch moves cache_from_obj() from slab.h to separate instances in slab.c >> and slub.c, where the SLAB version only does the kmemcg lookup and even could > > Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED > here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB), > and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to > cover SLAB and SLOB. > > The point being: I still want the sanity check for the SLAB case under > hardening. This needs to stay a common function. The whole point is > to catch corruption from the wrong kmem_cache * being associated with > an object, and that's agnostic of slab/slub/slob. > > So, I'll send a follow-up to this patch to actually do what I had > originally intended for 598a0717a816 ("mm/slab: validate cache membership > under freelist hardening"), which wasn't intended to be SLUB-specific. To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I think it's best if we modify my patch. The patch below should be squashed into the current version in mmots, with the commit log used for the whole result. This will cause conflicts while reapplying Roman's mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which can be fixed by a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c b) applying this hunk instead: --- a/mm/slab.h +++ b/mm/slab.h @@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) struct kmem_cache *cachep; if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && - !memcg_kmem_enabled() && !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) return s; cachep = virt_to_cache(x); - if (WARN(cachep && !slab_equal_or_root(cachep, s), + if (WARN(cachep && cachep != s, "%s: Wrong slab cache. %s but object is from %s\n", __func__, s->name, cachep->name)) print_tracking(cachep, x); The fixup patch itself: ----8<---- From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Thu, 18 Jun 2020 11:52:03 +0200 Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of cache_from_obj() The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: always get the cache from its page in kmem_cache_free()") to support kmemcg, where per-memcg cache can be different from the root one, so we can't use the kmem_cache pointer given to kmem_cache_free(). Prior to that commit, SLUB already had debugging check+warning that could be enabled to compare the given kmem_cache pointer to one referenced by the slab page where the object-to-be-freed resides. This check was moved to cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate cache membership under freelist hardening"). These checks and warnings can be useful especially for the debugging, which can be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported, others are silent. This patch changes it to WARN() so that all errors are reported. It's also useful to print SLUB allocation/free tracking info for the offending object, if tracking is enabled. Thus, export the SLUB print_tracking() function and provide an empty one for SLAB. For SLUB we can also benefit from the static key check in kmem_cache_debug_flags(), but we need to move this function to slab.h and declare the static key there. [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slab.c | 8 -------- mm/slab.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ mm/slub.c | 38 +------------------------------------- 3 files changed, 46 insertions(+), 45 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index 6134c4c36d4c..9350062ffc1a 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3672,14 +3672,6 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller) } EXPORT_SYMBOL(__kmalloc_track_caller); -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) -{ - if (memcg_kmem_enabled()) - return virt_to_cache(x); - else - return s; -} - /** * kmem_cache_free - Deallocate an object * @cachep: The cache the allocation was from. diff --git a/mm/slab.h b/mm/slab.h index a2696d306b62..a9f5ba9ce9a7 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -275,6 +275,34 @@ static inline int cache_vmstat_idx(struct kmem_cache *s) NR_SLAB_RECLAIMABLE : NR_SLAB_UNRECLAIMABLE; } +#ifdef CONFIG_SLUB_DEBUG +#ifdef CONFIG_SLUB_DEBUG_ON +DECLARE_STATIC_KEY_TRUE(slub_debug_enabled); +#else +DECLARE_STATIC_KEY_FALSE(slub_debug_enabled); +#endif +extern void print_tracking(struct kmem_cache *s, void *object); +#else +static inline void print_tracking(struct kmem_cache *s, void *object) +{ +} +#endif + +/* + * Returns true if any of the specified slub_debug flags is enabled for the + * cache. Use only for flags parsed by setup_slub_debug() as it also enables + * the static key. + */ +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) +{ + VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); +#ifdef CONFIG_SLUB_DEBUG + if (static_branch_unlikely(&slub_debug_enabled)) + return s->flags & flags; +#endif + return false; +} + #ifdef CONFIG_MEMCG_KMEM /* List of all root caches. */ @@ -503,6 +531,23 @@ static __always_inline void uncharge_slab_page(struct page *page, int order, memcg_uncharge_slab(page, order, s); } +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) +{ + struct kmem_cache *cachep; + + if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && + !memcg_kmem_enabled() && + !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) + return s; + + cachep = virt_to_cache(x); + if (WARN(cachep && !slab_equal_or_root(cachep, s), + "%s: Wrong slab cache. %s but object is from %s\n", + __func__, s->name, cachep->name)) + print_tracking(cachep, x); + return cachep; +} + static inline size_t slab_ksize(const struct kmem_cache *s) { #ifndef CONFIG_SLUB diff --git a/mm/slub.c b/mm/slub.c index 202fb423d195..0e635a8aa340 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -122,21 +122,6 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); #endif #endif -/* - * Returns true if any of the specified slub_debug flags is enabled for the - * cache. Use only for flags parsed by setup_slub_debug() as it also enables - * the static key. - */ -static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) -{ - VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); -#ifdef CONFIG_SLUB_DEBUG - if (static_branch_unlikely(&slub_debug_enabled)) - return s->flags & flags; -#endif - return false; -} - static inline bool kmem_cache_debug(struct kmem_cache *s) { return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS); @@ -653,7 +638,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time) #endif } -static void print_tracking(struct kmem_cache *s, void *object) +void print_tracking(struct kmem_cache *s, void *object) { unsigned long pr_time = jiffies; if (!(s->flags & SLAB_STORE_USER)) @@ -1525,10 +1510,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, { return false; } - -static void print_tracking(struct kmem_cache *s, void *object) -{ -} #endif /* CONFIG_SLUB_DEBUG */ /* @@ -3180,23 +3161,6 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr) } #endif -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) -{ - struct kmem_cache *cachep; - - if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && - !memcg_kmem_enabled() && - !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) - return s; - - cachep = virt_to_cache(x); - if (WARN(cachep && !slab_equal_or_root(cachep, s), - "%s: Wrong slab cache. %s but object is from %s\n", - __func__, s->name, cachep->name)) - print_tracking(cachep, x); - return cachep; -} - void kmem_cache_free(struct kmem_cache *s, void *x) { s = cache_from_obj(s, x);
On Thu, Jun 18, 2020 at 12:10:38PM +0200, Vlastimil Babka wrote: > > On 6/17/20 7:49 PM, Kees Cook wrote: > > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote: > >> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: > >> always get the cache from its page in kmem_cache_free()") to support kmemcg, > >> where per-memcg cache can be different from the root one, so we can't use > >> the kmem_cache pointer given to kmem_cache_free(). > >> > >> Prior to that commit, SLUB already had debugging check+warning that could be > >> enabled to compare the given kmem_cache pointer to one referenced by the slab > >> page where the object-to-be-freed resides. This check was moved to > >> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED > >> configs by commit 598a0717a816 ("mm/slab: validate cache membership under > >> freelist hardening"). > >> > >> These checks and warnings can be useful especially for the debugging, which can > >> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to > >> WARN_ONCE() so only the first hit is now reported, others are silent. This > >> patch changes it to WARN() so that all errors are reported. > >> > >> It's also useful to print SLUB allocation/free tracking info for the offending > >> object, if tracking is enabled. We could export the SLUB print_tracking() > >> function and provide an empty one for SLAB, or realize that both the debugging > >> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So > >> this patch moves cache_from_obj() from slab.h to separate instances in slab.c > >> and slub.c, where the SLAB version only does the kmemcg lookup and even could > > > > Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED > > here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB), > > and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to > > cover SLAB and SLOB. > > > > The point being: I still want the sanity check for the SLAB case under > > hardening. This needs to stay a common function. The whole point is > > to catch corruption from the wrong kmem_cache * being associated with > > an object, and that's agnostic of slab/slub/slob. > > > > So, I'll send a follow-up to this patch to actually do what I had > > originally intended for 598a0717a816 ("mm/slab: validate cache membership > > under freelist hardening"), which wasn't intended to be SLUB-specific. > > To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I > think it's best if we modify my patch. The patch below should be squashed into > the current version in mmots, with the commit log used for the whole result. > > This will cause conflicts while reapplying Roman's > mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which > can be fixed by > a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c > b) applying this hunk instead: > > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > struct kmem_cache *cachep; > > if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > - !memcg_kmem_enabled() && > !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > return s; > > cachep = virt_to_cache(x); > - if (WARN(cachep && !slab_equal_or_root(cachep, s), > + if (WARN(cachep && cachep != s, > "%s: Wrong slab cache. %s but object is from %s\n", > __func__, s->name, cachep->name)) > print_tracking(cachep, x); > > The fixup patch itself: > ----8<---- > From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 18 Jun 2020 11:52:03 +0200 > Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of > cache_from_obj() > > The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: > always get the cache from its page in kmem_cache_free()") to support > kmemcg, where per-memcg cache can be different from the root one, so we > can't use the kmem_cache pointer given to kmem_cache_free(). > > Prior to that commit, SLUB already had debugging check+warning that could > be enabled to compare the given kmem_cache pointer to one referenced by > the slab page where the object-to-be-freed resides. This check was moved > to cache_from_obj(). Later the check was also enabled for > SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate > cache membership under freelist hardening"). > > These checks and warnings can be useful especially for the debugging, > which can be improved. Commit 598a0717a816 changed the pr_err() with > WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported, > others are silent. This patch changes it to WARN() so that all errors are > reported. > > It's also useful to print SLUB allocation/free tracking info for the offending > object, if tracking is enabled. Thus, export the SLUB print_tracking() function > and provide an empty one for SLAB. > > For SLUB we can also benefit from the static key check in > kmem_cache_debug_flags(), but we need to move this function to slab.h and > declare the static key there. > > [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Acked-by: Kees Cook <keescook@chromium.org> I will rebase my fix for SLAB_FREELIST_HARDENED coverage on this. Thanks!
On Thu, Jun 18, 2020 at 12:10:38PM +0200, Vlastimil Babka wrote: > > On 6/17/20 7:49 PM, Kees Cook wrote: > > On Wed, Jun 10, 2020 at 06:31:35PM +0200, Vlastimil Babka wrote: > >> The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: > >> always get the cache from its page in kmem_cache_free()") to support kmemcg, > >> where per-memcg cache can be different from the root one, so we can't use > >> the kmem_cache pointer given to kmem_cache_free(). > >> > >> Prior to that commit, SLUB already had debugging check+warning that could be > >> enabled to compare the given kmem_cache pointer to one referenced by the slab > >> page where the object-to-be-freed resides. This check was moved to > >> cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED > >> configs by commit 598a0717a816 ("mm/slab: validate cache membership under > >> freelist hardening"). > >> > >> These checks and warnings can be useful especially for the debugging, which can > >> be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to > >> WARN_ONCE() so only the first hit is now reported, others are silent. This > >> patch changes it to WARN() so that all errors are reported. > >> > >> It's also useful to print SLUB allocation/free tracking info for the offending > >> object, if tracking is enabled. We could export the SLUB print_tracking() > >> function and provide an empty one for SLAB, or realize that both the debugging > >> and hardening cases in cache_from_obj() are only supported by SLUB anyway. So > >> this patch moves cache_from_obj() from slab.h to separate instances in slab.c > >> and slub.c, where the SLAB version only does the kmemcg lookup and even could > > > > Oops. I made a mistake when I applied CONFIG_SLAB_FREELIST_HARDENED > > here, I was thinking of SLAB_FREELIST_RANDOM's coverage (SLUB and SLAB), > > and I see now that I never updated CONFIG_SLAB_FREELIST_HARDENED to > > cover SLAB and SLOB. > > > > The point being: I still want the sanity check for the SLAB case under > > hardening. This needs to stay a common function. The whole point is > > to catch corruption from the wrong kmem_cache * being associated with > > an object, and that's agnostic of slab/slub/slob. > > > > So, I'll send a follow-up to this patch to actually do what I had > > originally intended for 598a0717a816 ("mm/slab: validate cache membership > > under freelist hardening"), which wasn't intended to be SLUB-specific. > > To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I > think it's best if we modify my patch. The patch below should be squashed into > the current version in mmots, with the commit log used for the whole result. > > This will cause conflicts while reapplying Roman's > mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which > can be fixed by > a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c > b) applying this hunk instead: > > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > struct kmem_cache *cachep; > > if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > - !memcg_kmem_enabled() && > !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > return s; > > cachep = virt_to_cache(x); > - if (WARN(cachep && !slab_equal_or_root(cachep, s), > + if (WARN(cachep && cachep != s, > "%s: Wrong slab cache. %s but object is from %s\n", > __func__, s->name, cachep->name)) > print_tracking(cachep, x); > > The fixup patch itself: > ----8<---- > From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 18 Jun 2020 11:52:03 +0200 > Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of > cache_from_obj() > > The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: > always get the cache from its page in kmem_cache_free()") to support > kmemcg, where per-memcg cache can be different from the root one, so we > can't use the kmem_cache pointer given to kmem_cache_free(). > > Prior to that commit, SLUB already had debugging check+warning that could > be enabled to compare the given kmem_cache pointer to one referenced by > the slab page where the object-to-be-freed resides. This check was moved > to cache_from_obj(). Later the check was also enabled for > SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate > cache membership under freelist hardening"). > > These checks and warnings can be useful especially for the debugging, > which can be improved. Commit 598a0717a816 changed the pr_err() with > WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported, > others are silent. This patch changes it to WARN() so that all errors are > reported. > > It's also useful to print SLUB allocation/free tracking info for the offending > object, if tracking is enabled. Thus, export the SLUB print_tracking() function > and provide an empty one for SLAB. > > For SLUB we can also benefit from the static key check in > kmem_cache_debug_flags(), but we need to move this function to slab.h and > declare the static key there. > > [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Acked-by: Roman Gushchin <guro@fb.com> Thanks! > --- > mm/slab.c | 8 -------- > mm/slab.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ > mm/slub.c | 38 +------------------------------------- > 3 files changed, 46 insertions(+), 45 deletions(-) > > diff --git a/mm/slab.c b/mm/slab.c > index 6134c4c36d4c..9350062ffc1a 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3672,14 +3672,6 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller) > } > EXPORT_SYMBOL(__kmalloc_track_caller); > > -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > -{ > - if (memcg_kmem_enabled()) > - return virt_to_cache(x); > - else > - return s; > -} > - > /** > * kmem_cache_free - Deallocate an object > * @cachep: The cache the allocation was from. > diff --git a/mm/slab.h b/mm/slab.h > index a2696d306b62..a9f5ba9ce9a7 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -275,6 +275,34 @@ static inline int cache_vmstat_idx(struct kmem_cache *s) > NR_SLAB_RECLAIMABLE : NR_SLAB_UNRECLAIMABLE; > } > > +#ifdef CONFIG_SLUB_DEBUG > +#ifdef CONFIG_SLUB_DEBUG_ON > +DECLARE_STATIC_KEY_TRUE(slub_debug_enabled); > +#else > +DECLARE_STATIC_KEY_FALSE(slub_debug_enabled); > +#endif > +extern void print_tracking(struct kmem_cache *s, void *object); > +#else > +static inline void print_tracking(struct kmem_cache *s, void *object) > +{ > +} > +#endif > + > +/* > + * Returns true if any of the specified slub_debug flags is enabled for the > + * cache. Use only for flags parsed by setup_slub_debug() as it also enables > + * the static key. > + */ > +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > +{ > + VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); > +#ifdef CONFIG_SLUB_DEBUG > + if (static_branch_unlikely(&slub_debug_enabled)) > + return s->flags & flags; > +#endif > + return false; > +} > + > #ifdef CONFIG_MEMCG_KMEM > > /* List of all root caches. */ > @@ -503,6 +531,23 @@ static __always_inline void uncharge_slab_page(struct page *page, int order, > memcg_uncharge_slab(page, order, s); > } > > +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > +{ > + struct kmem_cache *cachep; > + > + if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > + !memcg_kmem_enabled() && > + !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > + return s; > + > + cachep = virt_to_cache(x); > + if (WARN(cachep && !slab_equal_or_root(cachep, s), > + "%s: Wrong slab cache. %s but object is from %s\n", > + __func__, s->name, cachep->name)) > + print_tracking(cachep, x); > + return cachep; > +} > + > static inline size_t slab_ksize(const struct kmem_cache *s) > { > #ifndef CONFIG_SLUB > diff --git a/mm/slub.c b/mm/slub.c > index 202fb423d195..0e635a8aa340 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -122,21 +122,6 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); > #endif > #endif > > -/* > - * Returns true if any of the specified slub_debug flags is enabled for the > - * cache. Use only for flags parsed by setup_slub_debug() as it also enables > - * the static key. > - */ > -static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > -{ > - VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); > -#ifdef CONFIG_SLUB_DEBUG > - if (static_branch_unlikely(&slub_debug_enabled)) > - return s->flags & flags; > -#endif > - return false; > -} > - > static inline bool kmem_cache_debug(struct kmem_cache *s) > { > return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS); > @@ -653,7 +638,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time) > #endif > } > > -static void print_tracking(struct kmem_cache *s, void *object) > +void print_tracking(struct kmem_cache *s, void *object) > { > unsigned long pr_time = jiffies; > if (!(s->flags & SLAB_STORE_USER)) > @@ -1525,10 +1510,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, > { > return false; > } > - > -static void print_tracking(struct kmem_cache *s, void *object) > -{ > -} > #endif /* CONFIG_SLUB_DEBUG */ > > /* > @@ -3180,23 +3161,6 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr) > } > #endif > > -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > -{ > - struct kmem_cache *cachep; > - > - if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > - !memcg_kmem_enabled() && > - !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > - return s; > - > - cachep = virt_to_cache(x); > - if (WARN(cachep && !slab_equal_or_root(cachep, s), > - "%s: Wrong slab cache. %s but object is from %s\n", > - __func__, s->name, cachep->name)) > - print_tracking(cachep, x); > - return cachep; > -} > - > void kmem_cache_free(struct kmem_cache *s, void *x) > { > s = cache_from_obj(s, x); > -- > 2.27.0 > >
On Thu, Jun 18, 2020 at 01:05:53PM -0700, Roman Gushchin wrote: > On Thu, Jun 18, 2020 at 12:10:38PM +0200, Vlastimil Babka wrote: > > To prvent the churn of your patch moving the cache_from_obj() back to slab.h, I > > think it's best if we modify my patch. The patch below should be squashed into > > the current version in mmots, with the commit log used for the whole result. > > > > This will cause conflicts while reapplying Roman's > > mm-memcg-slab-use-a-single-set-of-kmem_caches-for-all-allocations.patch which > > can be fixed by > > a) throwing away the conflicting hunks for cache_from_obj() in slab.c and slub.c > > b) applying this hunk instead: > > > > --- a/mm/slab.h > > +++ b/mm/slab.h > > @@ -455,12 +455,11 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > > struct kmem_cache *cachep; > > > > if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > > - !memcg_kmem_enabled() && > > !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > > return s; > > > > cachep = virt_to_cache(x); > > - if (WARN(cachep && !slab_equal_or_root(cachep, s), > > + if (WARN(cachep && cachep != s, > > "%s: Wrong slab cache. %s but object is from %s\n", > > __func__, s->name, cachep->name)) > > print_tracking(cachep, x); > > > > The fixup patch itself: > > ----8<---- > > From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001 > > From: Vlastimil Babka <vbabka@suse.cz> > > Date: Thu, 18 Jun 2020 11:52:03 +0200 > > Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of > > cache_from_obj() Andrew, do you need this separately, or can you extract this fixup from this thread? -Kees > > > > The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: > > always get the cache from its page in kmem_cache_free()") to support > > kmemcg, where per-memcg cache can be different from the root one, so we > > can't use the kmem_cache pointer given to kmem_cache_free(). > > > > Prior to that commit, SLUB already had debugging check+warning that could > > be enabled to compare the given kmem_cache pointer to one referenced by > > the slab page where the object-to-be-freed resides. This check was moved > > to cache_from_obj(). Later the check was also enabled for > > SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate > > cache membership under freelist hardening"). > > > > These checks and warnings can be useful especially for the debugging, > > which can be improved. Commit 598a0717a816 changed the pr_err() with > > WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported, > > others are silent. This patch changes it to WARN() so that all errors are > > reported. > > > > It's also useful to print SLUB allocation/free tracking info for the offending > > object, if tracking is enabled. Thus, export the SLUB print_tracking() function > > and provide an empty one for SLAB. > > > > For SLUB we can also benefit from the static key check in > > kmem_cache_debug_flags(), but we need to move this function to slab.h and > > declare the static key there. > > > > [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > Acked-by: Roman Gushchin <guro@fb.com> > > Thanks! > > > --- > > mm/slab.c | 8 -------- > > mm/slab.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ > > mm/slub.c | 38 +------------------------------------- > > 3 files changed, 46 insertions(+), 45 deletions(-) > > > > diff --git a/mm/slab.c b/mm/slab.c > > index 6134c4c36d4c..9350062ffc1a 100644 > > --- a/mm/slab.c > > +++ b/mm/slab.c > > @@ -3672,14 +3672,6 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller) > > } > > EXPORT_SYMBOL(__kmalloc_track_caller); > > > > -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > > -{ > > - if (memcg_kmem_enabled()) > > - return virt_to_cache(x); > > - else > > - return s; > > -} > > - > > /** > > * kmem_cache_free - Deallocate an object > > * @cachep: The cache the allocation was from. > > diff --git a/mm/slab.h b/mm/slab.h > > index a2696d306b62..a9f5ba9ce9a7 100644 > > --- a/mm/slab.h > > +++ b/mm/slab.h > > @@ -275,6 +275,34 @@ static inline int cache_vmstat_idx(struct kmem_cache *s) > > NR_SLAB_RECLAIMABLE : NR_SLAB_UNRECLAIMABLE; > > } > > > > +#ifdef CONFIG_SLUB_DEBUG > > +#ifdef CONFIG_SLUB_DEBUG_ON > > +DECLARE_STATIC_KEY_TRUE(slub_debug_enabled); > > +#else > > +DECLARE_STATIC_KEY_FALSE(slub_debug_enabled); > > +#endif > > +extern void print_tracking(struct kmem_cache *s, void *object); > > +#else > > +static inline void print_tracking(struct kmem_cache *s, void *object) > > +{ > > +} > > +#endif > > + > > +/* > > + * Returns true if any of the specified slub_debug flags is enabled for the > > + * cache. Use only for flags parsed by setup_slub_debug() as it also enables > > + * the static key. > > + */ > > +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > > +{ > > + VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); > > +#ifdef CONFIG_SLUB_DEBUG > > + if (static_branch_unlikely(&slub_debug_enabled)) > > + return s->flags & flags; > > +#endif > > + return false; > > +} > > + > > #ifdef CONFIG_MEMCG_KMEM > > > > /* List of all root caches. */ > > @@ -503,6 +531,23 @@ static __always_inline void uncharge_slab_page(struct page *page, int order, > > memcg_uncharge_slab(page, order, s); > > } > > > > +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > > +{ > > + struct kmem_cache *cachep; > > + > > + if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > > + !memcg_kmem_enabled() && > > + !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > > + return s; > > + > > + cachep = virt_to_cache(x); > > + if (WARN(cachep && !slab_equal_or_root(cachep, s), > > + "%s: Wrong slab cache. %s but object is from %s\n", > > + __func__, s->name, cachep->name)) > > + print_tracking(cachep, x); > > + return cachep; > > +} > > + > > static inline size_t slab_ksize(const struct kmem_cache *s) > > { > > #ifndef CONFIG_SLUB > > diff --git a/mm/slub.c b/mm/slub.c > > index 202fb423d195..0e635a8aa340 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -122,21 +122,6 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); > > #endif > > #endif > > > > -/* > > - * Returns true if any of the specified slub_debug flags is enabled for the > > - * cache. Use only for flags parsed by setup_slub_debug() as it also enables > > - * the static key. > > - */ > > -static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > > -{ > > - VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); > > -#ifdef CONFIG_SLUB_DEBUG > > - if (static_branch_unlikely(&slub_debug_enabled)) > > - return s->flags & flags; > > -#endif > > - return false; > > -} > > - > > static inline bool kmem_cache_debug(struct kmem_cache *s) > > { > > return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS); > > @@ -653,7 +638,7 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time) > > #endif > > } > > > > -static void print_tracking(struct kmem_cache *s, void *object) > > +void print_tracking(struct kmem_cache *s, void *object) > > { > > unsigned long pr_time = jiffies; > > if (!(s->flags & SLAB_STORE_USER)) > > @@ -1525,10 +1510,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, > > { > > return false; > > } > > - > > -static void print_tracking(struct kmem_cache *s, void *object) > > -{ > > -} > > #endif /* CONFIG_SLUB_DEBUG */ > > > > /* > > @@ -3180,23 +3161,6 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr) > > } > > #endif > > > > -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > > -{ > > - struct kmem_cache *cachep; > > - > > - if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && > > - !memcg_kmem_enabled() && > > - !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) > > - return s; > > - > > - cachep = virt_to_cache(x); > > - if (WARN(cachep && !slab_equal_or_root(cachep, s), > > - "%s: Wrong slab cache. %s but object is from %s\n", > > - __func__, s->name, cachep->name)) > > - print_tracking(cachep, x); > > - return cachep; > > -} > > - > > void kmem_cache_free(struct kmem_cache *s, void *x) > > { > > s = cache_from_obj(s, x); > > -- > > 2.27.0 > > > >
On 6/18/20 12:10 PM, Vlastimil Babka wrote: > ----8<---- > From b8df607d92b37e5329ce7bda62b2b364cc249893 Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 18 Jun 2020 11:52:03 +0200 > Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of > cache_from_obj() > Another small fixup, please. ----8<---- From 2cd24408828a22a3cd4301afbaf98767b1a14eb1 Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Wed, 24 Jun 2020 09:49:15 +0200 Subject: [PATCH] mm, slab/slub: improve error reporting and overhead of cache_from_obj()-fix The added VM_WARN_ON_ONCE triggers [1] with CONFIG_SLAB, as SLAB_DEBUG_FLAGS doesn't include SLAB_CONSISTENCY_CHECKS. Move the check under #ifdef SLUB_DEBUG. [1] https://lore.kernel.org/r/20200623090213.GW5535@shao2-debian Reported-by: kernel test robot <rong.a.chen@intel.com> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slab.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/slab.h b/mm/slab.h index 525260217013..e829d9f5e6ef 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -229,8 +229,8 @@ static inline void print_tracking(struct kmem_cache *s, void *object) */ static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) { - VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); #ifdef CONFIG_SLUB_DEBUG + VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); if (static_branch_unlikely(&slub_debug_enabled)) return s->flags & flags; #endif
diff --git a/mm/slab.c b/mm/slab.c index 9350062ffc1a..6134c4c36d4c 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3672,6 +3672,14 @@ void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller) } EXPORT_SYMBOL(__kmalloc_track_caller); +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) +{ + if (memcg_kmem_enabled()) + return virt_to_cache(x); + else + return s; +} + /** * kmem_cache_free - Deallocate an object * @cachep: The cache the allocation was from. diff --git a/mm/slab.h b/mm/slab.h index 815e4e9a94cd..c0c4244f75da 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -503,29 +503,6 @@ static __always_inline void uncharge_slab_page(struct page *page, int order, memcg_uncharge_slab(page, order, s); } -static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) -{ - struct kmem_cache *cachep; - - /* - * When kmemcg is not being used, both assignments should return the - * same value. but we don't want to pay the assignment price in that - * case. If it is not compiled in, the compiler should be smart enough - * to not do even the assignment. In that case, slab_equal_or_root - * will also be a constant. - */ - if (!memcg_kmem_enabled() && - !IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && - !unlikely(s->flags & SLAB_CONSISTENCY_CHECKS)) - return s; - - cachep = virt_to_cache(x); - WARN_ONCE(cachep && !slab_equal_or_root(cachep, s), - "%s: Wrong slab cache. %s but object is from %s\n", - __func__, s->name, cachep->name); - return cachep; -} - static inline size_t slab_ksize(const struct kmem_cache *s) { #ifndef CONFIG_SLUB diff --git a/mm/slub.c b/mm/slub.c index efb08f2e9c66..f7a1d8537674 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1524,6 +1524,10 @@ static bool freelist_corrupted(struct kmem_cache *s, struct page *page, { return false; } + +static void print_tracking(struct kmem_cache *s, void *object) +{ +} #endif /* CONFIG_SLUB_DEBUG */ /* @@ -3175,6 +3179,23 @@ void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr) } #endif +static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) +{ + struct kmem_cache *cachep; + + if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) && + !memcg_kmem_enabled() && + !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) + return s; + + cachep = virt_to_cache(x); + if (WARN(cachep && !slab_equal_or_root(cachep, s), + "%s: Wrong slab cache. %s but object is from %s\n", + __func__, s->name, cachep->name)) + print_tracking(cachep, x); + return cachep; +} + void kmem_cache_free(struct kmem_cache *s, void *x) { s = cache_from_obj(s, x);
The function cache_from_obj() was added by commit b9ce5ef49f00 ("sl[au]b: always get the cache from its page in kmem_cache_free()") to support kmemcg, where per-memcg cache can be different from the root one, so we can't use the kmem_cache pointer given to kmem_cache_free(). Prior to that commit, SLUB already had debugging check+warning that could be enabled to compare the given kmem_cache pointer to one referenced by the slab page where the object-to-be-freed resides. This check was moved to cache_from_obj(). Later the check was also enabled for SLAB_FREELIST_HARDENED configs by commit 598a0717a816 ("mm/slab: validate cache membership under freelist hardening"). These checks and warnings can be useful especially for the debugging, which can be improved. Commit 598a0717a816 changed the pr_err() with WARN_ON_ONCE() to WARN_ONCE() so only the first hit is now reported, others are silent. This patch changes it to WARN() so that all errors are reported. It's also useful to print SLUB allocation/free tracking info for the offending object, if tracking is enabled. We could export the SLUB print_tracking() function and provide an empty one for SLAB, or realize that both the debugging and hardening cases in cache_from_obj() are only supported by SLUB anyway. So this patch moves cache_from_obj() from slab.h to separate instances in slab.c and slub.c, where the SLAB version only does the kmemcg lookup and even could be completely removed once the kmemcg rework [1] is merged. The SLUB version can thus easily use the print_tracking() function. It can also use the kmem_cache_debug_flags() static key check for improved performance in kernels without the hardening and with debugging not enabled on boot. [1] https://lore.kernel.org/r/20200608230654.828134-18-guro@fb.com Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slab.c | 8 ++++++++ mm/slab.h | 23 ----------------------- mm/slub.c | 21 +++++++++++++++++++++ 3 files changed, 29 insertions(+), 23 deletions(-)