perf kwork: Fix memory management of kwork_work

This commit addresses several memory management issues in builtin-kwork.c:

1. Implements a global cleanup function perf_kwork__exit to free all
   kwork_work and kwork_atom_page objects at the end of the command.

2. Ensures all 'name' fields in struct kwork_work are malloc-ed (or NULL)
   and properly freed by using strdup and zfree.

3. Fixes memory leaks in top_merge_tasks where kwork_work objects were
   dropped without being freed.

4. Adds robustness with NULL checks for name fields.

5. Fixes workqueue_work_init to correctly resolve and strdup kernel
   function names, preventing bad-free errors.

Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
index a4604e1520023b0182d54fb2585b1065ceab1922..f793ea578515d08cf4058bb097656e32a6298e5a 100644 (file)
--- a/tools/perf/builtin-kwork.c
+++ b/tools/perf/builtin-kwork.c
@@ -323,8 +323,8 @@ static struct kwork_work *work_search(struct rb_root_cached *root,
                else if (cmp < 0)
                        node = node->rb_right;
                else {
-                       if (work->name == NULL)
-                               work->name = key->name;
+                       if (work->name == NULL && key->name != NULL)
+                               work->name = strdup(key->name);
                        return work;
                }
        }
@@ -371,11 +371,54 @@ static struct kwork_work *work_new(struct kwork_work *key)
 
        work->id = key->id;
        work->cpu = key->cpu;
-       work->name = key->name;
+       work->name = key->name ? strdup(key->name) : NULL;
        work->class = key->class;
        return work;
 }
 
+
+static void work_delete(struct kwork_work *work)
+{
+       if (work) {
+               work_exit(work);
+               free(work);
+       }
+}
+
+static void kwork_work__free_root(struct rb_root_cached *root)
+{
+       struct rb_node *next;
+       struct kwork_work *work;
+
+       while ((next = rb_first_cached(root))) {
+               work = rb_entry(next, struct kwork_work, node);
+               rb_erase_cached(next, root);
+               work_delete(work);
+       }
+}
+
+static void perf_kwork__exit(struct perf_kwork *kwork)
+{
+       struct kwork_class *class;
+       struct kwork_atom_page *page, *tmp_page;
+
+       list_for_each_entry(class, &kwork->class_list, list) {
+               kwork_work__free_root(&class->work_root);
+       }
+
+       kwork_work__free_root(&kwork->sorted_work_root);
+
+       list_for_each_entry_safe(page, tmp_page, &kwork->atom_page_list, list) {
+               list_del_init(&page->list);
+               free(page);
+       }
+
+       INIT_LIST_HEAD(&kwork->class_list);
+       INIT_LIST_HEAD(&kwork->atom_page_list);
+       INIT_LIST_HEAD(&kwork->sort_list);
+       INIT_LIST_HEAD(&kwork->cmp_id);
+}
+
 static struct kwork_work *work_findnew(struct rb_root_cached *root,
                                       struct kwork_work *key,
                                       struct list_head *sort_list)
@@ -453,25 +496,29 @@ static int work_push_atom(struct perf_kwork *kwork,
                          struct kwork_work **ret_work,
                          bool overwrite)
 {
-       struct kwork_atom *atom, *dst_atom, *last_atom;
+       struct kwork_atom *atom = NULL, *dst_atom, *last_atom;
        struct kwork_work *work, key;
+       int ret = 0;
 
        BUG_ON(class->work_init == NULL);
        class->work_init(kwork, class, &key, src_type, sample, machine);
 
        atom = atom_new(kwork, sample);
-       if (atom == NULL)
-               return -1;
+       if (atom == NULL) {
+               ret = -1;
+               goto out;
+       }
 
        work = work_findnew(&class->work_root, &key, &kwork->cmp_id);
        if (work == NULL) {
                atom_free(atom);
-               return -1;
+               ret = -1;
+               goto out;
        }
 
        if (!profile_event_match(kwork, work, sample)) {
                atom_free(atom);
-               return 0;
+               goto out;
        }
 
        if (dst_type < KWORK_TRACE_MAX) {
@@ -498,8 +545,9 @@ static int work_push_atom(struct perf_kwork *kwork,
        }
 
        list_add_tail(&atom->list, &work->atom_list[src_type]);
-
-       return 0;
+out:
+       work_exit(&key);
+       return ret;
 }
 
 static struct kwork_atom *work_pop_atom(struct perf_kwork *kwork,
@@ -510,7 +558,7 @@ static struct kwork_atom *work_pop_atom(struct perf_kwork *kwork,
                                        struct machine *machine,
                                        struct kwork_work **ret_work)
 {
-       struct kwork_atom *atom, *src_atom;
+       struct kwork_atom *atom = NULL, *src_atom;
        struct kwork_work *work, key;
 
        BUG_ON(class->work_init == NULL);
@@ -521,15 +569,15 @@ static struct kwork_atom *work_pop_atom(struct perf_kwork *kwork,
                *ret_work = work;
 
        if (work == NULL)
-               return NULL;
+               goto out;
 
        if (!profile_event_match(kwork, work, sample))
-               return NULL;
+               goto out;
 
        atom = list_last_entry_or_null(&work->atom_list[dst_type],
                                       struct kwork_atom, list);
        if (atom != NULL)
-               return atom;
+               goto out;
 
        src_atom = atom_new(kwork, sample);
        if (src_atom != NULL)
@@ -538,8 +586,9 @@ static struct kwork_atom *work_pop_atom(struct perf_kwork *kwork,
                if (ret_work != NULL)
                        *ret_work = NULL;
        }
-
-       return NULL;
+out:
+       work_exit(&key);
+       return atom;
 }
 
 static struct kwork_work *find_work_by_id(struct rb_root_cached *root,
@@ -1002,13 +1051,16 @@ static void irq_work_init(struct perf_kwork *kwork,
                work->name = NULL;
        } else {
                work->id = perf_sample__intval(sample, "irq");
-               work->name = perf_sample__strval(sample, "name");
+               work->name = strdup(perf_sample__strval(sample, "name") ?: "<unknown>");
        }
 }
 
 static void irq_work_name(struct kwork_work *work, char *buf, int len)
 {
-       snprintf(buf, len, "%s:%" PRIu64 "", work->name, work->id);
+       if (work->name != NULL)
+               snprintf(buf, len, "%s:%" PRIu64 "", work->name, work->id);
+       else
+               snprintf(buf, len, "%" PRIu64 "", work->id);
 }
 
 static struct kwork_class kwork_irq = {
@@ -1135,7 +1187,10 @@ static void softirq_work_init(struct perf_kwork *kwork,
 
 static void softirq_work_name(struct kwork_work *work, char *buf, int len)
 {
-       snprintf(buf, len, "(s)%s:%" PRIu64 "", work->name, work->id);
+       if (work->name != NULL)
+               snprintf(buf, len, "(s)%s:%" PRIu64 "", work->name, work->id);
+       else
+               snprintf(buf, len, "(s)%" PRIu64 "", work->id);
 }
 
 static struct kwork_class kwork_softirq = {
@@ -1220,8 +1275,14 @@ static void workqueue_work_init(struct perf_kwork *kwork __maybe_unused,
        work->class = class;
        work->cpu = sample->cpu;
        work->id = perf_sample__intval(sample, "work");
-       work->name = function_addr == 0 ? NULL :
-               machine__resolve_kernel_addr(machine, &function_addr, &modp);
+       work->name = NULL;
+
+       if (function_addr != 0) {
+               const char *name = machine__resolve_kernel_addr(machine, &function_addr, &modp);
+
+               if (name)
+                       work->name = strdup(name);
+       }
 }
 
 static void workqueue_work_name(struct kwork_work *work, char *buf, int len)
@@ -1284,16 +1345,16 @@ static void sched_work_init(struct perf_kwork *kwork __maybe_unused,
 
        if (src_type == KWORK_TRACE_EXIT) {
                work->id = perf_sample__intval(sample, "prev_pid");
-               work->name = strdup(perf_sample__strval(sample, "prev_comm"));
+               work->name = strdup(perf_sample__strval(sample, "prev_comm") ?: "<unknown>");
        } else if (src_type == KWORK_TRACE_ENTRY) {
                work->id = perf_sample__intval(sample, "next_pid");
-               work->name = strdup(perf_sample__strval(sample, "next_comm"));
+               work->name = strdup(perf_sample__strval(sample, "next_comm") ?: "<unknown>");
        }
 }
 
 static void sched_work_name(struct kwork_work *work, char *buf, int len)
 {
-       snprintf(buf, len, "%s", work->name);
+       snprintf(buf, len, "%s", work->name ?: "");
 }
 
 static struct kwork_class kwork_sched = {
@@ -2100,8 +2161,10 @@ static void top_merge_tasks(struct perf_kwork *kwork)
                rb_erase_cached(node, &class->work_root);
                data = rb_entry(node, struct kwork_work, node);
 
-               if (!profile_name_match(kwork, data))
+               if (!profile_name_match(kwork, data)) {
+                       work_delete(data);
                        continue;
+               }
 
                cpu = data->cpu;
                merged_work = find_work_by_id(&merged_root, data->id,
@@ -2109,11 +2172,17 @@ static void top_merge_tasks(struct perf_kwork *kwork)
                if (!merged_work) {
                        work_insert(&merged_root, data, &kwork->cmp_id);
                } else {
+                       if (merged_work->name == NULL && data->name != NULL)
+                               merged_work->name = strdup(data->name);
+
                        merged_work->total_runtime += data->total_runtime;
                        merged_work->cpu_usage += data->cpu_usage;
                }
 
                top_calc_load_runtime(kwork, data);
+
+               if (merged_work)
+                       work_delete(data);
        }
 
        work_sort(kwork, class, &merged_root);
@@ -2523,6 +2592,8 @@ int cmd_kwork(int argc, const char **argv)
        } else
                usage_with_options(kwork_usage, kwork_options);
 
+       perf_kwork__exit(&kwork);
+
        /* free usage string allocated by parse_options_subcommand */
        free((void *)kwork_usage[0]);
 

diff --git a/tools/perf/util/bpf_kwork.c b/tools/perf/util/bpf_kwork.c
index d3a2e548f2b625bf405e0e5c0fa68aca7c6b35a1..2248f462a847790e1207520c46988d1c0b5d91fd 100644 (file)
--- a/tools/perf/util/bpf_kwork.c
+++ b/tools/perf/util/bpf_kwork.c
@@ -273,6 +273,7 @@ static int add_work(struct perf_kwork *kwork,
                .cpu = key->cpu,
        };
        enum kwork_class_type type = key->type;
+       int ret = 0;
 
        if (!valid_kwork_class_type(type)) {
                pr_debug("Invalid class type %d to add work\n", type);
@@ -287,8 +288,10 @@ static int add_work(struct perf_kwork *kwork,
                return -1;
 
        work = kwork->add_work(kwork, tmp.class, &tmp);
-       if (work == NULL)
-               return -1;
+       if (work == NULL) {
+               ret = -1;
+               goto out;
+       }
 
        if (kwork->report == KWORK_REPORT_RUNTIME) {
                work->nr_atoms = data->nr;
@@ -304,13 +307,16 @@ static int add_work(struct perf_kwork *kwork,
                work->max_latency_end = data->max_time_end;
        } else {
                pr_debug("Invalid bpf report type %d\n", kwork->report);
-               return -1;
+               ret = -1;
+               goto out;
        }
 
        kwork->timestart = (u64)ts_start.tv_sec * NSEC_PER_SEC + ts_start.tv_nsec;
        kwork->timeend = (u64)ts_end.tv_sec * NSEC_PER_SEC + ts_end.tv_nsec;
 
-       return 0;
+out:
+       work_exit(&tmp);
+       return ret;
 }
 
 int perf_kwork__report_read_bpf(struct perf_kwork *kwork)

diff --git a/tools/perf/util/kwork.h b/tools/perf/util/kwork.h
index abf637d447948f3a505046a93b259e3e73b92ad3..81d39a7f78c8b811e4220e0667511b94d8729049 100644 (file)
--- a/tools/perf/util/kwork.h
+++ b/tools/perf/util/kwork.h
@@ -5,6 +5,7 @@
 #include "util/tool.h"
 #include "util/time-utils.h"
 
+#include <stdlib.h>
 #include <linux/bitmap.h>
 #include <linux/list.h>
 #include <linux/rbtree.h>
@@ -164,6 +165,14 @@ struct kwork_class {
                          char *buf, int len);
 };
 
+static inline void work_exit(struct kwork_work *work)
+{
+       if (work) {
+               free(work->name);
+               work->name = NULL;
+       }
+}
+
 struct trace_kwork_handler {
        int (*raise_event)(struct perf_kwork *kwork,
                           struct kwork_class *class,