[PATCH 0/2] tweak kmemleak report format

[PATCH 0/2] tweak kmemleak report format

[Jim Cromie]

These 2 patches make minor changes to the report:

1st strips "age <increasing>" from output.  This makes the output
idempotent; unchanging until a new leak is reported.

2nd adds the backtrace.checksum to the "backtrace:" line.  This lets a
user see repeats without actually reading the whole backtrace.  So now
the backtrace line looks like this:

  backtrace (ck 603070071):

I surveyed for un-wanted effects upon users:

Syzkaller parses kmemleak in executor/common_linux.h:
static void check_leaks(char** frames, int nframes)

It just counts occurrences of "unreferenced object", specifically it
does not look for "age", nor would it choke on "crc" being added.

github has 3 repos with "kmemleak" mentioned, all are moribund.
gitlab has 0 hits on "kmemleak".


Jim Cromie (2):
  kmemleak: drop (age <increasing>) from leak record
  kmemleak: add checksum to backtrace report

 mm/kmemleak.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

-- 
2.41.0

[PATCH 1/2] kmemleak: drop (age <increasing>) from leak record

[Jim Cromie]

Displaying age is pretty, but counter-productive; it changes with
current-time, so it surrenders idempotency of the output, which breaks
simple hash-based cataloging of the records by the user.

The trouble: sequential reads, wo new leaks, get new results:

  :#> sum /sys/kernel/debug/kmemleak
  53439    74 /sys/kernel/debug/kmemleak
  :#> sum /sys/kernel/debug/kmemleak
  59066    74 /sys/kernel/debug/kmemleak

and age is why (nothing else changes):

  :#> grep -v age /sys/kernel/debug/kmemleak | sum
  58894    67
  :#> grep -v age /sys/kernel/debug/kmemleak | sum
  58894    67

Since jiffies is already printed in the "comm" line, age adds nothing.

Notably, syzkaller reads kmemleak only for "unreferenced object", and
won't care about this reform of age-ism.  A few moribund github repos
mention it, but don't compile.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 mm/kmemleak.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 1eacca03bedd..10c9b611c395 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -355,14 +355,12 @@ static void print_unreferenced(struct seq_file *seq,
 	int i;
 	unsigned long *entries;
 	unsigned int nr_entries;
-	unsigned int msecs_age = jiffies_to_msecs(jiffies - object->jiffies);
 
 	nr_entries = stack_depot_fetch(object->trace_handle, &entries);
 	warn_or_seq_printf(seq, "unreferenced object 0x%08lx (size %zu):\n",
 			  object->pointer, object->size);
-	warn_or_seq_printf(seq, "  comm \"%s\", pid %d, jiffies %lu (age %d.%03ds)\n",
-			   object->comm, object->pid, object->jiffies,
-			   msecs_age / 1000, msecs_age % 1000);
+	warn_or_seq_printf(seq, "  comm \"%s\", pid %d, jiffies %lu\n",
+			   object->comm, object->pid, object->jiffies);
 	hex_dump_object(seq, object);
 	warn_or_seq_printf(seq, "  backtrace:\n");
 
-- 
2.41.0

Re: [PATCH 1/2] kmemleak: drop (age <increasing>) from leak record

[Catalin Marinas]

On Thu, Nov 16, 2023 at 03:43:17PM -0700, Jim Cromie wrote:
> Displaying age is pretty, but counter-productive; it changes with
> current-time, so it surrenders idempotency of the output, which breaks
> simple hash-based cataloging of the records by the user.
> 
> The trouble: sequential reads, wo new leaks, get new results:
> 
>   :#> sum /sys/kernel/debug/kmemleak
>   53439    74 /sys/kernel/debug/kmemleak
>   :#> sum /sys/kernel/debug/kmemleak
>   59066    74 /sys/kernel/debug/kmemleak
> 
> and age is why (nothing else changes):
> 
>   :#> grep -v age /sys/kernel/debug/kmemleak | sum
>   58894    67
>   :#> grep -v age /sys/kernel/debug/kmemleak | sum
>   58894    67
> 
> Since jiffies is already printed in the "comm" line, age adds nothing.
> 
> Notably, syzkaller reads kmemleak only for "unreferenced object", and
> won't care about this reform of age-ism.  A few moribund github repos
> mention it, but don't compile.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

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

[PATCH 2/2] kmemleak: add checksum to backtrace report

[Jim Cromie]

Change /sys/kernel/debug/kmemleak report format slightly, adding
"(extra info)" to the backtrace header:

from: "  backtrace:"
to:   "  backtrace (crc <cksum>):"

The <cksum> allows a user to see recurring backtraces without
detailed/careful reading of multiline stacks.  So after cycling
kmemleak-test a few times, I know some leaks are repeating.

  bash-5.2# grep backtrace /sys/kernel/debug/kmemleak | wc
     62     186    1792
  bash-5.2# grep backtrace /sys/kernel/debug/kmemleak | sort -u | wc
     37     111    1067

syzkaller parses kmemleak for "unreferenced object" only, so is
unaffected by this change.  Other github repos are moribund.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 mm/kmemleak.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 10c9b611c395..4c22a2d7cab4 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -362,7 +362,7 @@ static void print_unreferenced(struct seq_file *seq,
 	warn_or_seq_printf(seq, "  comm \"%s\", pid %d, jiffies %lu\n",
 			   object->comm, object->pid, object->jiffies);
 	hex_dump_object(seq, object);
-	warn_or_seq_printf(seq, "  backtrace:\n");
+	warn_or_seq_printf(seq, "  backtrace (crc %x):\n", object->checksum);
 
 	for (i = 0; i < nr_entries; i++) {
 		void *ptr = (void *)entries[i];
-- 
2.41.0

Re: [PATCH 2/2] kmemleak: add checksum to backtrace report

[Catalin Marinas]

On Thu, Nov 16, 2023 at 03:43:18PM -0700, Jim Cromie wrote:
> Change /sys/kernel/debug/kmemleak report format slightly, adding
> "(extra info)" to the backtrace header:
> 
> from: "  backtrace:"
> to:   "  backtrace (crc <cksum>):"
> 
> The <cksum> allows a user to see recurring backtraces without
> detailed/careful reading of multiline stacks.  So after cycling
> kmemleak-test a few times, I know some leaks are repeating.
> 
>   bash-5.2# grep backtrace /sys/kernel/debug/kmemleak | wc
>      62     186    1792
>   bash-5.2# grep backtrace /sys/kernel/debug/kmemleak | sort -u | wc
>      37     111    1067
> 
> syzkaller parses kmemleak for "unreferenced object" only, so is
> unaffected by this change.  Other github repos are moribund.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

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

Re: [PATCH 2/2] kmemleak: add checksum to backtrace report

[jim.cromie]

On Sat, Nov 18, 2023 at 10:36 AM Catalin Marinas
<catalin.marinas@arm.com> wrote:
>
> On Thu, Nov 16, 2023 at 03:43:18PM -0700, Jim Cromie wrote:
> > Change /sys/kernel/debug/kmemleak report format slightly, adding
> > "(extra info)" to the backtrace header:
> >
> > from: "  backtrace:"
> > to:   "  backtrace (crc <cksum>):"
> >
> > The <cksum> allows a user to see recurring backtraces without
> > detailed/careful reading of multiline stacks.  So after cycling
> > kmemleak-test a few times, I know some leaks are repeating.
> >
> >   bash-5.2# grep backtrace /sys/kernel/debug/kmemleak | wc
> >      62     186    1792
> >   bash-5.2# grep backtrace /sys/kernel/debug/kmemleak | sort -u | wc
> >      37     111    1067

So, speculating from here,
what else could be done with <crc: deadbeef> ?

1 - (optionally) collapsing backtraces, replacing the stack with
   "seen previously, at <mumble>"
    of some clear / succinct flavor (maybe several ?)

2 - stack specific instructions from user

echo drop/ignore/histogram/<mumble>  deadbeef \
     > /sys/kernel/debug/kmemleak

this crc-specific instruction could control the optionality of 1.
on a trace-by-trace basis even.

The "seen previously" would be an obvious place to look
for a root cause of a detected leak.
tools beyond drop/ignore/histogram/<mumble>
are worth some consideration ?

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

Re: [PATCH 0/2] tweak kmemleak report format

[jim.cromie]

On Thu, Nov 16, 2023 at 3:43 PM Jim Cromie <jim.cromie@gmail.com> wrote:
>
> These 2 patches make minor changes to the report:
>
> 1st strips "age <increasing>" from output.  This makes the output
> idempotent; unchanging until a new leak is reported.
>
> 2nd adds the backtrace.checksum to the "backtrace:" line.  This lets a
> user see repeats without actually reading the whole backtrace.  So now
> the backtrace line looks like this:
>
>   backtrace (ck 603070071):

meh, in the actual patch, thats

backtrace (crc <%x format>)