Re: [PATCH v4 4/7] 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: wrong lock suffix match via strtailcmp() on short names
--

commit 30b0f8cede3e3d90dc8406410f23666b838bbc6b
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(),
lock_held_shared(), lock_held_write(), and lock_held_exclusive() 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 an
LRU hash map populated by fentry/fexit hooks on lock/unlock functions.

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index 4e99af4764f1d..ab9b58f6cb713 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
[ ... ]
> @@ -166,9 +182,844 @@ 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()
misclassify short kernel lock symbol names?

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

tools/perf/util/string.c:strtailcmp() {
	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(base, suffix) does not require base to be at least as long
as suffix, a lock symbol in kallsyms that is shorter than suffix and matches
its tail will return 0:

- A lock symbol named "lock" (such as a static mutex or rwsem named lock)
  matches strtailcmp("lock", "spinlock") == 0, causing prescan_one_lock_expr()
  to only enable LOCK_PROG_SPIN instead of LOCK_PROG_ALL and
  resolve_lock_expr() to reject lock_held_read(lock) with -EINVAL.
- A short rwsem symbol such as "cpu_rwsem" or "_rwsem" matches
  strtailcmp(base, "percpu_rwsem") == 0, attaching LOCK_PROG_PERCPU_RWSEM
  instead of LOCK_PROG_RWSEM_READ/WRITE, omitting PBF_LOCK_DIRECT_RWSEM, and
  rejecting read-mode filtering with -EINVAL.
- A short mutex symbol such as "_mutex" matches
  strtailcmp(base, "rt_mutex") == 0, setting PBF_LOCK_DIRECT_RTMUTEX instead
  of PBF_LOCK_DIRECT_MUTEX.

Should this use a helper that checks strlen(base) >= strlen(suffix) before
comparing the suffix?

> +
> +		entry->part = PBF_LOCK_ADDR | mode | direct_flag;
> +		entry->target = addr;

[ ... ]

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