Here is my third respin, this time rebased ontop of:
git://linux-arm.org/linux-2.6 kmemleak
As suggested by Catalin we now clear the list by only painting reported
unreferenced objects and the color we use is grey to ensure future
scans are possible on these same objects to account for new allocations
in the future referenced on the cleared objects.
Patch 3 is now a little different, now with a paint_ptr() and
a __paint_it() helper.
I tested this by clearing kmemleak after bootup, then writing my
own buggy module which kmalloc()'d onto some internal pointer,
scanned, unloaded, and scanned again and then saw a new shiny
report come up:
unreferenced object 0xffff88003ad70920 (size 16):
comm "insmod", pid 7449, jiffies 4296458482
hex dump (first 16 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff814e9d55>] kmemleak_alloc+0x25/0x60
[<ffffffff81118c3b>] kmem_cache_alloc+0x14b/0x1c0
[<ffffffffa000a07f>] 0xffffffffa000a07f
[<ffffffff8100a047>] do_one_initcall+0x37/0x1a0
[<ffffffff810950d9>] sys_init_module+0xd9/0x230
[<ffffffff81011f02>] system_call_fastpath+0x16/0x1b
[<ffffffffffffffff>] 0xffffffffffffffff
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
Documentation/kmemleak.txt | 30 +++++++++++
mm/kmemleak.c | 116 ++++++++++++++++++++++++++++++--------------
2 files changed, 109 insertions(+), 37 deletions(-)
Acked-by: Pekka Enberg <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
mm/kmemleak.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 401a89a..cde69f5 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -305,17 +305,17 @@ static void hex_dump_object(struct seq_file *seq,
* 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;
}
@@ -325,7 +325,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
In an ideal world your kmemleak output will be small, when its
not (usually during initial bootup) you can use the clear command
to ingore previously reported and unreferenced kmemleak objects. We
do this by painting all currently reported unreferenced objects grey.
We paint them grey instead of black to allow future scans on the same
objects as such objects could still potentially reference newly
allocated objects in the future.
To test a critical section on demand with a clean
/sys/kernel/debug/kmemleak you can do:
echo clear > /sys/kernel/debug/kmemleak
test your kernel or modules
echo scan > /sys/kernel/debug/kmemleak
Then as usual to get your report with:
cat /sys/kernel/debug/kmemleak
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
Documentation/kmemleak.txt | 30 ++++++++++++++++++++++++++++++
mm/kmemleak.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+), 0 deletions(-)
diff --git a/Documentation/kmemleak.txt b/Documentation/kmemleak.txt
index fa93249..3a66dcf 100644
--- a/Documentation/kmemleak.txt
+++ b/Documentation/kmemleak.txt
@@ -27,6 +27,13 @@ To trigger an intermediate memory scan:
# echo scan > /sys/kernel/debug/kmemleak
+To clear the list of all current possible memory leaks:
+
+ # echo clear > /sys/kernel/debug/kmemleak
+
+New leaks will then come up upon reading /sys/kernel/debug/kmemleak
+again.
+
Note that the orphan objects are listed in the order they were allocated
and one object at the beginning of the list may cause other subsequent
objects to be reported as orphan.
@@ -35,6 +42,8 @@ Memory scanning parameters can be modified at run-time by writing to the
/sys/kernel/debug/kmemleak file. The following parameters are supported:
off - disable kmemleak (irreversible)
+ clear - clear list of current memory leak suspects, done by
+ marking all current reported unreferenced objects grey.
scan=on - start the automatic memory scanning thread (default)
scan=off - stop the automatic memory scanning thread
scan=<secs> - set the automatic memory scanning period in seconds
@@ -85,6 +94,27 @@ avoid this, kmemleak can also store the number of values pointing to an
address inside the block address range that need to be found so that the
block is not considered a leak. One example is __vmalloc().
+Testing specific sections with kmemleak
+---------------------------------------
+
+Upon initial bootup your /sys/kernel/debug/kmemleak output page may be
+quite extensive. This can also be the case if you have very buggy code
+when doing development. To work around these situations you can use the
+'clear' command to clear all reported unreferenced objects from the
+/sys/kernel/debug/kmemleak output. By issuing a 'scan' after a 'clear'
+you can find new unreferenced objects; this should help with testing
+speficic sections of code.
+
+To test a critical section on demand with a clean kmemleak do:
+
+echo clear > /sys/kernel/debug/kmemleak
+ test your kernel or modules
+echo scan > /sys/kernel/debug/kmemleak
+
+Then as usual to get your report with
+
+cat /sys/kernel/debug/kmemleak
+
Kmemleak API
------------
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index cde69f5..76dd7af 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1404,9 +1404,38 @@ static int dump_str_object_info(const char *str)
}
/*
+ * We use grey instead of black to ensure we can do future
+ * scans on the same objects. If we did not do future scans
+ * these black objects could potentially contain references to
+ * newly allocated objects in the future and we'd end up with
+ * false positives.
+ */
+static void kmemleak_clear(void)
+{
+ struct kmemleak_object *object;
+ unsigned long flags;
+
+ stop_scan_thread();
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(object, &object_list, object_list) {
+ spin_lock_irqsave(&object->lock, flags);
+ if ((object->flags & OBJECT_REPORTED) &&
+ unreferenced_object(object))
+ object->min_count = -1;
+ spin_unlock_irqrestore(&object->lock, flags);
+ }
+ rcu_read_unlock();
+
+ 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:
* off - disable kmemleak (irreversible)
+ * clear - mark all current reported unreferenced kmemleak objects as
+ * grey to ingore printing them
* scan=on - start the automatic memory scanning thread
* scan=off - stop the automatic memory scanning thread
* scan=... - set the automatic memory scanning period in seconds (0 to
@@ -1432,6 +1461,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", 5) == 0)
+ kmemleak_clear();
else if (strncmp(buf, "scan=on", 7) == 0)
start_scan_thread();
else if (strncmp(buf, "scan=off", 8) == 0)
--
1.6.3.3
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 <[email protected]>
---
mm/kmemleak.c | 68 +++++++++++++++++++++++++++++++++-----------------------
1 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 76dd7af..18dfd62 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -122,6 +122,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
@@ -307,17 +310,19 @@ static void hex_dump_object(struct seq_file *seq,
*/
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;
}
/*
@@ -658,47 +663,54 @@ static void delete_object_part(unsigned long ptr, size_t size)
put_object(object);
}
-/*
- * 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)
+
+static void __paint_it(struct kmemleak_object *object, int color)
+{
+ object->min_count = color;
+ if (color == KMEMLEAK_BLACK)
+ object->flags |= OBJECT_NO_SCAN;
+}
+
+static void paint_it(struct kmemleak_object *object, int color)
{
unsigned long flags;
+ spin_lock_irqsave(&object->lock, flags);
+ __paint_it(object, color);
+ spin_unlock_irqrestore(&object->lock, flags);
+}
+
+static void paint_ptr(unsigned long ptr, int color)
+{
struct kmemleak_object *object;
object = find_and_get_object(ptr, 0);
if (!object) {
- kmemleak_warn("Graying unknown object at 0x%08lx\n", ptr);
+ kmemleak_warn("Tried to color unknown object "
+ "at 0x%08lx as %s\n", ptr,
+ (color == KMEMLEAK_GREY) ? "Grey" :
+ (color == KMEMLEAK_BLACK) ? "Black" : "Unknown");
return;
}
-
- spin_lock_irqsave(&object->lock, flags);
- object->min_count = 0;
- spin_unlock_irqrestore(&object->lock, flags);
+ paint_it(object, color);
put_object(object);
}
/*
+ * 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)
+{
+ paint_ptr(ptr, KMEMLEAK_GREY);
+}
+
+/*
* Mark the object as black-colored so that it is ignored from scans and
* reporting.
*/
static void make_black_object(unsigned long ptr)
{
- unsigned long flags;
- struct kmemleak_object *object;
-
- object = find_and_get_object(ptr, 0);
- if (!object) {
- kmemleak_warn("Blacking unknown object at 0x%08lx\n", ptr);
- return;
- }
-
- spin_lock_irqsave(&object->lock, flags);
- object->min_count = -1;
- object->flags |= OBJECT_NO_SCAN;
- spin_unlock_irqrestore(&object->lock, flags);
- put_object(object);
+ paint_ptr(ptr, KMEMLEAK_BLACK);
}
/*
@@ -1422,7 +1434,7 @@ static void kmemleak_clear(void)
spin_lock_irqsave(&object->lock, flags);
if ((object->flags & OBJECT_REPORTED) &&
unreferenced_object(object))
- object->min_count = -1;
+ __paint_it(object, KMEMLEAK_GREY);
spin_unlock_irqrestore(&object->lock, flags);
}
rcu_read_unlock();
--
1.6.3.3
A secondary irq_save is not required as a locking before it was
already disabling irqs.
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 <[email protected]>
---
mm/kmemleak.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 18dfd62..d078621 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -552,6 +552,7 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
object->tree_node.last = ptr + size - 1;
write_lock_irqsave(&kmemleak_lock, flags);
+
min_addr = min(min_addr, ptr);
max_addr = max(max_addr, ptr + size);
node = prio_tree_insert(&object_tree_root, &object->tree_node);
@@ -562,14 +563,12 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
* random memory blocks.
*/
if (node != &object->tree_node) {
- unsigned long flags;
-
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(&object->lock);
dump_object_info(object);
- spin_unlock_irqrestore(&object->lock, flags);
+ spin_unlock(&object->lock);
goto out;
}
--
1.6.3.3
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?
Acked-by: Pekka Enberg <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
mm/kmemleak.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index d078621..9b14e24 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1269,7 +1269,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;
@@ -1284,7 +1284,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
Luis R. Rodriguez wrote:
> A secondary irq_save is not required as a locking before it was
> already disabling irqs.
>
> 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 <[email protected]>
Acked-by: Pekka Enberg <[email protected]>
On Fri, 2009-09-04 at 17:44 -0700, Luis R. Rodriguez wrote:
> Here is my third respin, this time rebased ontop of:
>
> git://linux-arm.org/linux-2.6 kmemleak
>
> As suggested by Catalin we now clear the list by only painting reported
> unreferenced objects and the color we use is grey to ensure future
> scans are possible on these same objects to account for new allocations
> in the future referenced on the cleared objects.
>
> Patch 3 is now a little different, now with a paint_ptr() and
> a __paint_it() helper.
Thanks for the patches. They look ok now, I'll merge them tomorrow to my
kmemleak branch and give them a try.
> I tested this by clearing kmemleak after bootup, then writing my
> own buggy module which kmalloc()'d onto some internal pointer,
> scanned, unloaded, and scanned again and then saw a new shiny
> report come up:
BTW, kmemleak comes with a test module which does this.
--
Catalin
On Fri, 2009-09-04 at 17:44 -0700, Luis R. Rodriguez wrote:
> /*
> + * We use grey instead of black to ensure we can do future
> + * scans on the same objects. If we did not do future scans
> + * these black objects could potentially contain references to
> + * newly allocated objects in the future and we'd end up with
> + * false positives.
> + */
> +static void kmemleak_clear(void)
> +{
> + struct kmemleak_object *object;
> + unsigned long flags;
> +
> + stop_scan_thread();
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(object, &object_list, object_list) {
> + spin_lock_irqsave(&object->lock, flags);
> + if ((object->flags & OBJECT_REPORTED) &&
> + unreferenced_object(object))
> + object->min_count = -1;
> + spin_unlock_irqrestore(&object->lock, flags);
> + }
> + rcu_read_unlock();
> +
> + start_scan_thread();
> +}
Do we need to stop and start the scanning thread here? When starting it,
it will trigger a memory scan automatically. I don't think we want this
as a side-effect, so I dropped these lines from your patch.
Also you set min_count to -1 here which means black object, so a
subsequent patch corrects it. I'll set min_count to 0 here in case
anyone bisects over it.
--
Catalin
On Mon, Sep 07, 2009 at 10:29:30AM -0700, Catalin Marinas wrote:
> On Fri, 2009-09-04 at 17:44 -0700, Luis R. Rodriguez wrote:
> > Here is my third respin, this time rebased ontop of:
> >
> > git://linux-arm.org/linux-2.6 kmemleak
> >
> > As suggested by Catalin we now clear the list by only painting reported
> > unreferenced objects and the color we use is grey to ensure future
> > scans are possible on these same objects to account for new allocations
> > in the future referenced on the cleared objects.
> >
> > Patch 3 is now a little different, now with a paint_ptr() and
> > a __paint_it() helper.
>
> Thanks for the patches. They look ok now, I'll merge them tomorrow to my
> kmemleak branch and give them a try.
>
> > I tested this by clearing kmemleak after bootup, then writing my
> > own buggy module which kmalloc()'d onto some internal pointer,
> > scanned, unloaded, and scanned again and then saw a new shiny
> > report come up:
>
> BTW, kmemleak comes with a test module which does this.
Thanks for the note, I'll use this in case I need to test more stuff.
Luis
On Tue, Sep 8, 2009 at 9:11 AM, Catalin Marinas<[email protected]> wrote:
> On Fri, 2009-09-04 at 17:44 -0700, Luis R. Rodriguez wrote:
>> /*
>> + * We use grey instead of black to ensure we can do future
>> + * scans on the same objects. If we did not do future scans
>> + * these black objects could potentially contain references to
>> + * newly allocated objects in the future and we'd end up with
>> + * false positives.
>> + */
>> +static void kmemleak_clear(void)
>> +{
>> + struct kmemleak_object *object;
>> + unsigned long flags;
>> +
>> + stop_scan_thread();
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(object, &object_list, object_list) {
>> + spin_lock_irqsave(&object->lock, flags);
>> + if ((object->flags & OBJECT_REPORTED) &&
>> + unreferenced_object(object))
>> + object->min_count = -1;
>> + spin_unlock_irqrestore(&object->lock, flags);
>> + }
>> + rcu_read_unlock();
>> +
>> + start_scan_thread();
>> +}
>
> Do we need to stop and start the scanning thread here? When starting it,
> it will trigger a memory scan automatically. I don't think we want this
> as a side-effect, so I dropped these lines from your patch.
OK thanks.
> Also you set min_count to -1 here which means black object, so a
> subsequent patch corrects it. I'll set min_count to 0 here in case
> anyone bisects over it.
Dah, thanks for catching that, seems I only fixed the named set.
Luis