linux-kernel - Re: [PATCH v2 1/3] mm/slub: fix the race between validate_slab and slab

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ys6cymrtnHNlCDG9@ip-172-31-24-42.ap-northeast-1.compute.internal>
Date:   Wed, 13 Jul 2022 10:22:02 +0000
From:   Hyeonggon Yoo <42.hyeyoo@...il.com>
To:     Rongwei Wang <rongwei.wang@...ux.alibaba.com>
Cc:     akpm@...ux-foundation.org, vbabka@...e.cz,
        roman.gushchin@...ux.dev, iamjoonsoo.kim@....com,
        rientjes@...gle.com, penberg@...nel.org, cl@...two.de,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Feng Tang <feng.tang@...el.com>
Subject: Re: [PATCH v2 1/3] mm/slub: fix the race between validate_slab and
 slab_free

On Tue, Jul 12, 2022 at 10:28:05AM +0800, Rongwei Wang wrote:
> In use cases where allocating and freeing slab frequently, some
> error messages, such as "Left Redzone overwritten", "First byte
> 0xbb instead of 0xcc" would be printed when validating slabs.
> That's because an object has been filled with SLAB_RED_INACTIVE,
> but has not been added to slab's freelist. And between these
> two states, the behaviour of validating slab is likely to occur.
> 
> Actually, it doesn't mean the slab can not work stably. But, these
> confusing messages will disturb slab debugging more or less.
> 
> Signed-off-by: Rongwei Wang <rongwei.wang@...ux.alibaba.com>
> ---
>  mm/slub.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
>

This makes the code more complex.

A part of me says it may be more pleasant to split implementation
allocating from caches for debugging. That would make it simpler.

something like:

__slab_alloc() {
	if (kmem_cache_debug(s))
		slab_alloc_debug()
	else
		___slab_alloc()
}

slab_free() {
	if (kmem_cache_debug(s))
		slab_free_debug()
	else
		__do_slab_free()
}

See also:
	https://lore.kernel.org/lkml/faf416b9-f46c-8534-7fb7-557c046a564d@suse.cz/

> diff --git a/mm/slub.c b/mm/slub.c
> index b1281b8654bd..e950d8df8380 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1391,18 +1391,16 @@ static noinline int free_debug_processing(
>  	void *head, void *tail, int bulk_cnt,
>  	unsigned long addr)
>  {
> -	struct kmem_cache_node *n = get_node(s, slab_nid(slab));
>  	void *object = head;
>  	int cnt = 0;
> -	unsigned long flags, flags2;
> +	unsigned long flags;
>  	int ret = 0;
>  	depot_stack_handle_t handle = 0;
>  
>  	if (s->flags & SLAB_STORE_USER)
>  		handle = set_track_prepare();
>  
> -	spin_lock_irqsave(&n->list_lock, flags);
> -	slab_lock(slab, &flags2);
> +	slab_lock(slab, &flags);
>  
>  	if (s->flags & SLAB_CONSISTENCY_CHECKS) {
>  		if (!check_slab(s, slab))
> @@ -1435,8 +1433,7 @@ static noinline int free_debug_processing(
>  		slab_err(s, slab, "Bulk freelist count(%d) invalid(%d)\n",
>  			 bulk_cnt, cnt);
>  
> -	slab_unlock(slab, &flags2);
> -	spin_unlock_irqrestore(&n->list_lock, flags);
> +	slab_unlock(slab, &flags);
>  	if (!ret)
>  		slab_fix(s, "Object at 0x%p not freed", object);
>  	return ret;
> @@ -3330,7 +3327,7 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
>  
>  {
>  	void *prior;
> -	int was_frozen;
> +	int was_frozen, to_take_off = 0;
>  	struct slab new;
>  	unsigned long counters;
>  	struct kmem_cache_node *n = NULL;
> @@ -3341,14 +3338,23 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
>  	if (kfence_free(head))
>  		return;
>  
> -	if (kmem_cache_debug(s) &&
> -	    !free_debug_processing(s, slab, head, tail, cnt, addr))
> -		return;
> +	n = get_node(s, slab_nid(slab));
> +	if (kmem_cache_debug(s)) {
> +		int ret;
>  
> -	do {
> -		if (unlikely(n)) {
> +		spin_lock_irqsave(&n->list_lock, flags);
> +		ret = free_debug_processing(s, slab, head, tail, cnt, addr);
> +		if (!ret) {
>  			spin_unlock_irqrestore(&n->list_lock, flags);
> -			n = NULL;
> +			return;
> +		}
> +	}
> +
> +	do {
> +		if (unlikely(to_take_off)) {
> +			if (!kmem_cache_debug(s))
> +				spin_unlock_irqrestore(&n->list_lock, flags);
> +			to_take_off = 0;
>  		}
>  		prior = slab->freelist;
>  		counters = slab->counters;
> @@ -3369,8 +3375,6 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
>  				new.frozen = 1;
>  
>  			} else { /* Needs to be taken off a list */
> -
> -				n = get_node(s, slab_nid(slab));
>  				/*
>  				 * Speculatively acquire the list_lock.
>  				 * If the cmpxchg does not succeed then we may
> @@ -3379,8 +3383,10 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
>  				 * Otherwise the list_lock will synchronize with
>  				 * other processors updating the list of slabs.
>  				 */
> -				spin_lock_irqsave(&n->list_lock, flags);
> +				if (!kmem_cache_debug(s))
> +					spin_lock_irqsave(&n->list_lock, flags);
>  
> +				to_take_off = 1;
>  			}
>  		}
>  
> @@ -3389,8 +3395,9 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
>  		head, new.counters,
>  		"__slab_free"));
>  
> -	if (likely(!n)) {
> -
> +	if (likely(!to_take_off)) {
> +		if (kmem_cache_debug(s))
> +			spin_unlock_irqrestore(&n->list_lock, flags);
>  		if (likely(was_frozen)) {
>  			/*
>  			 * The list lock was not taken therefore no list
> -- 
> 2.27.0
>