Re: [PATCH bpf-next 2/6] bpf: Add support to specify uprobe_multi target via file descriptor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing strict zero-validation for the unused `path_fd` field when the `BPF_F_UPROBE_MULTI_PATH_FD` flag is not set.
- [Low] Mixing scope-based cleanup (`CLASS()`) with `goto`-based error handling in `bpf_uprobe_multi_link_attach`.
--

commit f7958ff1ce57c1ab456d3f134881a0081a58b8ed
Author: Jiri Olsa <jolsa@kernel.org>

bpf: Add support to specify uprobe_multi target via file descriptor

This commit adds the BPF_F_UPROBE_MULTI_PATH_FD flag to allow uprobe_multi
links to specify the target binary using an already opened file descriptor
instead of a path string.

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 970ce7bbf99e1..4965006d58ee6 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
[ ... ]
> @@ -3240,29 +3241,51 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
[ ... ]
>  	uref_ctr_offsets = u64_to_user_ptr(attr->link_create.uprobe_multi.ref_ctr_offsets);
>  	ucookies = u64_to_user_ptr(attr->link_create.uprobe_multi.cookies);
> +	upath = u64_to_user_ptr(attr->link_create.uprobe_multi.path);
>  
> -	err = user_path_at(AT_FDCWD, upath, LOOKUP_FOLLOW, &path);
> -	if (err)
> -		return err;
> +	if (flags & BPF_F_UPROBE_MULTI_PATH_FD) {
> +		/*
> +		 * When BPF_F_UPROBE_MULTI_PATH_FD is set, the executable is identified
> +		 * by path_fd, and upath must be NULL.
> +		 */
> +		if (upath)
> +			return -EINVAL;
> +
> +		CLASS(fd, f)(attr->link_create.uprobe_multi.path_fd);

[Severity: Low]
Does this code violate the cleanup subsystem guidelines by mixing scope-based
cleanup with goto-based error handling?

The bpf_uprobe_multi_link_attach() function relies heavily on goto labels
for error paths (e.g., error_path_put, error_free, error_unregister). The
kernel guidelines state that goto-based cleanup and scope-based cleanup
helpers (like CLASS) should never be mixed in the same function.

Should this function be fully converted to scope-based cleanup, or should it
use explicit fdget()/fdput() without CLASS() instead?

> +		if (fd_empty(f))
> +			return -EBADF;
> +		path = fd_file(f)->f_path;
> +		path_get(&path);
> +	} else {
> +		/*
> +		 * When BPF_F_UPROBE_MULTI_PATH_FD is not set, the path is resolved
> +		 * relative to the cwd (AT_FDCWD) or absolute  using the upath string.
> +		 */
> +		if (!upath)
> +			return -EINVAL;

[Severity: Medium]
Should there be strict zero-validation for the unused path_fd field here?

When the BPF_F_UPROBE_MULTI_PATH_FD flag is not set, this branch is taken
and attr->link_create.uprobe_multi.path_fd is ignored without being
validated as zero.

If userspace passes uninitialized garbage in this field, it won't be
rejected. Could this break forward compatibility if path_fd needs to be
assigned a new meaning in the future?

> +
> +		err = user_path_at(AT_FDCWD, upath, LOOKUP_FOLLOW, &path);
> +		if (err)
> +			return err;
> +	}

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