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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191031233642.xnqlz6qjfwzlmilt@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 31 Oct 2019 16:36:43 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net,
        netdev@...r.kernel.org, bpf@...r.kernel.org, kernel-team@...com
Subject: 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:
> > 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.