Re: [PATCH v11 26/33] perf kmem: Fix memory leaks on error path and when skipping

sashiko-bot
2026-04-13T01:50:06Z

Sashiko AI review found 2 potential issue(s):
- [High] Integer overflow in gfpcmp() breaks qsort/bsearch strict weak ordering
- [High] Valid but unrecognized GFP flags cause fatal -ENOMEM aborts
--

commit 24b8d3629b127f6af817b78e1ca9d3a0f9e7d9fd
Author: Ian Rogers <irogers@google.com>

perf kmem: Fix memory leaks on error path and when skipping

This commit addresses memory leaks in the perf kmem tool by adding missing
frees for allocated strings and dropping thread references when samples are
skipped. It introduces an err_out label in parse_gfp_flags for cleaner error
handling.

> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index 68a39f56204db..daf2272c73372 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c

[ ... ]

> @@ -783,17 +783,21 @@ static int parse_gfp_flags(struct perf_sample *sample, unsigned int gfp_flags)
>
> new = realloc(gfps, (nr_gfps + 1) * sizeof(*gfps));
> if (new == NULL)
> - return -ENOMEM;
> + goto err_out;
>
> gfps = new;
> - new += nr_gfps++;
> + new += nr_gfps;
>
> new->flags = gfp_flags;
> new->human_readable = strdup(str + 10);
> + if (!new->human_readable)
> + goto err_out;
> new->compact_str = compact_gfp_flags(str + 10);
> - if (!new->human_readable || !new->compact_str)
> - return -ENOMEM;
> -
> + if (!new->compact_str) {
> + free(new->human_readable);
> + goto err_out;
> + }

This isn't a new bug introduced by this patch, but does this error path
incorrectly assume a memory allocation failure?

If compact_gfp_flags() encounters a valid but unrecognized GFP flag (such
as a newly added kernel flag not present in gfp_compact_table), it seems
it can return NULL without it being an out of memory condition.

Because the code jumps to err_out and returns -ENOMEM, could this cause the
entire perf kmem session to fatally abort when processing data from newer
kernels?

> + nr_gfps++;
> qsort(gfps, nr_gfps, sizeof(*gfps), gfpcmp);

This is another pre-existing issue, but could this qsort() lead to
out-of-bounds memory accesses?

Looking at gfpcmp(), the subtraction is done using unsigned 32-bit
arithmetic:

static int gfpcmp(const void *a, const void *b)
{
const struct gfp_flag *fa = a;
const struct gfp_flag *fb = b;

return fa->flags - fb->flags;
}

If fa->flags has the MSB set (e.g. 0x80000000) and fb->flags is 0, the
result is 0x80000000. When implicitly cast to a signed int for the return
value, it becomes a negative number, indicating 0x80000000 is less than 0.
Conversely, fb->flags - fa->flags would also yield a negative value.

Since gfp_flags are parsed from the perf.data file, does this violate the
strict weak ordering requirement of qsort() where cmp(a,b) and cmp(b,a)
must not both be negative?

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260413012227.1089445-1-irogers@google.com?part=26