linux-kernel - Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating out of scan

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 26 May 2017 17:09:17 +0100
From:   Luis Henriques <lhenriques@...e.com>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Michal Hocko <mhocko@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating
 out of scan_block()

On Thu, May 25, 2017 at 04:42:16PM +0100, Catalin Marinas wrote:
> The scan_block() function updates the number of references (pointers) to
> objects, adding them to the gray_list when object->min_count is reached.
> The patch factors out this functionality into a separate update_refs()
> function.
> 
> Cc: Michal Hocko <mhocko@...nel.org>
> Cc: Andy Lutomirski <luto@...capital.net>
> Cc: "Luis R. Rodriguez" <mcgrof@...nel.org>
> Signed-off-by: Catalin Marinas <catalin.marinas@....com>
> ---
>  mm/kmemleak.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 964b12eba2c1..266482f460c2 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -1188,6 +1188,30 @@ static bool update_checksum(struct kmemleak_object *object)
>  }
>  
>  /*
> + * Update an object's references. object->lock must be held by the caller.
> + */
> +static void update_refs(struct kmemleak_object *object)
> +{
> +	if (!color_white(object)) {
> +		/* non-orphan, ignored or new */
> +		return;
> +	}
> +
> +	/*
> +	 * Increase the object's reference count (number of pointers to the
> +	 * memory block). If this count reaches the required minimum, the
> +	 * object's color will become gray and it will be added to the
> +	 * gray_list.
> +	 */
> +	object->count++;
> +	if (color_gray(object)) {
> +		/* put_object() called when removing from gray_list */
> +		WARN_ON(!get_object(object));
> +		list_add_tail(&object->gray_list, &gray_list);
> +	}
> +}
> +
> +/*
>   * Memory scanning is a long process and it needs to be interruptable. This
>   * function checks whether such interrupt condition occurred.
>   */
> @@ -1259,24 +1283,7 @@ static void scan_block(void *_start, void *_end,
>  		 * enclosed by scan_mutex.
>  		 */
>  		spin_lock_nested(&object->lock, SINGLE_DEPTH_NESTING);
> -		if (!color_white(object)) {
> -			/* non-orphan, ignored or new */
> -			spin_unlock(&object->lock);
> -			continue;
> -		}
> -
> -		/*
> -		 * Increase the object's reference count (number of pointers
> -		 * to the memory block). If this count reaches the required
> -		 * minimum, the object's color will become gray and it will be
> -		 * added to the gray_list.
> -		 */
> -		object->count++;
> -		if (color_gray(object)) {
> -			/* put_object() called when removing from gray_list */
> -			WARN_ON(!get_object(object));
> -			list_add_tail(&object->gray_list, &gray_list);
> -		}
> +		update_refs(object);
>  		spin_unlock(&object->lock);

FWIW, I've tested this patchset and I don't see kmemleak triggering the
false positives anymore.

I've also done a quick review and couldn't find anything obviously
incorrect, just a question: why didn't you moved the spin_lock/unlock into
update_refs() too?  It would save you 2 lines in the next patch :)

Cheers,
--
Luís


>  	}
>  	read_unlock_irqrestore(&kmemleak_lock, flags);
>