Re: [PATCH bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, May 20, 2024 at 04:47:18PM -0700, Andrii Nakryiko wrote:
> Libbpf is automatically (and transparently to user) detecting
> multi-uprobe support in the kernel, and, if supported, uses
> multi-uprobes to improve USDT attachment speed.
> 
> USDTs can be attached system-wide or for the specific process by PID. In
> the latter case, we rely on correct kernel logic of not triggering USDT
> for unrelated processes.
> 
> As such, on older kernels that do support multi-uprobes, but still have
> broken PID filtering logic, we need to fall back to singular uprobes.
> 
> Unfortunately, whether user is using PID filtering or not is known at
> the attachment time, which happens after relevant BPF programs were
> loaded into the kernel. Also unfortunately, we need to make a call
> whether to use multi-uprobes or singular uprobe for SEC("usdt") programs
> during BPF object load time, at which point we have no information about
> possible PID filtering.
> 
> The distinction between single and multi-uprobes is small, but important
> for the kernel. Multi-uprobes get BPF_TRACE_UPROBE_MULTI attach type,
> and kernel internally substitiute different implementation of some of
> BPF helpers (e.g., bpf_get_attach_cookie()) depending on whether uprobe
> is multi or singular. So, multi-uprobes and singular uprobes cannot be
> intermixed.
> 
> All the above implies that we have to make an early and conservative
> call about the use of multi-uprobes. And so this patch modifies libbpf's
> existing feature detector for multi-uprobe support to also check correct
> PID filtering. If PID filtering is not yet fixed, we fall back to
> singular uprobes for USDTs.
> 
> This extension to feature detection is simple thanks to kernel's -EINVAL
> addition for pid < 0.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/lib/bpf/features.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
> index a336786a22a3..cff8640ca66f 100644
> --- a/tools/lib/bpf/features.c
> +++ b/tools/lib/bpf/features.c
> @@ -392,11 +392,40 @@ static int probe_uprobe_multi_link(int token_fd)
>  	link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, &link_opts);
>  	err = -errno; /* close() can clobber errno */
>  
> +	if (link_fd >= 0 || err != -EBADF) {
> +		close(link_fd);
> +		close(prog_fd);
> +		return 0;
> +	}
> +
> +	/* Initial multi-uprobe support in kernel didn't handle PID filtering
> +	 * correctly (it was doing thread filtering, not process filtering).
> +	 * So now we'll detect if PID filtering logic was fixed, and, if not,
> +	 * we'll pretend multi-uprobes are not supported, if not.
> +	 * Multi-uprobes are used in USDT attachment logic, and we need to be
> +	 * conservative here, because multi-uprobe selection happens early at
> +	 * load time, while the use of PID filtering is known late at
> +	 * attachment time, at which point it's too late to undo multi-uprobe
> +	 * selection.
> +	 *
> +	 * Creating uprobe with pid == -1 for (invalid) '/' binary will fail
> +	 * early with -EINVAL on kernels with fixed PID filtering logic;
> +	 * otherwise -ESRCH would be returned if passed correct binary path
> +	 * (but we'll just get -BADF, of course).
> +	 */
> +	link_opts.uprobe_multi.pid = -1, /* invalid PID */

                                       ^ s/,/;/

so this affects just USDT load/attach, you right?

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka


> +	link_opts.uprobe_multi.path = "/"; /* invalid path */
> +	link_opts.uprobe_multi.offsets = &offset;
> +	link_opts.uprobe_multi.cnt = 1;
> +
> +	link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, &link_opts);
> +	err = -errno; /* close() can clobber errno */
> +
>  	if (link_fd >= 0)
>  		close(link_fd);
>  	close(prog_fd);
>  
> -	return link_fd < 0 && err == -EBADF;
> +	return link_fd < 0 && err == -EINVAL;
>  }
>  
>  static int probe_kern_bpf_cookie(int token_fd)
> -- 
> 2.43.0
> 
>