Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650605 > unrolled thread

[PATCH v2 0/3] mm: kmemleak: Improve vmalloc() false positives for thread stack allocation

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] mm: kmemleak: Improve vmalloc() false positives for thread stack allocation Catalin Marinas <catalin.marinas@arm.com> - 2017-05-25 17:50 +0200
    [PATCH v2 2/3] mm: kmemleak: Factor object reference updating out of scan_block() Catalin Marinas <catalin.marinas@arm.com> - 2017-05-25 17:50 +0200
      Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating  out of scan_block() Luis Henriques <lhenriques@suse.com> - 2017-05-26 18:10 +0200
        Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating  out of scan_block() Catalin Marinas <catalin.marinas@arm.com> - 2017-05-26 18:30 +0200
          Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating  out of scan_block() Catalin Marinas <catalin.marinas@arm.com> - 2017-05-26 18:30 +0200
            Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating  out of scan_block() Luis Henriques <lhenriques@suse.com> - 2017-05-26 19:20 +0200

#1650605 — [PATCH v2 0/3] mm: kmemleak: Improve vmalloc() false positives for thread stack allocation

Hi,

This is a follow up from [1] (mm: kmemleak: Treat vm_struct as
alternative reference to vmalloc'ed objects).

The first two patches are just clean-up and refactoring. The third
introduces the kmemleak_vmalloc() API which allows a vmalloc() caller to
keep either the returned pointer or a pointer to vm_struct as a
reference (see the patch description for the implementation details).
The false positives were noticed with alloc_thread_stack_node(),
free_thread_stack() and CONFIG_VMAP_STACK where a per-CPU array is used
to cache the freed thread stacks as vm_struct pointers.

Changes since v1:

- Split the patch into three for easier review
- Only call update_refs() if !color_gray() on the found object, it
  avoids an unnecessary function call

[1] http://lkml.kernel.org/r/1495474514-24425-1-git-send-email-catalin.marinas@arm.com

Catalin Marinas (3):
  mm: kmemleak: Slightly reduce the size of some structures on 64-bit
    architectures
  mm: kmemleak: Factor object reference updating out of scan_block()
  mm: kmemleak: Treat vm_struct as alternative reference to vmalloc'ed
    objects

 Documentation/dev-tools/kmemleak.rst |   1 +
 include/linux/kmemleak.h             |   7 ++
 mm/kmemleak.c                        | 136 +++++++++++++++++++++++++++++------
 mm/vmalloc.c                         |   7 +-
 4 files changed, 123 insertions(+), 28 deletions(-)

[toc] | [next] | [standalone]

#1650606 — [PATCH v2 2/3] mm: kmemleak: Factor object reference updating out of scan_block()

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@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.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);
 	}
 	read_unlock_irqrestore(&kmemleak_lock, flags);

[toc] | [prev] | [next] | [standalone]

#1651455 — 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@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.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);
> 

[toc] | [prev] | [next] | [standalone]

#1651511 — Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating out of scan_block()

On Fri, May 26, 2017 at 05:09:17PM +0100, Luis Henriques wrote:
> 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@kernel.org>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.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.

Thanks for re-testing (I dropped your tested-by from the initial patch
since I made a small modification).

> 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 :)

There is a small difference: for the first object it needs to check
color_gray() and access object->excess_ref while the lock is held. It
doesn't need this in the second case. I could've written it in different
ways but probably with a similar number of lines; I just found this
clearer.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1651512 — Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating out of scan_block()

On Fri, May 26, 2017 at 05:21:08PM +0100, Catalin Marinas wrote:
> On Fri, May 26, 2017 at 05:09:17PM +0100, Luis Henriques wrote:
> > 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@kernel.org>
> > > Cc: Andy Lutomirski <luto@amacapital.net>
> > > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
[...]
> > FWIW, I've tested this patchset and I don't see kmemleak triggering the
> > false positives anymore.
> 
> Thanks for re-testing (I dropped your tested-by from the initial patch
> since I made a small modification).

Sorry, the "re-testing" comment was meant at the other Luis on cc ;)
(Luis R. Rodriguez). It's been a long day...

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1651532 — Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating out of scan_block()

On Fri, May 26, 2017 at 05:23:30PM +0100, Catalin Marinas wrote:
> On Fri, May 26, 2017 at 05:21:08PM +0100, Catalin Marinas wrote:
> > On Fri, May 26, 2017 at 05:09:17PM +0100, Luis Henriques wrote:
> > > 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@kernel.org>
> > > > Cc: Andy Lutomirski <luto@amacapital.net>
> > > > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> [...]
> > > FWIW, I've tested this patchset and I don't see kmemleak triggering the
> > > false positives anymore.
> > 
> > Thanks for re-testing (I dropped your tested-by from the initial patch
> > since I made a small modification).
> 
> Sorry, the "re-testing" comment was meant at the other Luis on cc ;)
> (Luis R. Rodriguez). It's been a long day...

Heh, no worries!  What are the odds of having 2 different guys named Luis
testing the same patch? :-)

Cheers,
--
Luís

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web