Re: [PATCH bpf-next 4/5] libbpf: add support for sleepable kprobe and uprobe programs

[Delyan Kratunov <delyank@fb.com>]

On Thu, 2022-04-28 at 11:33 -0700, Andrii Nakryiko wrote:
> On Thu, Apr 28, 2022 at 9:54 AM Delyan Kratunov <delyank@fb.com> wrote:
> > 
> > Add section mappings for uprobe.s and kprobe.s programs. The latter
> > cannot currently attach but they're still useful to open and load in
> > order to validate that prohibition.
> > 
> 
> This patch made me realize that some changes I did few weeks ago
> hasn't landed ([0]). I'm going to rebase and resubmit and I'll ask you
> to rebase on top of those changes.
> 
>   [0] https://patchwork.kernel.org/project/netdevbpf/list/?series=630550&state=*
> 
> > Signed-off-by: Delyan Kratunov <delyank@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 9a213aaaac8a..9e89a478d40e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8692,9 +8692,12 @@ static const struct bpf_sec_def section_defs[] = {
> >         SEC_DEF("sk_reuseport/migrate", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> >         SEC_DEF("sk_reuseport",         SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> >         SEC_DEF("kprobe/",              KPROBE, 0, SEC_NONE, attach_kprobe),
> > +       SEC_DEF("kprobe.s/",            KPROBE, 0, SEC_SLEEPABLE, attach_kprobe),
> 
> but do we really have sleepable kprobes supported in the kernel? I
> don't think yet, let's not advertise as if SEC("kprobe.s") is a thing
> until we do

Fair enough, I was being lazy. I'll have the test explicitly set flags/type, it's not
that hard anyway.

> >         SEC_DEF("uprobe+",              KPROBE, 0, SEC_NONE, attach_uprobe),
> > +       SEC_DEF("uprobe.s+",            KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
> >         SEC_DEF("kretprobe/",           KPROBE, 0, SEC_NONE, attach_kprobe),
> >         SEC_DEF("uretprobe+",           KPROBE, 0, SEC_NONE, attach_uprobe),
> > +       SEC_DEF("uretprobe.s+",         KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
> >         SEC_DEF("kprobe.multi/",        KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> >         SEC_DEF("kretprobe.multi/",     KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> >         SEC_DEF("usdt+",                KPROBE, 0, SEC_NONE, attach_usdt),
> > @@ -10432,13 +10435,18 @@ static int attach_kprobe(const struct bpf_program *prog, long cookie, struct bpf
> >         const char *func_name;
> >         char *func;
> >         int n;
> > +       bool sleepable = false;
> > 
> >         opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
> > +       sleepable = str_has_pfx(prog->sec_name, "kprobe.s/");
> >         if (opts.retprobe)
> >                 func_name = prog->sec_name + sizeof("kretprobe/") - 1;
> > +       else if (sleepable)
> > +               func_name = prog->sec_name + sizeof("kprobe.s/") - 1;
> >         else
> >                 func_name = prog->sec_name + sizeof("kprobe/") - 1;
> > 
> > +
> >         n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
> >         if (n < 1) {
> >                 pr_warn("kprobe name is invalid: %s\n", func_name);
> > @@ -10957,7 +10965,7 @@ static int attach_uprobe(const struct bpf_program *prog, long cookie, struct bpf
> >                 break;
> >         case 3:
> >         case 4:
> > -               opts.retprobe = strcmp(probe_type, "uretprobe") == 0;
> > +               opts.retprobe = str_has_pfx(probe_type, "uretprobe");
> 
> it's a total nit but I'd feel a bit more comfortable with explicit
> check for "uretprobe" and "uretprobe.s" instead of a prefix match, if
> you don't mind.

Sure.

> 
> >                 if (opts.retprobe && offset != 0) {
> >                         pr_warn("prog '%s': uretprobes do not support offset specification\n",
> >                                 prog->name);
> > --
> > 2.35.1