| 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