Re: [PATCH v6 06/25] perf evsel: Refactor evsel tracepoint sample accessors perf_sample

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Apr 06, 2026 at 08:24:31AM -0700, Ian Rogers wrote:
> On Sun, Apr 5, 2026 at 11:06 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, Apr 03, 2026 at 08:43:06PM -0700, Ian Rogers wrote:
> > > The evsel argument to evsel__intval, evsel__rawptr, and similar
> > > functions, is unnecessary as it can be read from the sample. Remove
> > > the evsel and rename the function to match that the data is coming
> > > from the sample.
> > >
> > > Add bounds checks to a number read values from review feedback. Make
> > > perf_sample__strval avoid returning NULL pointers, return an empty
> > > string instead. Fix the function type to reflect this, catching a bug
> > > in kwork where the string wasn't being duplicated.
> >
> > I'm not sure if it's a bug... the string was never freed and duplicating
> > it would create a lot of memory leaks.
> >
> > Also it seems you still check the return value of perf_sample__strval()
> > if it's NULL.
> 
> Yeah, this is one of the areas where working with Sashiko is
> difficult. The kwork code has memory handling issues, and each patch
> series has revealed new warnings. I believe the issues are fixed in
> v6, but as you point out, I haven't addressed the pre-existing memory
> leaks. I don't think we need to overcomplicate fixing the evsel in
> perf_sample issue this series is addressing.

I thought this patch introduced memory leaks by adding strdup() and no
free().  It looks like there is a pre-existing memory issue in kwork,
then you can leave it and just convert the function not to pass evsel.

> 
> Specifically the current code will assign to struct perf_kwork name
> both a value within a sample:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-kwork.c?h=perf-tools-next#n1021
> ```
> work->name = evsel__strval(evsel, sample, "name");
> ```
> 
> Or a strdup-ed value:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-kwork.c?h=perf-tools-next#n1152
> ```
> work->name = evsel__softirq_name(evsel, num);
> ```
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-kwork.c?h=perf-tools-next#n1125
> ```
> static char *evsel__softirq_name(struct evsel *evsel, u64 num)
> ...
> name = strdup(sym->str);
> ...
> return name;
> ```
> 
> The name's lifetime must match that of a sample, which seems
> ridiculous given the ring buffer... will be overwritten. So in this
> change, based on the Sashiko feedback, I've strdup-ed the names to
> avoid the name pointing to memory within a sample that will change
> when the typically stack allocated value disappears. Sashiko
> originally complained about a lack of bounds checking, but then
> complained about strdups of NULL, etc. At some point I have to stop
> addressing every issue within the codebase that Sashiko raises, and
> kwork seems to generate more of those than elsewhere.

Agreed, you don't need to fix all the pre-existing bugs.

> 
> I think it is pretty obvious the kwork memory handling at least needs
> a free function for struct perf_kwork, this can then also free the
> name, but refactoring and fixing kwork wasn't the point of this patch
> series. We've just merged the tests I sent out to provide minimal
> kwork coverage.

Right, let's not make it worse and leave it fixed separately.

Thanks,
Namhyung