Re: [PATCH 2/9] tools/perf: Add CONFIG_AUXTRACE support for HTM pmu on powerpc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Missing explicit inclusions of POSIX standard headers `<stdlib.h>` and `<limits.h>` for standard libc symbols.
- [Medium] Memory allocation failure during HTM auxtrace initialization is silently dropped, causing a broken recording state.

Pre-existing issues:
- [High] Directly accessing `pos->name` with `strstarts()` causes a segmentation fault when the event name is NULL.
--

commit d4675d2351298802a38f0a4941c6bb4d86a4ed1f
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add CONFIG_AUXTRACE support for HTM pmu on powerpc

The powerpc PMU collecting Hardware Trace Macro (HTM) entries makes use of
AUX support in perf infrastructure. The PMU driver has the functionality
to collect trace entries in the aux buffer. On the tools side, this data
is made available as PERF_RECORD_AUXTRACE records. This record is
generated by "perf record" command. To enable the creation of
PERF_RECORD_AUXTRACE, add functions to initialize auxtrace records ie
"htm_recording_init()". Fill in fields for other callbacks like
info_priv_size, info_fill, free, recording options etc. Add header file
to define htm pmu specific details.

> diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/powerpc/util/auxtrace.c
> index 0053526329e0e..ec84f8876a4ad 100644
> --- a/tools/perf/arch/powerpc/util/auxtrace.c
> +++ b/tools/perf/arch/powerpc/util/auxtrace.c
> @@ -31,13 +33,19 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
>  			found_vpa_dtl = 1;
>  			pos->needs_auxtrace_mmap = true;
>  			break;
> +		} else if (strstarts(pos->name, "htm")) {
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does accessing pos->name
directly cause a segmentation fault if the event name is NULL?

If a standard hardware event is parsed without an explicit name (like
"perf record -e cycles"), the name remains NULL until it is lazily generated
via evsel__name(). Evaluating strncmp(NULL, "htm", 3) inside strstarts()
would crash.

Since this patch duplicates this pattern here and in htm_recording_options(),
should we use evsel__name() or check for NULL first?

> +			found_htm = 1;
> +			pos->needs_auxtrace_mmap = true;
> +			break;
>  		}
>  	}
>  
>  	if (found_vpa_dtl)
>  		return vpa_dtl_recording_init(pos);
> +	else if (found_htm)
> +		return htm_recording_init(pos);

[Severity: Medium]
If htm_recording_init() fails to allocate memory, it returns NULL. Since we
don't update the *err parameter before returning, does this propagate a false
success to the caller?

If *err remains 0, builtin-record.c might assume auxtrace is simply not
required and continue recording, silently dropping the HTM trace data instead
of aborting with -ENOMEM.

>  	else {
> -		*err = -EINVAL;
> +		*err = 0;
>  		return NULL;
>  	}
>  }

> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
> new file mode 100644
> index 0000000000000..cc733f45ac9b0
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/htm.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HTM support
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/bitops.h>
> +#include <linux/log2.h>
> +#include <linux/string.h>
> +#include <time.h>
> +
> +#include "../../util/cpumap.h"
> +#include "../../util/evsel.h"
> +#include "../../util/evlist.h"
> +#include "../../util/session.h"
> +#include "../../util/util.h"
> +#include "../../util/pmu.h"
> +#include "../../util/debug.h"
> +#include "../../util/auxtrace.h"
> +#include "../../util/powerpc-htm.h"
> +#include "../../util/record.h"
> +#include <internal/lib.h> // page_size
> +#include <errno.h>

[Severity: Medium]
Are we missing the explicit inclusions of <stdlib.h> and <limits.h> here?

This file uses UINT_MAX in htm_recording_options() and free() in htm_free().
Without directly including <limits.h> and <stdlib.h>, building the perf tools
with musl libc might fail due to missing declarations.

[ ... ]

> +	/*
> +	 * Set auxtrace_mmap_pages to minimum
> +	 * two pages
> +	 */
> +	if (!opts->auxtrace_mmap_pages) {
> +		opts->auxtrace_mmap_pages = KiB(128) / page_size;
> +		if (opts->mmap_pages == UINT_MAX)
> +			opts->mmap_pages = KiB(256) / page_size;
> +	}
> +
> +	evlist__for_each_entry(evlist, pos) {
> +		if (strstarts(pos->name, "htm")) {

[Severity: High]
This is a pre-existing issue, but as mentioned above, this could crash if
pos->name is NULL.

> +			pos->needs_auxtrace_mmap = true;
> +			pos->core.attr.aux_watermark = opts->auxtrace_mmap_pages * (size_t)page_size;
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701084115.80383-1-atrajeev@linux.ibm.com?part=2