linux-kernel - Re: [PATCH v3 7/7] mm/kmemleak: fix partially freeing unknown object warning

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] [day] [month] [year] [list]

Message-ID: <ZTAKJstsXrcUtOjn@arm.com>
Date:   Wed, 18 Oct 2023 17:39:02 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     Liu Shixin <liushixin2@...wei.com>
Cc:     Patrick Wang <patrick.wang.shcn@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kefeng Wang <wangkefeng.wang@...wei.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 7/7] mm/kmemleak: fix partially freeing unknown object
 warning

On Wed, Oct 18, 2023 at 06:29:52PM +0800, Liu Shixin wrote:
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 7c9125c18956..a956b2734324 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -816,16 +816,25 @@ static void delete_object_full(unsigned long ptr)
>   */
>  static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
>  {
> -	struct kmemleak_object *object;
> -	unsigned long start, end;
> +	struct kmemleak_object *object, *object_l, *object_r;
> +	unsigned long start, end, flags;
> +
> +	object_l = __alloc_object(GFP_KERNEL);
> +	if (!object_l)
> +		return;
>  
> -	object = find_and_remove_object(ptr, 1, is_phys);
> +	object_r = __alloc_object(GFP_KERNEL);
> +	if (!object_r)
> +		goto out;
> +
> +	raw_spin_lock_irqsave(&kmemleak_lock, flags);
> +	object = __find_and_remove_object(ptr, 1, is_phys);
>  	if (!object) {
>  #ifdef DEBUG
>  		kmemleak_warn("Partially freeing unknown object at 0x%08lx (size %zu)\n",
>  			      ptr, size);
>  #endif
> -		return;
> +		goto unlock;
>  	}
>  
>  	/*
> @@ -835,14 +844,25 @@ static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
>  	 */
>  	start = object->pointer;
>  	end = object->pointer + object->size;
> -	if (ptr > start)
> -		__create_object(start, ptr - start, object->min_count,
> -			      GFP_KERNEL, is_phys);
> -	if (ptr + size < end)
> -		__create_object(ptr + size, end - ptr - size, object->min_count,
> -			      GFP_KERNEL, is_phys);
> +	if ((ptr > start) &&
> +	    !__link_object(object_l, start, ptr - start,
> +			   object->min_count, is_phys))
> +		object_l = NULL;
> +	if ((ptr + size < end) &&
> +	    !__link_object(object_r, ptr + size, end - ptr - size,
> +			   object->min_count, is_phys))
> +		object_r = NULL;
> +
> +unlock:
> +	raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
> +	if (object)
> +		__delete_object(object);
>  
> -	__delete_object(object);
> +out:
> +	if (object_l)
> +		mem_pool_free(object_l);
> +	if (object_r)
> +		mem_pool_free(object_r);
>  }

Ah, I see now why __link_object() needs to do further object
initialisation under the lock as prior to find_and_remove_object() we
don't have all the information needed for object_l and object_r.

Maybe in the __create_object splitting patch you can leave
__alloc_object() just do the actual allocation and __link_object() do
the full initialisation. Up to you, you can send a separate patch on top
as I can see Andrew picked them up already.

Otherwise the series looks fine.

Reviewed-by: Catalin Marinas <catalin.marinas@....com>