Re: [PATCH v5 5/8] perf bpf-filter: Add lock_held() sample filter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf bpf-filter: strtailcmp() misclassifies short lock symbol names
--

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

perf bpf-filter: Add lock_held() sample filter

This commit adds BPF sample filter predicates lock_held(), lock_held_read(),
and lock_held_write() (along with :R and :W suffixes) to filter perf samples
based on whether a specified kernel lock is held. It supports direct owner
inspection for mutexes, rt-mutexes, and write-locked rwsems, and uses BPF
fentry/fexit hooks with an LRU hash map for other lock families and modes.

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index ac0e7cf40f3af..d5a85ee98d5fd 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
[ ... ]
> @@ -166,9 +182,913 @@ static int check_sample_flags(struct evsel *evsel, struct perf_bpf_filter_expr *
[ ... ]
> +static void prescan_one_lock_expr(struct perf_bpf_filter_expr *expr,
> +				  bool *has_lock_filter,
> +				  unsigned int *prog_groups,
> +				  unsigned int *required_groups,
> +				  bool *needs_lock_syms,
> +				  bool *needs_slab_iter)
> +{
[ ... ]
> +	if (strtailcmp(base, "rt_mutex") == 0 || strtailcmp(base, "rtmutex") == 0 ||
> +	    strtailcmp(base, "mutex") == 0) {
> +		/* Inspected directly via lock->owner; no tracing hooks needed. */
> +		return;
> +	} else if (strtailcmp(base, "percpu_rwsem") == 0 ||
> +		   strtailcmp(base, "pcpu_rwsem") == 0) {
> +		*prog_groups |= LOCK_PROG_PERCPU_RWSEM;
> +		*required_groups |= LOCK_PROG_PERCPU_RWSEM;
> +	} else if (strtailcmp(base, "rwsem") == 0) {
> +		if (mode & PBF_LOCK_MODE_READ) {
> +			*prog_groups |= LOCK_PROG_RWSEM_READ;
> +			*required_groups |= LOCK_PROG_RWSEM_READ;
> +		}
> +		if (mode & PBF_LOCK_MODE_WRITE) {
> +			*prog_groups |= LOCK_PROG_RWSEM_WRITE | LOCK_PROG_PERCPU_RWSEM;
> +			*required_groups |= LOCK_PROG_RWSEM_WRITE;
> +		}
> +	} else if (strtailcmp(base, "spinlock") == 0) {
> +		*prog_groups |= LOCK_PROG_SPIN;
> +		*required_groups |= LOCK_PROG_SPIN;
> +	} else if (strtailcmp(base, "sem") == 0) {
> +		*prog_groups |= LOCK_PROG_SEM;
> +		*required_groups |= LOCK_PROG_SEM;
> +	} else if (strtailcmp(base, "rwlock") == 0) {
> +		if (mode & PBF_LOCK_MODE_READ)
> +			groups |= LOCK_PROG_RWLOCK_READ;
> +		if (mode & PBF_LOCK_MODE_WRITE)
> +			groups |= LOCK_PROG_RWLOCK_WRITE;
> +		*prog_groups |= groups;
> +		*required_groups |= groups;
> +	} else {
> +		*prog_groups |= LOCK_PROG_ALL;
> +	}
> +}
[ ... ]
> +static int resolve_lock_expr(struct sample_filter_bpf *skel,
> +			     struct perf_bpf_filter_expr *expr,
> +			     struct perf_bpf_filter_entry *entry)
> +{
[ ... ]
> +	ret = find_kernel_lock_sym(base, &addr);
> +	if (ret == 0) {
> +		unsigned int direct_flag = 0;
> +
> +		if (strtailcmp(base, "rt_mutex") == 0 || strtailcmp(base, "rtmutex") == 0)
> +			direct_flag = PBF_LOCK_DIRECT_RTMUTEX;
> +		else if (strtailcmp(base, "mutex") == 0)
> +			direct_flag = PBF_LOCK_DIRECT_MUTEX;
> +		else if (strtailcmp(base, "rwsem") == 0 &&
> +			 strtailcmp(base, "percpu_rwsem") != 0 &&
> +			 strtailcmp(base, "pcpu_rwsem") != 0)
> +			direct_flag = PBF_LOCK_DIRECT_RWSEM;
> +
> +		if ((direct_flag == PBF_LOCK_DIRECT_MUTEX ||
> +		     direct_flag == PBF_LOCK_DIRECT_RTMUTEX ||
> +		     strtailcmp(base, "percpu_rwsem") == 0 ||
> +		     strtailcmp(base, "pcpu_rwsem") == 0 ||
> +		     strtailcmp(base, "spinlock") == 0 ||
> +		     (strtailcmp(base, "sem") == 0 &&
> +		      strtailcmp(base, "rwsem") != 0)) &&
> +		    !(mode & PBF_LOCK_MODE_WRITE)) {
> +			pr_err("Error: Lock '%s' is exclusive and does not support read/shared mode\n",
> +			       name);
> +			return -EINVAL;
> +		}

[Severity: Medium]
Can strtailcmp() in prescan_one_lock_expr() and resolve_lock_expr() in
tools/perf/util/bpf-filter.c misclassify lock symbol names that are shorter
than the compared suffix?

In tools/perf/util/string.c, strtailcmp() compares characters backward from
the end of both strings and returns 0 as soon as either string reaches the
beginning:

int strtailcmp(const char *s1, const char *s2)
{
	int i1 = strlen(s1);
	int i2 = strlen(s2);
	while (--i1 >= 0 && --i2 >= 0) {
		if (s1[i1] != s2[i2])
			return s1[i1] - s2[i2];
	}
	return 0;
}

Because strtailcmp() is symmetric, it returns 0 when base is shorter than the
tested suffix and matches the tail of that suffix:

1. Passing a lock symbol named "lock" (such as the static mutexes named lock
   in kernel/ksysfs.c or mm/folio.c) to lock_held(lock) or
   lock_held_read(lock) matches strtailcmp("lock", "spinlock") == 0. Instead
   of falling through to LOCK_PROG_ALL, prescan_one_lock_expr() only enables
   LOCK_PROG_SPIN, and resolve_lock_expr() treats it as an exclusive spinlock
   and rejects read mode with -EINVAL.

2. Any rw_semaphore symbol shorter than 12 characters whose name is a suffix
   of "percpu_rwsem" or "pcpu_rwsem" (such as "cpu_rwsem" or "u_rwsem")
   matches strtailcmp(base, "percpu_rwsem") == 0, causing
   prescan_one_lock_expr() to only enable LOCK_PROG_PERCPU_RWSEM and
   resolve_lock_expr() to omit PBF_LOCK_DIRECT_RWSEM and reject read mode
   with -EINVAL.

3. Any mutex symbol named "_mutex" or "t_mutex" matches
   strtailcmp(base, "rt_mutex") == 0, causing resolve_lock_expr() to set
   PBF_LOCK_DIRECT_RTMUTEX instead of PBF_LOCK_DIRECT_MUTEX.

Would it be better to use str_ends_with() from linux/string.h here so that
base is required to be at least as long as the suffix?

[ ... ]

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