Re: [PATCH v6 06/11] perf bpf-filter: Extend lock_held() to other kernel lock types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf bpf-filter: prescan_one_lock_expr() misses percpu_rw_semaphore symbols
- [Medium] perf bpf-filter: '*_sem' rw_semaphore and percpu_rw_semaphore symbols misclassified as counting semaphores
--

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

perf bpf-filter: Extend lock_held() to other kernel lock types

This commit extends the lock_held() BPF sample filter to support additional
kernel lock types, lock type flags, and hexadecimal lock addresses. It uses
direct owner inspection where possible and attaches fentry/fexit BPF hooks
to track lock acquisitions and releases in an LRU hash map otherwise.

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index 639db6c031379..8797f67698f7d 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
[ ... ]
> @@ -301,26 +517,225 @@ static int find_kernel_lock_sym(const char *name, u64 *addr)
[ ... ]
>  static void prescan_one_lock_expr(struct perf_bpf_filter_expr *expr,
> -				  bool *has_lock_filter)
> +				  bool *has_lock_filter,
> +				  unsigned int *prog_groups,
> +				  unsigned int *required_groups)
>  {
> +	char base[128];
> +	bool has_amp;
> +	unsigned int mode, groups = 0;
> +	u64 dummy;
> +
>  	if (expr->term != PBF_TERM_LOCK_HELD || !expr->lock_name)
>  		return;
>  
>  	*has_lock_filter = true;
> +	if (split_lock_name_mode(expr->lock_name, expr->part, base,
> +				 sizeof(base), &has_amp, &mode) < 0 || has_amp)
> +		return;
> +
> +	if (parse_lock_type_name(base, mode, NULL, NULL, &groups)) {
> +		*prog_groups |= groups;
> +		*required_groups |= groups;
> +		return;
> +	}
> +
> +	if (parse_lock_hex_addr(base, &dummy)) {
> +		*prog_groups |= LOCK_PROG_ALL;
> +		return;
> +	}
> +
> +	if (str_ends_with(base, "rt_mutex") || str_ends_with(base, "rtmutex") ||
> +	    str_ends_with(base, "mutex")) {
> +		/* Inspected directly via lock->owner; no tracing hooks needed. */
> +		return;
> +	} else if (str_ends_with(base, "percpu_rwsem") ||
> +		   str_ends_with(base, "pcpu_rwsem")) {
> +		*prog_groups |= LOCK_PROG_PERCPU_RWSEM;
> +		*required_groups |= LOCK_PROG_PERCPU_RWSEM;
> +	} else if (str_ends_with(base, "rwsem")) {
> +		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 (str_ends_with(base, "spinlock")) {
> +		*prog_groups |= LOCK_PROG_SPIN;
> +		*required_groups |= LOCK_PROG_SPIN;
> +	} else if (str_ends_with(base, "sem")) {
> +		*prog_groups |= LOCK_PROG_SEM;
> +		*required_groups |= LOCK_PROG_SEM;
> +	} else if (str_ends_with(base, "rwlock")) {
> +		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;
> +	}
>  }

[Severity: Medium]
In perf_bpf_filter__prepare(), prescan_lock_filters() calls
prescan_one_lock_expr() before btf__load_vmlinux_btf() is called:

perf_bpf_filter__prepare()
  prescan_lock_filters()
    prescan_one_lock_expr()
  ...
  btf = btf__load_vmlinux_btf();
  configure_lock_progs(skel, btf, prog_groups, required_groups, ...);
  sample_filter_bpf__load(skel);
  get_filter_entries(evsel, skel, btf, entry)
    resolve_lock_expr(skel, btf, ...)
      is_percpu_rwsem_sym(btf, base)

Because prescan_one_lock_expr() only checks whether base ends with
"percpu_rwsem" or "pcpu_rwsem" without calling is_percpu_rwsem_sym(),
won't percpu_rw_semaphore symbols with other suffixes be misclassified
during prescan?

For example, when filtering on dup_mmap_sem or bp_cpuinfo_sem,
prescan_one_lock_expr() matches str_ends_with(base, "sem") and only sets
LOCK_PROG_SEM. Then configure_lock_progs() disables autoload for
fexit_percpu_down_write and fentry_percpu_up_write before the skeleton is
loaded. When resolve_lock_expr() later runs and identifies the symbol via
is_percpu_rwsem_sym(btf, base), the LOCK_PROG_PERCPU_RWSEM programs were
never loaded or attached.

Similarly, for percpu_rw_semaphore symbols ending in "_rwsem" (such as
cgroup_threadgroup_rwsem) or "_lock" (such as cpu_hotplug_lock and
mem_hotplug_lock), wouldn't prescan_one_lock_expr() enable unnecessary
LOCK_PROG_RWSEM or LOCK_PROG_ALL hooks across other lock families?

> @@ -390,14 +805,42 @@ static int normalize_lock_expr_op(const struct perf_bpf_filter_expr *expr,
>  	return -EINVAL;
>  }
>  
> -static int resolve_lock_expr(struct perf_bpf_filter_expr *expr,
> +static bool is_percpu_rwsem_sym(struct btf *btf, const char *name)
> +{
> +	char rc_name[160];
> +
> +	if (str_ends_with(name, "percpu_rwsem") ||
> +	    str_ends_with(name, "pcpu_rwsem"))
> +		return true;
> +
> +	if (!btf)
> +		return false;
> +
> +	snprintf(rc_name, sizeof(rc_name), "__percpu_rwsem_rc_%s", name);
> +	return btf__find_by_name_kind(btf, rc_name, BTF_KIND_VAR) > 0;
> +}
[ ... ]
> @@ -410,29 +853,79 @@ static int resolve_lock_expr(struct perf_bpf_filter_expr *expr,
[ ... ]
> +	ret = find_kernel_lock_sym(base, &addr);
> +	if (ret == 0) {
> +		unsigned int direct_flag = 0;
> +		bool is_pcpu_rwsem = is_percpu_rwsem_sym(btf, base);
> +
> +		if (str_ends_with(base, "rt_mutex") || str_ends_with(base, "rtmutex"))
> +			direct_flag = PBF_LOCK_DIRECT_RTMUTEX;
> +		else if (str_ends_with(base, "mutex"))
> +			direct_flag = PBF_LOCK_DIRECT_MUTEX;
> +		else if (str_ends_with(base, "rwsem") && !is_pcpu_rwsem)
> +			direct_flag = PBF_LOCK_DIRECT_RWSEM;
> +
> +		if ((direct_flag == PBF_LOCK_DIRECT_MUTEX ||
> +		     direct_flag == PBF_LOCK_DIRECT_RTMUTEX ||
> +		     is_pcpu_rwsem ||
> +		     str_ends_with(base, "spinlock") ||
> +		     (str_ends_with(base, "sem") &&
> +		      !str_ends_with(base, "rwsem"))) &&
> +		    !(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]
How are global or static rw_semaphore locks whose names end in "_sem"
(such as namespace_sem, uts_sem, pci_bus_sem, or dev_addr_sem, defined
with DECLARE_RWSEM) handled here and in prescan_one_lock_expr()?

Because both functions classify any symbol ending in "sem" (and not
"rwsem") as a counting semaphore:

1. In resolve_lock_expr(), read-mode filters such as
   lock_held_read(namespace_sem) or lock_held(uts_sem:R) hit the
   str_ends_with(base, "sem") && !str_ends_with(base, "rwsem") check
   when !(mode & PBF_LOCK_MODE_WRITE) and are rejected with -EINVAL.

2. For write-mode or any-mode filters such as lock_held(namespace_sem)
   or lock_held_write(uts_sem), prescan_one_lock_expr() only enables
   LOCK_PROG_SEM (down/up) and resolve_lock_expr() leaves direct_flag as 0,
   so neither PBF_LOCK_DIRECT_RWSEM nor the down_read/down_write hooks are
   used.

Could BTF type inspection on the symbol (or checking whether the BTF var
type is struct rw_semaphore vs struct semaphore) be used to distinguish
rw_semaphore symbols ending in "_sem" from counting semaphores?

> +
> +		entry->part = PBF_LOCK_ADDR | mode | direct_flag;
> +		entry->value = addr;
[ ... ]

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