On Mon, Jan 8, 2024 at 3:45 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> >
> > Not artificially, but only when pt_regs in bpf prog doesn't match
> > what kernel is passing.
> > I think allowing only:
> > handle_event_user(void *ctx __arg_ctx)
> > and prog will cast it to pt_regs immediately is less surprising
> > and proper C code,
> > but
> > handle_event_user(struct pt_regs *ctx __arg_ctx)
> > is also ok when pt_regs is indeed what is being passed.
> > Which will be the case for x86.
> > And will be fine on arm64 too, because
> > arch/arm64/include/asm/ptrace.h
> > struct pt_regs {
> > union {
> > struct user_pt_regs user_regs;
> >
> > but if arm64 ever changes that layout we should start failing to load.
>
> Ok, I'm glad you agreed to allow `struct pt_regs *`. I also will say
> that (as it stands right now) passing `struct pt_regs *` is valid on
> all architectures, because that's what kernel passes around internally
> as context for uprobe, kprobe, and kprobe-multi. See uprobe_prog_run,
> kprobe_multi_link_prog_run, and perf_trace_run_bpf_submit, we always
> pass real `struct pt_regs *`.
Right, but for perf event progs it's actually bpf_user_pt_regs_t:
ctx.regs = perf_arch_bpf_user_pt_regs(regs);
bpf_prog_run(prog, &ctx);
yet all such progs are written assuming struct pt_regs
which is not correct.
It's a bit of a mess while strict type checking should make it better.
BPF is a strictly typed assembly language and the verifier
should not be violating its own promises of type checking when
it sees arg_ctx.
The reason I was proposing to restrict both kernel and libbpf
to 'void *ctx __arg_ctx' is because it's trivial to implement
in both.
To allow 'struct correct_type *ctx __arg_ctx' generically is much more
work.
> So, I'll add kprobe/multi-kprobe special handling to allows `struct
> pt_regs *` then, ok?
If you mean to allow 'void *ctx __arg_ctx' in kernel and libbpf and
in addition allow 'struct pt_reg *ctx __arg_ctx' for kprobe and other
prog types where that's what is being passed then yes.
Totally fine with me.
These two are easy to enforce in kernel and libbpf.
> Yes, of course, sk_buff instead of pt_regs is definitely broken. But
> that will be detected even by the compiler.
Right. C can do casts, but in bpf asm the verifier promises strict type
checking and it goes further and makes safety decisions based on types.
> Anyways, I can add special casing for pt_regs and start enforcing
> types. A bit hesitant to do that on libbpf side, still, due to that
> eager global func behavior, which deviates from kernel, but if you
> insist I'll do it.
I don't understand this part.
Both kernel and libbpf can check
if (btf_type_id(ctx) == 'struct pt_regs'
&& prog_type == kprobe) allow such __arg_ctx.
>
> (Eduard, I'll add feature detection for the need to rewrite BTF at the
> same time, just FYI)
>
> Keep in mind, though, for non-global subprogs kernel doesn't enforce
> any types, so you can really pass sk_buff into pt_regs argument, if
> you really want to, but kernel will happily still assume PTR_TO_CTX
> (and I'm sure you know this as well, so this is mostly for others and
> for completeness).
static functions are very different. They're typeless and will stay
typeless for some time.
Compiler can do whatever it wants with them.
Like in katran case the static function of 6 arguments is optimized
into 5 args. The types are unknown. The compiler can specialize
args with constant, partially inline, etc.
Even if it kept types of args after heavy transformations
the verifier cannot rely on that for safety or enforce strict types (yet).
static foo() is like static inline foo().
kprobe-ing into static func is questionable.
Only if case of global __weak the types are dependable and that's why
the verifier treats them differently.
Hopefully the -Overifiable llvm/gcc proposal will keep moving.
Then, one day, we can potentially disable some of the transformations
on static functions that makes types useless. Then the verifier
will be able to verify them just as globals and enforce strict types.
