lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y4M5bWgjSWKZEXnO@hyeyoo> Date: Sun, 27 Nov 2022 19:18:21 +0900 From: Hyeonggon Yoo <42.hyeyoo@...il.com> To: Vlastimil Babka <vbabka@...e.cz> Cc: Christoph Lameter <cl@...ux.com>, David Rientjes <rientjes@...gle.com>, Joonsoo Kim <iamjoonsoo.kim@....com>, Pekka Enberg <penberg@...nel.org>, Roman Gushchin <roman.gushchin@...ux.dev>, Andrew Morton <akpm@...ux-foundation.org>, Linus Torvalds <torvalds@...ux-foundation.org>, Matthew Wilcox <willy@...radead.org>, patches@...ts.linux.dev, linux-mm@...ck.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 08/12] mm, slub: refactor free debug processing On Mon, Nov 21, 2022 at 06:11:58PM +0100, Vlastimil Babka wrote: > Since commit c7323a5ad078 ("mm/slub: restrict sysfs validation to debug > caches and make it safe"), caches with debugging enabled use the > free_debug_processing() function to do both freeing checks and actual > freeing to partial list under list_lock, bypassing the fast paths. > > We will want to use the same path for CONFIG_SLUB_TINY, but without the > debugging checks, so refactor the code so that free_debug_processing() > does only the checks, while the freeing is handled by a new function > free_to_partial_list(). > > For consistency, change return parameter alloc_debug_processing() from > int to bool and correct the !SLUB_DEBUG variant to return true and not > false. This didn't matter until now, but will in the following changes. > > Signed-off-by: Vlastimil Babka <vbabka@...e.cz> > --- > mm/slub.c | 154 +++++++++++++++++++++++++++++------------------------- > 1 file changed, 83 insertions(+), 71 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index bf726dd00f7d..fd56d7cca9c2 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -1368,7 +1368,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, > return 1; > } > > -static noinline int alloc_debug_processing(struct kmem_cache *s, > +static noinline bool alloc_debug_processing(struct kmem_cache *s, > struct slab *slab, void *object, int orig_size) > { > if (s->flags & SLAB_CONSISTENCY_CHECKS) { > @@ -1380,7 +1380,7 @@ static noinline int alloc_debug_processing(struct kmem_cache *s, > trace(s, slab, object, 1); > set_orig_size(s, object, orig_size); > init_object(s, object, SLUB_RED_ACTIVE); > - return 1; > + return true; > > bad: > if (folio_test_slab(slab_folio(slab))) { > @@ -1393,7 +1393,7 @@ static noinline int alloc_debug_processing(struct kmem_cache *s, > slab->inuse = slab->objects; > slab->freelist = NULL; > } > - return 0; > + return false; > } > > static inline int free_consistency_checks(struct kmem_cache *s, > @@ -1646,17 +1646,17 @@ static inline void setup_object_debug(struct kmem_cache *s, void *object) {} > static inline > void setup_slab_debug(struct kmem_cache *s, struct slab *slab, void *addr) {} > > -static inline int alloc_debug_processing(struct kmem_cache *s, > - struct slab *slab, void *object, int orig_size) { return 0; } > +static inline bool alloc_debug_processing(struct kmem_cache *s, > + struct slab *slab, void *object, int orig_size) { return true; } > > -static inline void free_debug_processing( > - struct kmem_cache *s, struct slab *slab, > - void *head, void *tail, int bulk_cnt, > - unsigned long addr) {} > +static inline bool free_debug_processing(struct kmem_cache *s, > + struct slab *slab, void *head, void *tail, int *bulk_cnt, > + unsigned long addr, depot_stack_handle_t handle) { return true; } > > static inline void slab_pad_check(struct kmem_cache *s, struct slab *slab) {} > static inline int check_object(struct kmem_cache *s, struct slab *slab, > void *object, u8 val) { return 1; } > +static inline depot_stack_handle_t set_track_prepare(void) { return 0; } > static inline void set_track(struct kmem_cache *s, void *object, > enum track_item alloc, unsigned long addr) {} > static inline void add_full(struct kmem_cache *s, struct kmem_cache_node *n, > @@ -2833,38 +2833,28 @@ static inline unsigned long node_nr_objs(struct kmem_cache_node *n) > } > > /* Supports checking bulk free of a constructed freelist */ > -static noinline void free_debug_processing( > - struct kmem_cache *s, struct slab *slab, > - void *head, void *tail, int bulk_cnt, > - unsigned long addr) > +static inline bool free_debug_processing(struct kmem_cache *s, > + struct slab *slab, void *head, void *tail, int *bulk_cnt, > + unsigned long addr, depot_stack_handle_t handle) > { > - struct kmem_cache_node *n = get_node(s, slab_nid(slab)); > - struct slab *slab_free = NULL; > + bool checks_ok = false; > void *object = head; > int cnt = 0; > - unsigned long flags; > - bool checks_ok = false; > - depot_stack_handle_t handle = 0; > - > - if (s->flags & SLAB_STORE_USER) > - handle = set_track_prepare(); > - > - spin_lock_irqsave(&n->list_lock, flags); > > if (s->flags & SLAB_CONSISTENCY_CHECKS) { > if (!check_slab(s, slab)) > goto out; > } > > - if (slab->inuse < bulk_cnt) { > + if (slab->inuse < *bulk_cnt) { > slab_err(s, slab, "Slab has %d allocated objects but %d are to be freed\n", > - slab->inuse, bulk_cnt); > + slab->inuse, *bulk_cnt); > goto out; > } > > next_object: > > - if (++cnt > bulk_cnt) > + if (++cnt > *bulk_cnt) > goto out_cnt; > > if (s->flags & SLAB_CONSISTENCY_CHECKS) { > @@ -2886,57 +2876,18 @@ static noinline void free_debug_processing( > checks_ok = true; > > out_cnt: > - if (cnt != bulk_cnt) > + if (cnt != *bulk_cnt) { > slab_err(s, slab, "Bulk free expected %d objects but found %d\n", > - bulk_cnt, cnt); > - > -out: > - if (checks_ok) { > - void *prior = slab->freelist; > - > - /* Perform the actual freeing while we still hold the locks */ > - slab->inuse -= cnt; > - set_freepointer(s, tail, prior); > - slab->freelist = head; > - > - /* > - * If the slab is empty, and node's partial list is full, > - * it should be discarded anyway no matter it's on full or > - * partial list. > - */ > - if (slab->inuse == 0 && n->nr_partial >= s->min_partial) > - slab_free = slab; > - > - if (!prior) { > - /* was on full list */ > - remove_full(s, n, slab); > - if (!slab_free) { > - add_partial(n, slab, DEACTIVATE_TO_TAIL); > - stat(s, FREE_ADD_PARTIAL); > - } > - } else if (slab_free) { > - remove_partial(n, slab); > - stat(s, FREE_REMOVE_PARTIAL); > - } > + *bulk_cnt, cnt); > + *bulk_cnt = cnt; > } > > - if (slab_free) { > - /* > - * Update the counters while still holding n->list_lock to > - * prevent spurious validation warnings > - */ > - dec_slabs_node(s, slab_nid(slab_free), slab_free->objects); > - } > - > - spin_unlock_irqrestore(&n->list_lock, flags); > +out: > > if (!checks_ok) > slab_fix(s, "Object at 0x%p not freed", object); > > - if (slab_free) { > - stat(s, FREE_SLAB); > - free_slab(s, slab_free); > - } > + return checks_ok; > } > #endif /* CONFIG_SLUB_DEBUG */ > > @@ -3453,6 +3404,67 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node) > } > EXPORT_SYMBOL(kmem_cache_alloc_node); > > +static noinline void free_to_partial_list( > + struct kmem_cache *s, struct slab *slab, > + void *head, void *tail, int bulk_cnt, > + unsigned long addr) > +{ > + struct kmem_cache_node *n = get_node(s, slab_nid(slab)); > + struct slab *slab_free = NULL; > + int cnt = bulk_cnt; > + unsigned long flags; > + depot_stack_handle_t handle = 0; > + > + if (s->flags & SLAB_STORE_USER) > + handle = set_track_prepare(); > + > + spin_lock_irqsave(&n->list_lock, flags); > + > + if (free_debug_processing(s, slab, head, tail, &cnt, addr, handle)) { > + void *prior = slab->freelist; > + > + /* Perform the actual freeing while we still hold the locks */ > + slab->inuse -= cnt; > + set_freepointer(s, tail, prior); > + slab->freelist = head; > + > + /* > + * If the slab is empty, and node's partial list is full, > + * it should be discarded anyway no matter it's on full or > + * partial list. > + */ > + if (slab->inuse == 0 && n->nr_partial >= s->min_partial) > + slab_free = slab; > + > + if (!prior) { > + /* was on full list */ > + remove_full(s, n, slab); > + if (!slab_free) { > + add_partial(n, slab, DEACTIVATE_TO_TAIL); > + stat(s, FREE_ADD_PARTIAL); > + } > + } else if (slab_free) { > + remove_partial(n, slab); > + stat(s, FREE_REMOVE_PARTIAL); > + } > + } > + > + if (slab_free) { > + /* > + * Update the counters while still holding n->list_lock to > + * prevent spurious validation warnings > + */ > + dec_slabs_node(s, slab_nid(slab_free), slab_free->objects); > + } > + > + spin_unlock_irqrestore(&n->list_lock, flags); > + > + if (slab_free) { > + stat(s, FREE_SLAB); > + free_slab(s, slab_free); > + } > +} > + > /* > * Slow path handling. This may still be called frequently since objects > * have a longer lifetime than the cpu slabs in most processing loads. > @@ -3479,7 +3491,7 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab, > return; > > if (kmem_cache_debug(s)) { > - free_debug_processing(s, slab, head, tail, cnt, addr); > + free_to_partial_list(s, slab, head, tail, cnt, addr); > return; > } > > -- > 2.38.1 > Looks good to me. Reviewed-by: Hyeonggon Yoo <42.hyeyoo@...il.com> -- Thanks, Hyeonggon
Powered by blists - more mailing lists