Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andriin@fb.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
Date: Thu, 24 Oct 2019 20:39:23 -0700	[thread overview]
Message-ID: <CAEf4BzZ2SrR0CBhbXo64bwWuBq-=JDS6p6=KRaLtH1bCGBwa3w@mail.gmail.com> (raw)
In-Reply-To: <5db1f389aa6f2_5c282ada047205c012@john-XPS-13-9370.notmuch>

On Thu, Oct 24, 2019 at 11:55 AM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > On Tue, Oct 22, 2019 at 12:20 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On Mon, Oct 21, 2019 at 10:07:59PM -0700, John Fastabend wrote:
> > > > Andrii Nakryiko wrote:
> > > > > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > > > > >
> > > > > > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > > > > > kprobes programs on older kernels.
> > > > >
> > > > > My main concern with this is that this code is born bit-rotten,
> > > > > because selftests are never testing the legacy code path. How did you
> > > > > think about testing this and ensuring that this keeps working going
> > > > > forward?
> > > >
> > > > Well we use it, but I see your point and actually I even broke the retprobe
> > > > piece hastily fixing merge conflicts in this patch. When I ran tests on it
> > > > I missed running retprobe tests on the set of kernels that would hit that
> > > > code.
> > >
> > > If it also gets explicitly exposed as bpf_program__attach_legacy_kprobe() or
> > > such, it should be easy to add BPF selftests for that API to address the test
> > > coverage concern. Generally more selftests for exposed libbpf APIs is good to
> > > have anyway.
> > >
> >
> > Agree about tests. Disagree about more APIs, especially that the only
> > difference will be which underlying kernel machinery they are using to
> > set everything up. We should ideally avoid exposing that to users.
>
> Maybe a build flag to build with only the older style supported for testing?
> Then we could build, test in selftests at least. Be clear the flag is only
> for testing and can not be relied upon.

Build flag will necessitate another "flavor" of test_progs just to
test this. That seems like an overkill.

How about this approach:

$ cat silent-features.c
#include <stdio.h>

int __attribute__((weak)) __bpf_internal__force_legacy_kprobe;

int main() {
        if (__bpf_internal__force_legacy_kprobe)
                printf("LEGACY MODE!\n");
        else
                printf("FANCY NEW MODE!\n");
        return 0;
}
$ cat silent-features-testing.c
int __bpf_internal__force_legacy_kprobe = 1;
$ cc -g -O2 silent-features.c -o silent-features && ./silent-features
FANCY NEW MODE!
$ cc -g -O2 silent-features.c silent-features-testing.c -o
silent-features && ./silent-features
LEGACY MODE!

This seems like an extensible mechanism without introducing any new
public APIs or knobs, and we can control that in runtime. Some good
naming convention to emphasize this is only for testing and internal
needs, and I think it should be fine.

>
> >
> > > Cheers,
> > > Daniel