Re: [PATCH v2 bpf-next 1/2] bpf: replace prog_raw_tp+btf_id with prog_tracing

On Fri, Nov 01, 2019 at 12:20:13AM +0100, Daniel Borkmann wrote:

On 10/30/19 11:32 PM, Alexei Starovoitov wrote:

quoted

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.

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.

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.
I guess it's hard to judge, since you haven't seen my first version
hacking into existing types that I quickly discarded due to messy and
hard to read code logic.