Re: [PATCH v6 07/11] perf bpf-filter: Support per-process and semi-global locks in lock_held()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf bpf-filter: unhandled zone_lock resolution errors in init_numa_data()
--

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

perf bpf-filter: Support per-process and semi-global locks in lock_held()

This commit extends the lock_held() BPF sample filter to support the
per-process locks mmap_lock and siglock as well as the semi-global locks
rq_lock and zone_lock. For rq_lock and zone_lock, it populates the lock_syms
BPF map at filter setup time via a raw_tp/bpf_test_finish program.

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index 8797f67698f7d..f542920f914fc 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
> @@ -534,6 +534,54 @@ static bool parse_lock_hex_addr(const char *str, u64 *addr)
>  	return true;
>  }
>  
> +static void init_numa_data(struct sample_filter_bpf *skel, struct btf *btf)
> +{
> +	u64 addr = 0;
> +	char *buf = NULL, *p;
> +	size_t len;
> +	long last = -1;
> +	int ret;
> +
> +	if (!btf)
> +		return;
> +
> +	ret = btf__find_by_name_kind(btf, "zone", BTF_KIND_STRUCT);
> +	if (ret < 0)
> +		return;
> +
> +	ret = btf__resolve_size(btf, ret);
> +	if (ret < 0)
> +		return;
> +
> +	skel->rodata->sizeof_zone = ret;
> +
> +	if (find_kernel_lock_sym("contig_page_data", &addr) == 0) {
> +		skel->rodata->contig_page_data_addr = addr;
> +		return;
> +	}
> +
> +	if (find_kernel_lock_sym("node_data", &addr) != 0)
> +		return;
> +
> +	skel->rodata->node_data_addr = addr;
> +
> +	if (sysfs__read_str("devices/system/node/online", &buf, &len) >= 0) {
> +		p = buf;
> +		while (p && *p) {
> +			char *end;
> +
> +			last = strtol(p, &end, 0);
> +			if (end == p)
> +				break;
> +			p = end;
> +			if (*p == ',' || *p == '-' || *p == '\n')
> +				p++;
> +		}
> +		skel->rodata->nr_nodes = last + 1;
> +		free(buf);
> +	}
> +}
> +
>  static bool btf_has_func(struct btf *btf, const char *name)

[ ... ]

> @@ -657,7 +707,8 @@ static int configure_lock_progs(struct sample_filter_bpf *skel, struct btf *btf,
>  static void prescan_one_lock_expr(struct perf_bpf_filter_expr *expr,
>  				  bool *has_lock_filter,
>  				  unsigned int *prog_groups,
> -				  unsigned int *required_groups)
> +				  unsigned int *required_groups,
> +				  bool *needs_lock_syms)
>  {
>  	char base[128];
>  	bool has_amp;
> @@ -672,6 +723,29 @@ static void prescan_one_lock_expr(struct perf_bpf_filter_expr *expr,
>  				 sizeof(base), &has_amp, &mode) < 0 || has_amp)
>  		return;
>  
> +	if (!strcmp(base, "mmap_lock") || !strcmp(base, "mmap_sem")) {
> +		/*
> +		 * Write ownership of mm->mmap_lock is inspected directly via
> +		 * is_rwsem_writer_owner() without tracing hooks; only reader
> +		 * tracking requires LOCK_PROG_RWSEM_READ.
> +		 */
> +		if (mode & PBF_LOCK_MODE_READ) {
> +			*prog_groups |= LOCK_PROG_RWSEM_READ;
> +			*required_groups |= LOCK_PROG_RWSEM_READ;
> +		}
> +		return;
> +	}
> +	if (!strcmp(base, "siglock")) {
> +		*prog_groups |= LOCK_PROG_SPIN;
> +		*required_groups |= LOCK_PROG_SPIN;
> +		return;
> +	}
> +	if (!strcmp(base, "rq_lock") || !strcmp(base, "zone_lock")) {
> +		*prog_groups |= LOCK_PROG_SPIN;
> +		*required_groups |= LOCK_PROG_SPIN;
> +		*needs_lock_syms = true;
> +		return;
> +	}

[ ... ]

> @@ -853,6 +932,53 @@ static int resolve_lock_expr(struct sample_filter_bpf *skel, struct btf *btf,

[ ... ]

> +	if (!strcmp(base, "zone_lock")) {
> +		if (!(mode & PBF_LOCK_MODE_WRITE)) {
> +			pr_err("Error: Lock '%s' is exclusive and does not support read/shared mode\n",
> +			       name);
> +			return -EINVAL;
> +		}
> +		entry->part = PBF_LOCK_CLASS | mode;
> +		entry->value = LOCK_CLASS_ZONE_LOCK;
> +		if (skel)
> +			skel->bss->has_lock_class = 1;
> +		return 0;
> +	}

[ ... ]

> @@ -1280,12 +1406,16 @@ int perf_bpf_filter__prepare(struct evsel *evsel, struct target *target)
>  		btf = btf__load_vmlinux_btf();
>  		bpf_map__set_max_entries(skel->maps.held_locks, MAX_ENTRIES);
>  		bpf_map__set_max_entries(skel->maps.lock_addr_filter, MAX_FILTERS);
> +		if (needs_lock_syms) {
> +			bpf_map__set_max_entries(skel->maps.lock_syms, MAX_ENTRIES);
> +			init_numa_data(skel, btf);
> +		}

[Severity: Medium]
In perf_bpf_filter__prepare() -> prescan_lock_filters() ->
prescan_one_lock_expr(), both rq_lock and zone_lock set *needs_lock_syms =
true, and perf_bpf_filter__prepare() unconditionally calls
init_numa_data(skel, btf) whenever needs_lock_syms is set.

When a user only specifies lock_held(rq_lock), which resolves runqueues via
BPF BTF ksym and does not use kallsyms or NUMA zone data, init_numa_data()
still queries kallsyms for contig_page_data and node_data. If /proc/kallsyms
addresses are restricted (for instance under kptr_restrict=2 or when running
with CAP_BPF without CAP_SYSLOG), find_kernel_lock_sym() prints spurious
permission error messages for NUMA symbols that rq_lock does not use. Also,
on a UMA kernel where contig_page_data exists in /proc/kallsyms with address
0, find_kernel_lock_sym("contig_page_data", &addr) prints an error and
returns -EACCES, and init_numa_data() then falls through to query node_data
as well.

Conversely, when a user specifies lock_held(zone_lock), init_numa_data()
returns void and silently ignores failures if BTF lookup or size resolution
for struct zone fails, if both find_kernel_lock_sym("contig_page_data",
&addr) and find_kernel_lock_sym("node_data", &addr) fail, or if
sysfs__read_str("devices/system/node/online", ...) fails (leaving
skel->rodata->nr_nodes as 0).

In resolve_lock_expr(), the zone_lock branch then succeeds without checking
whether skel->rodata->contig_page_data_addr or skel->rodata->node_data_addr
(with skel->rodata->nr_nodes > 0) was populated. As a result,
collect_lock_syms() runs do_collect_zone_lock() with zeroed addresses,
populates zero LOCK_CLASS_ZONE_LOCK entries in lock_syms, and attaches
kernel-wide spinlock fentry/fexit programs for a filter that can never match
any zone_lock.

Should init_numa_data() only be invoked when zone_lock is used, and should
failures to resolve the NUMA zone parameters be propagated so that
perf_bpf_filter__prepare() aborts instead of attaching a non-functional
zone_lock filter?

[ ... ]

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