Re: [PATCH v6 05/11] perf bpf-filter: Add lock_held() sample filter for global mutexes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf bpf-filter: fix module symbol matching and _edata bound in find_lock_sym_cb()
--

commit 50246e2587ada8982167d48e27c24df7afceb9ec
Author: Ian Rogers <irogers@google.com>

perf bpf-filter: Add lock_held() sample filter for global mutexes

This commit adds BPF sample filter predicates such as lock_held(<lock>) to
record perf event samples only when a specified kernel lock is held or not
held. In this initial step, global and static kernel mutex symbols are
resolved via /proc/kallsyms and inspected in kernel memory via
get_lock_owner().

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index 0d3c700d3dcd5..639db6c031379 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
[ ... ]
> @@ -169,9 +178,263 @@ static int check_sample_flags(struct evsel *evsel, struct perf_bpf_filter_expr *
[ ... ]
> +struct lock_sym_cb_args {
> +	const char *name;
> +	u64 addr;
> +	bool in_text;
> +	bool in_data;
> +	bool found;
> +};
> +
> +static int find_lock_sym_cb(void *arg, const char *name, char type, u64 start)
> +{
> +	struct lock_sym_cb_args *args = arg;
> +
> +	if (!strcmp(name, "_text") || !strcmp(name, "_stext") ||
> +	    !strcmp(name, "_sinittext")) {
> +		args->in_text = true;
> +		return 0;
> +	}
> +	if (!strcmp(name, "_etext") || !strcmp(name, "_einittext")) {
> +		args->in_text = false;
> +		return 0;
> +	}
> +	if (!strcmp(name, "_sdata") || !strcmp(name, "__bss_start")) {
> +		args->in_text = false;
> +		args->in_data = true;
> +		return 0;
> +	}
> +	if (!strcmp(name, "__bss_stop")) {
> +		args->in_data = false;
> +		return 0;
> +	}
> +
> +	/*
> +	 * Ignore kernel text functions, module text functions, and
> +	 * absolute/undefined symbols. On CONFIG_LTO_CLANG kernels, vmlinux
> +	 * .data symbols between _sdata and _edata may have kallsyms type
> +	 * 'T'/'t', so allow 'T'/'t' within vmlinux data/bss (args->in_data).
> +	 */
> +	if (args->in_text || type == 'A' || type == 'a' ||
> +	    type == 'U' || type == 'u' ||
> +	    (!args->in_data && kallsyms__is_function(type)))
> +		return 0;
> +
> +	if (!strcmp(name, args->name)) {
> +		args->found = true;
> +		args->addr = start;
> +		if (start != 0)
> +			return 1;
> +	}

[Severity: Medium]
When resolving a lock symbol through the call chain:

  perf_bpf_filter__prepare()
    -> get_filter_entries()
      -> resolve_lock_expr()
        -> find_kernel_lock_sym()
          -> kallsyms__parse(..., find_lock_sym_cb)

there appear to be two symbol resolution issues in find_lock_sym_cb() in
tools/perf/util/bpf-filter.c:

First, in tools/lib/symbol/kallsyms.c, kallsyms__parse() reads each symbol
name up to the newline character:

	for (i = 0; i < KSYM_NAME_LEN; i++) {
		ch = io__get_char(&io);
		if (ch < 0 || ch == '\n')
			break;
		symbol_name[i]  = ch;
	}
	symbol_name[i]  = '\0';

In /proc/kallsyms, module symbols are formatted with a trailing module name
suffix ("\t[<module>]"). Because find_lock_sym_cb() compares names using
!strcmp(name, args->name), will lock symbols residing in loaded kernel
modules always fail to match? Other kallsyms callbacks such as
kern_sym_name_match() in tools/perf/util/auxtrace.c handle this by allowing a
tab delimiter after the symbol name:

static bool kern_sym_name_match(const char *kname, const char *name)
{
	size_t n = strlen(name);

	return !strcmp(kname, name) ||
	       (!strncmp(kname, name, n) && kname[n] == '\t');
}

Second, find_lock_sym_cb() sets args->in_data = true at both _sdata and
__bss_start, and the comment notes that .data symbols between _sdata and
_edata may have type 'T' or 't'. However, only __bss_stop resets
args->in_data to false. Should _edata also reset args->in_data = false so
that sections between _edata and __bss_start (such as .exit.text outside
_sinittext.._einittext) are not treated as vmlinux data/bss?

> +	return 0;
> +}
[ ... ]

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