Re: [PATCH bpf-next 06/29] bpf: Add bpf_arg/bpf_ret_value helpers for tracing programs

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>
Subject: Re: [PATCH bpf-next 06/29] bpf: Add bpf_arg/bpf_ret_value helpers for tracing programs
Date: Wed, 1 Dec 2021 09:59:57 -0800	[thread overview]
Message-ID: <CAEf4BzabQ9YU=d-F0ypA6W73YD534cAb2SkAkwYuyD6dk71LSQ@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQ+6iMkRh3YLjJpyoLtqgzU2Fwhdhbv3ue7ObWWoZTmFmw@mail.gmail.com>

On Wed, Dec 1, 2021 at 9:37 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 30, 2021 at 11:13 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > Hm... I'd actually try to keep kprobe BTF-free. We have fentry for
> > cases where BTF is present and the function is simple enough (like <=6
> > args, etc). Kprobe is an escape hatch mechanism when all the BTF
> > fanciness just gets in the way (retsnoop being a primary example from
> > my side). What I meant here was that bpf_get_arg(int n) would read
> > correct fields from pt_regs that map to first N arguments passed in
> > the registers. What we currently have with PT_REGS_PARM macros in
> > bpf_tracing.h, but with a proper unified BPF helper.
>
> and these macros are arch specific.
> which means that it won't be a trivial patch to add bpf_get_arg()
> support for kprobes.

no one suggested it would be trivial :) things worth doing are usually
non-trivial, as can be evidenced by Jiri's patch set

> Plenty of things to consider. Like should it return an error
> at run-time or verification time when a particular arch is not supported.

See my other replies to Jiri, I'm more and more convinced that dynamic
is the way to go for things like this, where the safety of the kernel
or BPF program are not compromised.

But you emphasized an important point, that it's probably good to
allow users to distinguish errors from reading actual value 0. There
are and will be situations where argument isn't available or some
combination of conditions are not supported. So I think, while it's a
bit more verbose, these forms are generally better:

int bpf_get_func_arg(int n, u64 *value);
int bpf_get_func_ret(u64 *value);

WDYT?

> Or argument 6 might be available on one arch, but not on the other.
> 32-bit CPU regs vs 64-bit regs of BPF, etc.
> I wouldn't attempt to mix this work with current patches.

Oh, I didn't suggest doing it as part of this already huge and
complicated set. But I think it's good to think a bit ahead and design
the helper API appropriately, at the very least.

And again, I think bpf_get_func_arg/bpf_get_func_ret deserve their own
patch set where we can discuss all this independently from
multi-attach.