diff mbox series

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

Message ID aac0c6adae881f57c247d7bf35e3047f7bf6cfe0.1651103126.git.delyank@fb.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series sleepable uprobe support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/apply fail Patch does not apply to bpf-next
bpf/vmtest-bpf-next-PR fail merge-conflict

Commit Message

Delyan Kratunov April 28, 2022, 4:53 p.m. UTC 
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.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/lib/bpf/libbpf.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Andrii Nakryiko April 28, 2022, 6:33 p.m. UTC | #1 
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

>         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.

>                 if (opts.retprobe && offset != 0) {
>                         pr_warn("prog '%s': uretprobes do not support offset specification\n",
>                                 prog->name);
> --
> 2.35.1
Delyan Kratunov April 28, 2022, 7:11 p.m. UTC | #2 
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
diff mbox series

Patch

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