On 11/1/19 12:36 AM, Alexei Starovoitov wrote:
On Fri, Nov 01, 2019 at 12:20:13AM +0100, Daniel Borkmann wrote:
On 10/30/19 11:32 PM, Alexei Starovoitov wrote:
The bpf program type raw_tp together with 'expected_attach_type'
was the most appropriate api to indicate BTF-enabled raw_tp programs.
But during development it became apparent that 'expected_attach_type'
cannot be used and new 'attach_btf_id' field had to be introduced.
Which means that the information is duplicated in two fields where
one of them is ignored.
Clean it up by introducing new program type where both
'expected_attach_type' and 'attach_btf_id' fields have
specific meaning.
Hm, just for my understanding, the expected_attach_type is unused for
tracing so far. Are you aware of anyone (bcc / bpftrace / etc) leaving
uninitialized garbage in there?
I'm not aware, but the risk is there. Better safe than sorry.
If we need to revert in the future that would be massive.
I'm already worried about new CHECK_ATTR check in raw_tp_open.
Equally unlikely user space breakage, but that one is easy to revert
whereas what you're proposing would mean revert everything.
Hmm, yeah, it's unfortunate that expected_attach_type is not enforced as
0. We sort of implicitly do for older kernels where expected_attach_type
is not known to it, but there is still non-zero risk given there seems
plenty of stuff in the wild on BPF tracing and not all might rely on libbpf.
Perhaps we should probably enforce it for all new program types, so we can
reuse it in future.
Just seems confusing that we have all
the different tracing prog types and now adding yet another one as
BPF_RPOG_TYPE_TRACING which will act as umbrella one and again have
different attach types some of which probably resemble existing tracing
prog types again (kprobes / kretprobes for example). Sounds like this
new type would implicitly deprecate all the existing types (sort of as
we're replacing them with new sub-types)?
All existing once are still supported and may grow its own helpers and what not.
Having new prog type makes things grow independently much easier.
I was thinking to call it BPF_PROG_TYPE_BTF_ENABLED or BPF_PROG_TYPE_GENERIC,
since I suspect upcoming lsm and others will fit right in,
but I think it's cleaner to define categories of bpf programs now
instead of specific purpose types like we had in the past before BTF.
Yes, otherwise we likely would have BPF_PROG_TYPE_GENERIC as last one and
would keep defining sub-types. ;)
True that k[ret]probe expects pt_regs whereas BTF enabled program context
will be the same as raw_tp as well, but couldn't this logic be hidden in
the kernel e.g. via attach_btf_id as well since this is an opt-in? Could
the fentry/fexit be described through attach_btf_id as well?
That's what I tried first, but the code grows too ugly.
Also for attaching fentry/fexit I'm adding new bpf_trace_open command
similar to bpf_raw_tp_open, since existing kprobe attach style doesn't
work at all. imo the code is much cleaner now.
Ok, fair enough that the fentry/fexit approach doesn't have too much in common
after all with plain kprobes. Applied then, thanks!
