Re: [PATCH v2 2/6] perf bpf-filter: Add lock_held() sample filter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf bpf-filter: rwlock read_count leak in record_lock_acquired_owner()
- [Medium] perf bpf-filter: missing error return on node_data lookup in init_numa_data()
--

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

perf bpf-filter: Add lock_held() sample filter

This commit adds BPF sample filter predicates lock_held(<lock>),
lock_held_read(<lock>) / lock_held_shared(<lock>), and
lock_held_write(<lock>) / lock_held_exclusive(<lock>) to record perf event
samples only when a specified kernel lock is held. It evaluates lock
ownership directly in kernel memory where possible and falls back to a
held_locks BPF hash map populated by fentry/fexit hooks on kernel
lock/unlock functions.

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index 72e83ac1d6a4b..2f4c6bd85f89f 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
[ ... ]
> @@ -155,9 +171,814 @@ static int check_sample_flags(struct evsel *evsel, struct perf_bpf_filter_expr *
[ ... ]
> +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)
> +		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);
> +	}
> +}

[Severity: Medium]
What happens in init_numa_data() when find_kernel_lock_sym("node_data",
&addr) fails (for example, when /proc/kallsyms zeroes symbol addresses
under kptr_restrict for a non-root user with CAP_BPF)?

Unlike init_numa_data() in tools/perf/util/bpf_lock_contention.c, this does
not return early on failure, so skel->rodata->node_data_addr remains 0
while skel->rodata->nr_nodes is populated from devices/system/node/online.

When collect_lock_syms later calls do_collect_lock_syms() ->
do_collect_zone_lock() in tools/perf/util/bpf_skel/lock_data.h:

	} else if (nr_nodes > 0) {
		struct pglist_data **node_data = (void *)(long)node_data_addr;

		for (int i = 0; i < nr_nodes; i++) {
			struct pglist_data *pgdat = NULL;
			int err;

			err = bpf_core_read(&pgdat, sizeof(pgdat), &node_data[i]);

will this attempt to read from kernel address 0x0 and leave lock_syms
unpopulated without reporting an error for lock_held(zone_lock)?

[ ... ]
> diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c
> index e5666d4c17228..480ba4b49793d 100644
> --- a/tools/perf/util/bpf_skel/sample_filter.bpf.c
> +++ b/tools/perf/util/bpf_skel/sample_filter.bpf.c
[ ... ]
> @@ -295,4 +525,739 @@ int perf_sample_filter(void *ctx)
[ ... ]
> +static __always_inline int record_lock_acquired_owner(__u64 lock_addr, __u32 flags, __u32 owner_id)
> +{
[ ... ]
> +	existing = bpf_map_lookup_elem(&held_locks, &addr_key);
> +	if (existing) {
> +		/* Non-reader locks are not recursive; avoid double-counting */
> +		if (!is_read && existing->write_count > 0)
> +			return 0;
> +		if (is_read)
> +			__sync_fetch_and_add(&existing->read_count, 1);
> +		else
> +			__sync_fetch_and_add(&existing->write_count, 1);
> +	} else {
> +		struct held_lock_val first = {
> +			.read_count = is_read ? 1 : 0,
> +			.write_count = is_read ? 0 : 1,
> +			.flags = flags,
> +		};
> +
> +		if (bpf_map_update_elem(&held_locks, &addr_key, &first, BPF_ANY) < 0)
> +			return 0;
> +	}
> +
> +	if (class_id)
> +		update_held_lock_count(class_id, owner_id, PBF_LOCK_CLASS, flags, true);
> +	if (kmem_cache_addr) {
> +		update_held_lock_count((__u64)kmem_cache_addr, owner_id,
> +				       PBF_LOCK_SLAB, flags, true);
> +	}
> +	if (match_type)
> +		update_held_lock_count(type_flag, owner_id, PBF_LOCK_TYPE, flags, true);
> +
> +	return 0;
> +}

[Severity: Medium]
Can record_lock_acquired_owner() leak read_count in the held_locks map when
rwlocks are acquired via read_lock_irq() or read_lock_bh() on kernels with
CONFIG_GENERIC_LOCKBREAK=y and CONFIG_DEBUG_LOCK_ALLOC=n?

In kernel/locking/spinlock.c, BUILD_LOCK_OPS(read, rwlock, __acquires_shared)
implements __raw_read_lock_irq() and __raw_read_lock_bh() by calling the
noinline function _raw_read_lock_irqsave():

	static void __lockfunc __raw_##op##_lock_irq(locktype##_t *lock)	\
		lock_ctx_op(lock)						\
	{									\
		_raw_##op##_lock_irqsave(lock);					\
	}									\
										\
	static void __lockfunc __raw_##op##_lock_bh(locktype##_t *lock)		\
		lock_ctx_op(lock)						\
	{									\
		unsigned long flags;						\
		flags = _raw_##op##_lock_irqsave(lock);				\
		local_bh_disable();						\
		local_irq_restore(flags);					\
	}

Since LOCK_PROG_RWLOCK_READ attaches fexit hooks to _raw_read_lock_irq,
_raw_read_lock_bh, and _raw_read_lock_irqsave, a single read_lock_irq() or
read_lock_bh() call fires both the inner fexit_raw_read_lock_irqsave hook
and the outer fexit_raw_read_lock_irq / fexit_raw_read_lock_bh hook.

Because is_read is true for LCB_F_SPIN | LCB_F_READ, the nested-call check
(!is_read && existing->write_count > 0) is bypassed and read_count is
incremented twice (for PBF_LOCK_ADDR as well as any active PBF_LOCK_SLAB or
PBF_LOCK_TYPE entries). On release, _raw_read_unlock_irq() and
_raw_read_unlock_bh() do not call _raw_read_unlock_irqrestore(), so
record_lock_released() only runs once, leaving read_count permanently
elevated in held_locks.

[ ... ]
> +/* RWlock acquire/release hooks */
> +SEC("fexit/_raw_read_lock")
> +int BPF_PROG(fexit_raw_read_lock, void *lock)
> +{
> +	return record_lock_acquired(ctx, LCB_F_SPIN | LCB_F_READ);
> +}
> +
> +SEC("fexit/_raw_read_lock_irq")
> +int BPF_PROG(fexit_raw_read_lock_irq, void *lock)
> +{
> +	return record_lock_acquired(ctx, LCB_F_SPIN | LCB_F_READ);
> +}
> +
> +SEC("fexit/_raw_read_lock_irqsave")
> +int BPF_PROG(fexit_raw_read_lock_irqsave, void *lock, unsigned long flags)
> +{
> +	return record_lock_acquired(ctx, LCB_F_SPIN | LCB_F_READ);
> +}
> +
> +SEC("fexit/_raw_read_lock_bh")
> +int BPF_PROG(fexit_raw_read_lock_bh, void *lock)
> +{
> +	return record_lock_acquired(ctx, LCB_F_SPIN | LCB_F_READ);
> +}
[ ... ]

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