Re: [PATCH] perf lock contention: Allow 'mmap_lock' in -L/--lock-filter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Combining `mmap_lock` with other lock names in the `-L/--lock-filter` option drops all events.
- [High] Specifying only an unknown lock symbol in `-L/--lock-filter` silently traces all locks in the system instead of aborting.
--

commit ba21190eae512a9060154fb29ca23ce62c636cc2
Author: Namhyung Kim <namhyung@kernel.org>
Subject: perf lock contention: Allow 'mmap_lock' in -L/--lock-filter

This commit adds support for filtering lock contention events specifically for
the 'mmap_lock' using the -L/--lock-filter option. It introduces a special
flag to bypass the usual address-based filtering for mmap_lock.

> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index cbd7435579fe..cd7ee5d1d1dd 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c

[ ... ]

> @@ -264,7 +270,10 @@ int lock_contention_prepare(struct lock_contention *con)
>  			con->filters->addrs = addrs;
>  		}
>  		naddrs = con->filters->nr_addrs;
> -		skel->rodata->has_addr = 1;
> +		if (naddrs > 0)
> +			skel->rodata->has_addr = 1;
> +		else
> +			naddrs = 1;
>  	}

Prior to this patch, providing an unknown symbol (e.g., -L non_existent)
resulted in naddrs = 0 and has_addr = 1, which correctly caused libbpf to
fail creating the map with 0 entries, gracefully aborting the process.

With this change, if naddrs is 0, has_addr remains 0 but naddrs is
artificially set to 1. The BPF map creation succeeds, but since has_addr
and has_mmap_lock are both 0, the BPF program skips the address filter
entirely.

Does this unintentionally trace all locks in the system instead of gracefully
aborting when an unknown lock name is provided?

> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 96e7d853b9ed..45ec2fb73984 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c

[ ... ]

> @@ -295,6 +298,14 @@ static inline int can_record(u64 *ctx)
>  			return 0;
>  	}
>  
> +	if (has_mmap_lock) {
> +		__u64 lock = ctx[0];
> +		__u32 flag = ctx[1];
> +
> +		if (check_lock_type(lock, flag) != LCD_F_MMAP_LOCK)
> +			return 0;
> +	}
> +
>  	return 1;
>  }

The -L option allows filtering by multiple lock names, but can_record()
evaluates active filters sequentially. If a user specifies
-L mmap_lock,other_lock, both has_mmap_lock = 1 and has_addr = 1 will be
set in the BPF rodata.

In this scenario:

1. An other_lock event passes the has_addr check but then reaches the
has_mmap_lock check. Since its type is not LCD_F_MMAP_LOCK, the function
returns 0.

2. An mmap_lock event is evaluated against the has_addr check first.
Because its dynamic address isn't in addr_filter, the lookup fails, and the
function returns 0 before even reaching the has_mmap_lock check.

Will this cause all valid events to be silently dropped when combining
mmap_lock with other lock names?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504060859.84987-1-namhyung@kernel.org?part=1