Re: [PATCH v7 04/23] perf kwork: Duplicate IRQ name in irq_work_init

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Apr 08, 2026 at 08:08:11AM +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 2 potential issue(s):
> - [High] Massive per-event memory leak of dynamically allocated IRQ names when updating existing `kwork_work` entries.
> - [Medium] Missing check for `strdup()` allocation failure leads to undefined behavior in `snprintf()`.
> --
> 
> commit fd542621f59f12c7e1282f77ef0817d45c3b81a0
> Author: Ian Rogers <irogers@google.com>
> 
> perf kwork: Duplicate IRQ name in irq_work_init
> 
> This commit duplicates the IRQ name string when initializing a kwork work
> item to avoid lifecycle issues with pointers into the perf event sample.
> It also updates perf_sample__strval to return a const char pointer.
> 
> > diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
> > index f600037ee4603..dceb9b79aa6ac 100644
> > --- a/tools/perf/builtin-kwork.c
> > +++ b/tools/perf/builtin-kwork.c
> > @@ -1018,7 +1018,7 @@ 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>");
> 
> Does this code leak the allocated name string when processing existing work
> items?
> 
> In event processing loops like work_push_atom() and work_pop_atom(), a
> temporary kwork_work struct is allocated on the stack for every single trace
> event. If the item is already present in the RB-tree, work_findnew() or
> work_search() returns the existing kwork_work object which already has its
> name populated.

Similarly, I think you can drop this patch unless you really want to fix
the memory problems in kwork code base properly.

Thanks,
Namhyung

> 
> Since the newly allocated name string on the temporary structure is ignored
> for existing items, does this leak the duplicated string for every repeated
> IRQ trace event?
> 
> What happens if strdup() fails and returns NULL here?
> 
> Later in irq_work_name(), work->name is passed directly to snprintf() with
> the %s format specifier:
> 
> static void irq_work_name(struct kwork_work *work, char *buf, int len)
> {
>         snprintf(buf, len, "%s:%" PRIu64 "", work->name, work->id);
> }
> 
> Could this regression cause a segmentation fault during memory pressure if
> work->name becomes NULL?
> 
> >  	}
> >  }
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260408072601.210837-1-irogers@google.com?part=4