[PATCH 0/5] kmemleak: few small cleanups and clear command support

[PATCH 0/5] kmemleak: few small cleanups and clear command support

[Luis R. Rodriguez]

Few patches after a quick review of kmemleak, adds clear command support
and fixes the sparse warnings. The only sparse warning I couldn't address
was this sparse warning which seems to be a false positive due to the way
the seq ops are used:

  CHECK   mm/kmemleak.c
mm/kmemleak.c:1207:13: warning: context imbalance in 'kmemleak_seq_start' - different lock contexts for basic block
mm/kmemleak.c:1261:17: warning: context imbalance in 'kmemleak_seq_stop' - unexpected unlock

I haven't tested this, nor do I know if the clear stuff will work, if you
can think of a better way please let me know. I'll test later when I get home.

Also, not sure who these patches go through, if welcomed.

Luis R. Rodriguez (5):
  kmemleak: use bool for true/false questions
  kmemleak: add clear command support
  kmemleak: move common painting code together
  kmemleak: fix sparse warning over overshadowed flags
  kmemleak: fix sparse warning for static declarations

 mm/kmemleak.c |   72 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 50 insertions(+), 22 deletions(-)

[PATCH 1/5] kmemleak: use bool for true/false questions

[Luis R. Rodriguez]

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 4872673..c6e71bb 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -264,17 +264,17 @@ static void kmemleak_disable(void);
  * Newly created objects don't have any color assigned (object->count == -1)
  * before the next memory scan when they become white.
  */
-static int color_white(const struct kmemleak_object *object)
+static bool color_white(const struct kmemleak_object *object)
 {
 	return object->count != -1 && object->count < object->min_count;
 }
 
-static int color_gray(const struct kmemleak_object *object)
+static bool color_gray(const struct kmemleak_object *object)
 {
 	return object->min_count != -1 && object->count >= object->min_count;
 }
 
-static int color_black(const struct kmemleak_object *object)
+static bool color_black(const struct kmemleak_object *object)
 {
 	return object->min_count == -1;
 }
@@ -284,7 +284,7 @@ static int color_black(const struct kmemleak_object *object)
  * not be deleted and have a minimum age to avoid false positives caused by
  * pointers temporarily stored in CPU registers.
  */
-static int unreferenced_object(struct kmemleak_object *object)
+static bool unreferenced_object(struct kmemleak_object *object)
 {
 	return (object->flags & OBJECT_ALLOCATED) && color_white(object) &&
 		time_before_eq(object->jiffies + jiffies_min_age,
-- 
1.6.3.3

Re: [PATCH 1/5] kmemleak: use bool for true/false questions

[Pekka Enberg]

On Thu, Sep 3, 2009 at 4:40 AM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote:
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

> ---
>  mm/kmemleak.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 4872673..c6e71bb 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -264,17 +264,17 @@ static void kmemleak_disable(void);
>  * Newly created objects don't have any color assigned (object->count == -1)
>  * before the next memory scan when they become white.
>  */
> -static int color_white(const struct kmemleak_object *object)
> +static bool color_white(const struct kmemleak_object *object)
>  {
>        return object->count != -1 && object->count < object->min_count;
>  }
>
> -static int color_gray(const struct kmemleak_object *object)
> +static bool color_gray(const struct kmemleak_object *object)
>  {
>        return object->min_count != -1 && object->count >= object->min_count;
>  }
>
> -static int color_black(const struct kmemleak_object *object)
> +static bool color_black(const struct kmemleak_object *object)
>  {
>        return object->min_count == -1;
>  }
> @@ -284,7 +284,7 @@ static int color_black(const struct kmemleak_object *object)
>  * not be deleted and have a minimum age to avoid false positives caused by
>  * pointers temporarily stored in CPU registers.
>  */
> -static int unreferenced_object(struct kmemleak_object *object)
> +static bool unreferenced_object(struct kmemleak_object *object)
>  {
>        return (object->flags & OBJECT_ALLOCATED) && color_white(object) &&
>                time_before_eq(object->jiffies + jiffies_min_age,
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH 2/5] kmemleak: add clear command support

[Luis R. Rodriguez]

In an ideal world your kmemleak output will be small,
when its not you can use the clear command to ingore previously
annotated kmemleak objects. We do this by painting them black.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index c6e71bb..4840212 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1294,6 +1294,26 @@ static int kmemleak_release(struct inode *inode, struct file *file)
 	return seq_release(inode, file);
 }
 
+static void kmemleak_clear(void)
+{
+	struct kmemleak_object *object;
+	unsigned long flags;
+
+	mutex_lock(&scan_mutex);
+	stop_scan_thread();
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(object, &object_list, object_list) {
+		spin_lock_irqsave(&object->lock, flags);
+		object->min_count = -1;
+		spin_unlock_irqrestore(&object->lock, flags);
+	}
+	rcu_read_unlock();
+	mutex_unlock(&scan_mutex);
+
+	start_scan_thread();
+}
+
 /*
  * File write operation to configure kmemleak at run-time. The following
  * commands can be written to the /sys/kernel/debug/kmemleak file:
@@ -1324,6 +1344,8 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
 
 	if (strncmp(buf, "off", 3) == 0)
 		kmemleak_disable();
+	else if (strncmp(buf, "clear", 8) == 0)
+		kmemleak_clear();
 	else if (strncmp(buf, "stack=on", 8) == 0)
 		kmemleak_stack_scan = 1;
 	else if (strncmp(buf, "stack=off", 9) == 0)
-- 
1.6.3.3

[PATCH 3/5] kmemleak: move common painting code together

[Luis R. Rodriguez]

When painting grey or black we do the same thing, bring
this together into a helper and identify coloring grey or
black explicitly with defines. This makes this a little
easier to read.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 4840212..7bb1d48 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -120,6 +120,9 @@ struct kmemleak_scan_area {
 	size_t length;
 };
 
+#define KMEMLEAK_GREY	0
+#define KMEMLEAK_BLACK	-1
+
 /*
  * Structure holding the metadata for each allocated memory block.
  * Modifications to such objects should be made while holding the
@@ -266,17 +269,19 @@ static void kmemleak_disable(void);
  */
 static bool color_white(const struct kmemleak_object *object)
 {
-	return object->count != -1 && object->count < object->min_count;
+	return object->count != KMEMLEAK_BLACK &&
+		object->count < object->min_count;
 }
 
 static bool color_gray(const struct kmemleak_object *object)
 {
-	return object->min_count != -1 && object->count >= object->min_count;
+	return object->min_count != KMEMLEAK_BLACK &&
+		object->count >= object->min_count;
 }
 
 static bool color_black(const struct kmemleak_object *object)
 {
-	return object->min_count == -1;
+	return object->min_count == KMEMLEAK_BLACK;
 }
 
 /*
@@ -604,13 +609,21 @@ static void delete_object_part(unsigned long ptr, size_t size)
 
 	put_object(object);
 }
+
+static void paint_it(struct kmemleak_object *object, int color)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&object->lock, flags);
+	object->min_count = color;
+	spin_unlock_irqrestore(&object->lock, flags);
+}
+
 /*
  * Make a object permanently as gray-colored so that it can no longer be
  * reported as a leak. This is used in general to mark a false positive.
  */
 static void make_gray_object(unsigned long ptr)
 {
-	unsigned long flags;
 	struct kmemleak_object *object;
 
 	object = find_and_get_object(ptr, 0);
@@ -618,10 +631,7 @@ static void make_gray_object(unsigned long ptr)
 		kmemleak_warn("Graying unknown object at 0x%08lx\n", ptr);
 		return;
 	}
-
-	spin_lock_irqsave(&object->lock, flags);
-	object->min_count = 0;
-	spin_unlock_irqrestore(&object->lock, flags);
+	paint_it(object, KMEMLEAK_GREY);
 	put_object(object);
 }
 
@@ -631,7 +641,6 @@ static void make_gray_object(unsigned long ptr)
  */
 static void make_black_object(unsigned long ptr)
 {
-	unsigned long flags;
 	struct kmemleak_object *object;
 
 	object = find_and_get_object(ptr, 0);
@@ -639,10 +648,7 @@ static void make_black_object(unsigned long ptr)
 		kmemleak_warn("Blacking unknown object at 0x%08lx\n", ptr);
 		return;
 	}
-
-	spin_lock_irqsave(&object->lock, flags);
-	object->min_count = -1;
-	spin_unlock_irqrestore(&object->lock, flags);
+	paint_it(object, KMEMLEAK_BLACK);
 	put_object(object);
 }
 
-- 
1.6.3.3

Re: [PATCH 3/5] kmemleak: move common painting code together

[Pekka Enberg]

On Thu, Sep 3, 2009 at 4:40 AM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote:
> When painting grey or black we do the same thing, bring
> this together into a helper and identify coloring grey or
> black explicitly with defines. This makes this a little
> easier to read.
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

[PATCH 4/5] kmemleak: fix sparse warning over overshadowed flags

[Luis R. Rodriguez]

This fixes this sparse warning:
mm/kmemleak.c:512:31: warning: symbol 'flags' shadows an earlier one
mm/kmemleak.c:448:23: originally declared here

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 7bb1d48..09ddf9c 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -509,14 +509,14 @@ static void create_object(unsigned long ptr, size_t size, int min_count,
 	 * random memory blocks.
 	 */
 	if (node != &object->tree_node) {
-		unsigned long flags;
+		unsigned long flags_object;
 
 		kmemleak_stop("Cannot insert 0x%lx into the object search tree "
 			      "(already existing)\n", ptr);
 		object = lookup_object(ptr, 1);
-		spin_lock_irqsave(&object->lock, flags);
+		spin_lock_irqsave(&object->lock, flags_object);
 		dump_object_info(object);
-		spin_unlock_irqrestore(&object->lock, flags);
+		spin_unlock_irqrestore(&object->lock, flags_object);
 
 		goto out;
 	}
-- 
1.6.3.3

Re: [PATCH 4/5] kmemleak: fix sparse warning over overshadowed flags

[Pekka Enberg]

On Thu, Sep 3, 2009 at 4:40 AM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote:
> This fixes this sparse warning:
> mm/kmemleak.c:512:31: warning: symbol 'flags' shadows an earlier one
> mm/kmemleak.c:448:23: originally declared here
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  mm/kmemleak.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 7bb1d48..09ddf9c 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -509,14 +509,14 @@ static void create_object(unsigned long ptr, size_t size, int min_count,
>         * random memory blocks.
>         */
>        if (node != &object->tree_node) {
> -               unsigned long flags;
> +               unsigned long flags_object;
>
>                kmemleak_stop("Cannot insert 0x%lx into the object search tree "
>                              "(already existing)\n", ptr);
>                object = lookup_object(ptr, 1);
> -               spin_lock_irqsave(&object->lock, flags);
> +               spin_lock_irqsave(&object->lock, flags_object);
>                dump_object_info(object);
> -               spin_unlock_irqrestore(&object->lock, flags);
> +               spin_unlock_irqrestore(&object->lock, flags_object);

Catalin, why do we have nested "irqsave" in this function? Can't we
just make these two spin_lock() and spin_unlock()?

>
>                goto out;
>        }
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH 5/5] kmemleak: fix sparse warning for static declarations

[Luis R. Rodriguez]

This fixes these sparse warnings:

mm/kmemleak.c:1179:6: warning: symbol 'start_scan_thread' was not declared. Should it be static?
mm/kmemleak.c:1194:6: warning: symbol 'stop_scan_thread' was not declared. Should it be static?

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 09ddf9c..38496ab 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1176,7 +1176,7 @@ static int kmemleak_scan_thread(void *arg)
  * Start the automatic memory scanning thread. This function must be called
  * with the scan_mutex held.
  */
-void start_scan_thread(void)
+static void start_scan_thread(void)
 {
 	if (scan_thread)
 		return;
@@ -1191,7 +1191,7 @@ void start_scan_thread(void)
  * Stop the automatic memory scanning thread. This function must be called
  * with the scan_mutex held.
  */
-void stop_scan_thread(void)
+static void stop_scan_thread(void)
 {
 	if (scan_thread) {
 		kthread_stop(scan_thread);
-- 
1.6.3.3

Re: [PATCH 5/5] kmemleak: fix sparse warning for static declarations

[Pekka Enberg]

On Thu, Sep 3, 2009 at 4:40 AM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote:
> This fixes these sparse warnings:
>
> mm/kmemleak.c:1179:6: warning: symbol 'start_scan_thread' was not declared. Should it be static?
> mm/kmemleak.c:1194:6: warning: symbol 'stop_scan_thread' was not declared. Should it be static?
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>