[PATCH bpf-next 0/2] libbpf: allow users to set kprobe/uprobe attach mode

[PATCH bpf-next 0/2] libbpf: allow users to set kprobe/uprobe attach mode

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

By default, libbpf will attach the kprobe/uprobe eBPF program in the
latest mode that supported by kernel. In this series, we add the support
to let users manually attach kprobe/uprobe in legacy or perf mode in the
1th patch.

And in the 2th patch, we add the selftests for it.

*** BLURB HERE ***

Menglong Dong (2):
  libbpf: add support to set kprobe/uprobe attach mode
  selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

 tools/lib/bpf/libbpf.c                        | 30 ++++++++-
 tools/lib/bpf/libbpf.h                        | 19 ++++++
 .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
 .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
 4 files changed, 140 insertions(+), 2 deletions(-)

-- 
2.39.0

[PATCH bpf-next 1/2] libbpf: add support to set kprobe/uprobe attach mode

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

By default, libbpf will attach the kprobe/uprobe eBPF program in the
latest mode that supported by kernel. In this patch, we add the support
to let users manually attach kprobe/uprobe in legacy or perf mode.

There are 3 mode that supported by the kernel to attach kprobe/uprobe:

  LEGACY: create perf event in legacy way and don't use bpf_link
  PERF: create perf event with perf_event_open() and don't use bpf_link
  LINK: create perf event with perf_event_open() and use bpf_link

Users now can manually choose the mode with
bpf_program__attach_uprobe_opts()/bpf_program__attach_kprobe_opts().

Link: https://lore.kernel.org/bpf/20230113093427.1666466-1-imagedong@tencent.com/
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 tools/lib/bpf/libbpf.c | 26 +++++++++++++++++++++++++-
 tools/lib/bpf/libbpf.h | 19 ++++++++++++++++---
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index eed5cec6f510..0d20bf1ee301 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9784,7 +9784,7 @@ struct bpf_link *bpf_program__attach_perf_event_opts(const struct bpf_program *p
 	link->link.dealloc = &bpf_link_perf_dealloc;
 	link->perf_event_fd = pfd;
 
-	if (kernel_supports(prog->obj, FEAT_PERF_LINK)) {
+	if (kernel_supports(prog->obj, FEAT_PERF_LINK) && !opts->no_link) {
 		DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_opts,
 			.perf_event.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0));
 
@@ -10148,16 +10148,28 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
 	struct bpf_link *link;
 	size_t offset;
 	bool retprobe, legacy;
+	enum probe_mode mode;
 	int pfd, err;
 
 	if (!OPTS_VALID(opts, bpf_kprobe_opts))
 		return libbpf_err_ptr(-EINVAL);
 
+	mode = OPTS_GET(opts, mode, PROBE_MODE_DEFAULT);
 	retprobe = OPTS_GET(opts, retprobe, false);
 	offset = OPTS_GET(opts, offset, 0);
 	pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
 
 	legacy = determine_kprobe_perf_type() < 0;
+	switch (mode) {
+	case PROBE_MODE_LEGACY:
+		legacy = true;
+	case PROBE_MODE_PERF:
+		pe_opts.no_link = true;
+		break;
+	default:
+		break;
+	}
+
 	if (!legacy) {
 		pfd = perf_event_open_probe(false /* uprobe */, retprobe,
 					    func_name, offset,
@@ -10817,10 +10829,12 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
 	int pfd, err;
 	bool retprobe, legacy;
 	const char *func_name;
+	enum probe_mode mode;
 
 	if (!OPTS_VALID(opts, bpf_uprobe_opts))
 		return libbpf_err_ptr(-EINVAL);
 
+	mode = OPTS_GET(opts, mode, PROBE_MODE_DEFAULT);
 	retprobe = OPTS_GET(opts, retprobe, false);
 	ref_ctr_off = OPTS_GET(opts, ref_ctr_offset, 0);
 	pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
@@ -10849,6 +10863,16 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
 	}
 
 	legacy = determine_uprobe_perf_type() < 0;
+	switch (mode) {
+	case PROBE_MODE_LEGACY:
+		legacy = true;
+	case PROBE_MODE_PERF:
+		pe_opts.no_link = true;
+		break;
+	default:
+		break;
+	}
+
 	if (!legacy) {
 		pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
 					    func_offset, pid, ref_ctr_off);
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 8777ff21ea1d..7fb474e036a3 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -451,8 +451,10 @@ struct bpf_perf_event_opts {
 	size_t sz;
 	/* custom user-provided value fetchable through bpf_get_attach_cookie() */
 	__u64 bpf_cookie;
+	/* don't use bpf_link when attach eBPF pprogram */
+	bool no_link;
 };
-#define bpf_perf_event_opts__last_field bpf_cookie
+#define bpf_perf_event_opts__last_field no_link
 
 LIBBPF_API struct bpf_link *
 bpf_program__attach_perf_event(const struct bpf_program *prog, int pfd);
@@ -461,6 +463,13 @@ LIBBPF_API struct bpf_link *
 bpf_program__attach_perf_event_opts(const struct bpf_program *prog, int pfd,
 				    const struct bpf_perf_event_opts *opts);
 
+enum probe_mode {
+	PROBE_MODE_DEFAULT = 0, /* latest supported by kernel */
+	PROBE_MODE_LEGACY,
+	PROBE_MODE_PERF,
+	PROBE_MODE_LINK,
+};
+
 struct bpf_kprobe_opts {
 	/* size of this struct, for forward/backward compatiblity */
 	size_t sz;
@@ -470,9 +479,11 @@ struct bpf_kprobe_opts {
 	size_t offset;
 	/* kprobe is return probe */
 	bool retprobe;
+	/* kprobe attach mode */
+	enum probe_mode mode;
 	size_t :0;
 };
-#define bpf_kprobe_opts__last_field retprobe
+#define bpf_kprobe_opts__last_field mode
 
 LIBBPF_API struct bpf_link *
 bpf_program__attach_kprobe(const struct bpf_program *prog, bool retprobe,
@@ -570,9 +581,11 @@ struct bpf_uprobe_opts {
 	 * binary_path.
 	 */
 	const char *func_name;
+	/* uprobe attach mode */
+	enum probe_mode mode;
 	size_t :0;
 };
-#define bpf_uprobe_opts__last_field func_name
+#define bpf_uprobe_opts__last_field mode
 
 /**
  * @brief **bpf_program__attach_uprobe()** attaches a BPF program
-- 
2.39.0

Re: [PATCH bpf-next 1/2] libbpf: add support to set kprobe/uprobe attach mode

[Andrii Nakryiko]

On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
>
> From: Menglong Dong <imagedong@tencent.com>
>
> By default, libbpf will attach the kprobe/uprobe eBPF program in the
> latest mode that supported by kernel. In this patch, we add the support
> to let users manually attach kprobe/uprobe in legacy or perf mode.
>
> There are 3 mode that supported by the kernel to attach kprobe/uprobe:
>
>   LEGACY: create perf event in legacy way and don't use bpf_link
>   PERF: create perf event with perf_event_open() and don't use bpf_link
>   LINK: create perf event with perf_event_open() and use bpf_link
>
> Users now can manually choose the mode with
> bpf_program__attach_uprobe_opts()/bpf_program__attach_kprobe_opts().
>
> Link: https://lore.kernel.org/bpf/20230113093427.1666466-1-imagedong@tencent.com/
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  tools/lib/bpf/libbpf.c | 26 +++++++++++++++++++++++++-
>  tools/lib/bpf/libbpf.h | 19 ++++++++++++++++---
>  2 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index eed5cec6f510..0d20bf1ee301 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -9784,7 +9784,7 @@ struct bpf_link *bpf_program__attach_perf_event_opts(const struct bpf_program *p
>         link->link.dealloc = &bpf_link_perf_dealloc;
>         link->perf_event_fd = pfd;
>
> -       if (kernel_supports(prog->obj, FEAT_PERF_LINK)) {
> +       if (kernel_supports(prog->obj, FEAT_PERF_LINK) && !opts->no_link) {
>                 DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_opts,
>                         .perf_event.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0));
>
> @@ -10148,16 +10148,28 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
>         struct bpf_link *link;
>         size_t offset;
>         bool retprobe, legacy;
> +       enum probe_mode mode;
>         int pfd, err;
>
>         if (!OPTS_VALID(opts, bpf_kprobe_opts))
>                 return libbpf_err_ptr(-EINVAL);
>
> +       mode = OPTS_GET(opts, mode, PROBE_MODE_DEFAULT);
>         retprobe = OPTS_GET(opts, retprobe, false);
>         offset = OPTS_GET(opts, offset, 0);
>         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
>
>         legacy = determine_kprobe_perf_type() < 0;
> +       switch (mode) {
> +       case PROBE_MODE_LEGACY:
> +               legacy = true;
> +       case PROBE_MODE_PERF:
> +               pe_opts.no_link = true;
> +               break;
> +       default:
> +               break;
> +       }
> +
>         if (!legacy) {
>                 pfd = perf_event_open_probe(false /* uprobe */, retprobe,
>                                             func_name, offset,
> @@ -10817,10 +10829,12 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
>         int pfd, err;
>         bool retprobe, legacy;
>         const char *func_name;
> +       enum probe_mode mode;
>
>         if (!OPTS_VALID(opts, bpf_uprobe_opts))
>                 return libbpf_err_ptr(-EINVAL);
>
> +       mode = OPTS_GET(opts, mode, PROBE_MODE_DEFAULT);
>         retprobe = OPTS_GET(opts, retprobe, false);
>         ref_ctr_off = OPTS_GET(opts, ref_ctr_offset, 0);
>         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
> @@ -10849,6 +10863,16 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
>         }
>
>         legacy = determine_uprobe_perf_type() < 0;
> +       switch (mode) {
> +       case PROBE_MODE_LEGACY:
> +               legacy = true;
> +       case PROBE_MODE_PERF:
> +               pe_opts.no_link = true;
> +               break;
> +       default:
> +               break;
> +       }
> +

I think this is a good place to also return errors early if, say, user
requested LINK mode, but that mode is not supported by kernel. Instead
of returning some -ENOTSUP generic error, we can error out early?
Similar for PERF mode, if only legacy is supported, and so on. Similar
for attach_uprobe, of course.

>         if (!legacy) {
>                 pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
>                                             func_offset, pid, ref_ctr_off);
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 8777ff21ea1d..7fb474e036a3 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -451,8 +451,10 @@ struct bpf_perf_event_opts {
>         size_t sz;
>         /* custom user-provided value fetchable through bpf_get_attach_cookie() */
>         __u64 bpf_cookie;
> +       /* don't use bpf_link when attach eBPF pprogram */

typo: pprogram

> +       bool no_link;

I've been struggling with this "no_link" name a bit. It is quite
confusing considering that from libbpf's API side we do return `struct
bpf_link`. So I'm thinking that maybe "force_ioctl_attach" would be a
better way to describe it (and will be scary enough for people not
knowing what this is about to not set it to true?)

Also, we'll need size_t: 0 at the end to avoid uninitialized padding
issues (like we have in kprobe_opts and uprobe_opts)

>  };
> -#define bpf_perf_event_opts__last_field bpf_cookie
> +#define bpf_perf_event_opts__last_field no_link
>
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_perf_event(const struct bpf_program *prog, int pfd);
> @@ -461,6 +463,13 @@ LIBBPF_API struct bpf_link *
>  bpf_program__attach_perf_event_opts(const struct bpf_program *prog, int pfd,
>                                     const struct bpf_perf_event_opts *opts);
>
> +enum probe_mode {

shall we call it probe_attach_mode?

also let's elaborate a bit more in doc comment that specifying mode
will force libbpf to use that, but if kernel doesn't support it --
then we'll error out.

> +       PROBE_MODE_DEFAULT = 0, /* latest supported by kernel */
> +       PROBE_MODE_LEGACY,
> +       PROBE_MODE_PERF,
> +       PROBE_MODE_LINK,
> +};
> +
>  struct bpf_kprobe_opts {
>         /* size of this struct, for forward/backward compatiblity */
>         size_t sz;
> @@ -470,9 +479,11 @@ struct bpf_kprobe_opts {
>         size_t offset;
>         /* kprobe is return probe */
>         bool retprobe;
> +       /* kprobe attach mode */
> +       enum probe_mode mode;

nit: mode -> attach_mode?

>         size_t :0;
>  };
> -#define bpf_kprobe_opts__last_field retprobe
> +#define bpf_kprobe_opts__last_field mode
>
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_kprobe(const struct bpf_program *prog, bool retprobe,
> @@ -570,9 +581,11 @@ struct bpf_uprobe_opts {
>          * binary_path.
>          */
>         const char *func_name;
> +       /* uprobe attach mode */
> +       enum probe_mode mode;
>         size_t :0;
>  };
> -#define bpf_uprobe_opts__last_field func_name
> +#define bpf_uprobe_opts__last_field mode

ditto

>
>  /**
>   * @brief **bpf_program__attach_uprobe()** attaches a BPF program
> --
> 2.39.0
>

Re: [PATCH bpf-next 1/2] libbpf: add support to set kprobe/uprobe attach mode

[Menglong Dong]

On Tue, Feb 7, 2023 at 3:59 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
> >
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > By default, libbpf will attach the kprobe/uprobe eBPF program in the
> > latest mode that supported by kernel. In this patch, we add the support
> > to let users manually attach kprobe/uprobe in legacy or perf mode.
> >
> > There are 3 mode that supported by the kernel to attach kprobe/uprobe:
> >
> >   LEGACY: create perf event in legacy way and don't use bpf_link
> >   PERF: create perf event with perf_event_open() and don't use bpf_link
> >   LINK: create perf event with perf_event_open() and use bpf_link
> >
> > Users now can manually choose the mode with
> > bpf_program__attach_uprobe_opts()/bpf_program__attach_kprobe_opts().
> >
> > Link: https://lore.kernel.org/bpf/20230113093427.1666466-1-imagedong@tencent.com/
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 26 +++++++++++++++++++++++++-
> >  tools/lib/bpf/libbpf.h | 19 ++++++++++++++++---
> >  2 files changed, 41 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index eed5cec6f510..0d20bf1ee301 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -9784,7 +9784,7 @@ struct bpf_link *bpf_program__attach_perf_event_opts(const struct bpf_program *p
> >         link->link.dealloc = &bpf_link_perf_dealloc;
> >         link->perf_event_fd = pfd;
> >
> > -       if (kernel_supports(prog->obj, FEAT_PERF_LINK)) {
> > +       if (kernel_supports(prog->obj, FEAT_PERF_LINK) && !opts->no_link) {
> >                 DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_opts,
> >                         .perf_event.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0));
> >
> > @@ -10148,16 +10148,28 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> >         struct bpf_link *link;
> >         size_t offset;
> >         bool retprobe, legacy;
> > +       enum probe_mode mode;
> >         int pfd, err;
> >
> >         if (!OPTS_VALID(opts, bpf_kprobe_opts))
> >                 return libbpf_err_ptr(-EINVAL);
> >
> > +       mode = OPTS_GET(opts, mode, PROBE_MODE_DEFAULT);
> >         retprobe = OPTS_GET(opts, retprobe, false);
> >         offset = OPTS_GET(opts, offset, 0);
> >         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
> >
> >         legacy = determine_kprobe_perf_type() < 0;
> > +       switch (mode) {
> > +       case PROBE_MODE_LEGACY:
> > +               legacy = true;
> > +       case PROBE_MODE_PERF:
> > +               pe_opts.no_link = true;
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +
> >         if (!legacy) {
> >                 pfd = perf_event_open_probe(false /* uprobe */, retprobe,
> >                                             func_name, offset,
> > @@ -10817,10 +10829,12 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> >         int pfd, err;
> >         bool retprobe, legacy;
> >         const char *func_name;
> > +       enum probe_mode mode;
> >
> >         if (!OPTS_VALID(opts, bpf_uprobe_opts))
> >                 return libbpf_err_ptr(-EINVAL);
> >
> > +       mode = OPTS_GET(opts, mode, PROBE_MODE_DEFAULT);
> >         retprobe = OPTS_GET(opts, retprobe, false);
> >         ref_ctr_off = OPTS_GET(opts, ref_ctr_offset, 0);
> >         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
> > @@ -10849,6 +10863,16 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> >         }
> >
> >         legacy = determine_uprobe_perf_type() < 0;
> > +       switch (mode) {
> > +       case PROBE_MODE_LEGACY:
> > +               legacy = true;
> > +       case PROBE_MODE_PERF:
> > +               pe_opts.no_link = true;
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +
>
> I think this is a good place to also return errors early if, say, user
> requested LINK mode, but that mode is not supported by kernel. Instead
> of returning some -ENOTSUP generic error, we can error out early?
> Similar for PERF mode, if only legacy is supported, and so on. Similar
> for attach_uprobe, of course.
>

Yes, sounds great.

> >         if (!legacy) {
> >                 pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
> >                                             func_offset, pid, ref_ctr_off);
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 8777ff21ea1d..7fb474e036a3 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -451,8 +451,10 @@ struct bpf_perf_event_opts {
> >         size_t sz;
> >         /* custom user-provided value fetchable through bpf_get_attach_cookie() */
> >         __u64 bpf_cookie;
> > +       /* don't use bpf_link when attach eBPF pprogram */
>
> typo: pprogram

Ops, get it!

>
> > +       bool no_link;
>
> I've been struggling with this "no_link" name a bit. It is quite
> confusing considering that from libbpf's API side we do return `struct
> bpf_link`. So I'm thinking that maybe "force_ioctl_attach" would be a
> better way to describe it (and will be scary enough for people not
> knowing what this is about to not set it to true?)
>

Yeah, "force_ioctl_attach" sounds more appropriate.

> Also, we'll need size_t: 0 at the end to avoid uninitialized padding
> issues (like we have in kprobe_opts and uprobe_opts)
>
> >  };
> > -#define bpf_perf_event_opts__last_field bpf_cookie
> > +#define bpf_perf_event_opts__last_field no_link
> >
> >  LIBBPF_API struct bpf_link *
> >  bpf_program__attach_perf_event(const struct bpf_program *prog, int pfd);
> > @@ -461,6 +463,13 @@ LIBBPF_API struct bpf_link *
> >  bpf_program__attach_perf_event_opts(const struct bpf_program *prog, int pfd,
> >                                     const struct bpf_perf_event_opts *opts);
> >
> > +enum probe_mode {
>
> shall we call it probe_attach_mode?
>
> also let's elaborate a bit more in doc comment that specifying mode
> will force libbpf to use that, but if kernel doesn't support it --
> then we'll error out.
>

OK.

> > +       PROBE_MODE_DEFAULT = 0, /* latest supported by kernel */
> > +       PROBE_MODE_LEGACY,
> > +       PROBE_MODE_PERF,
> > +       PROBE_MODE_LINK,
> > +};
> > +
> >  struct bpf_kprobe_opts {
> >         /* size of this struct, for forward/backward compatiblity */
> >         size_t sz;
> > @@ -470,9 +479,11 @@ struct bpf_kprobe_opts {
> >         size_t offset;
> >         /* kprobe is return probe */
> >         bool retprobe;
> > +       /* kprobe attach mode */
> > +       enum probe_mode mode;
>
> nit: mode -> attach_mode?
>
> >         size_t :0;
> >  };
> > -#define bpf_kprobe_opts__last_field retprobe
> > +#define bpf_kprobe_opts__last_field mode
> >
> >  LIBBPF_API struct bpf_link *
> >  bpf_program__attach_kprobe(const struct bpf_program *prog, bool retprobe,
> > @@ -570,9 +581,11 @@ struct bpf_uprobe_opts {
> >          * binary_path.
> >          */
> >         const char *func_name;
> > +       /* uprobe attach mode */
> > +       enum probe_mode mode;
> >         size_t :0;
> >  };
> > -#define bpf_uprobe_opts__last_field func_name
> > +#define bpf_uprobe_opts__last_field mode
>
> ditto
>

I'll fix these problems in the V2.

Thanks!
Menglong Dong

> >
> >  /**
> >   * @brief **bpf_program__attach_uprobe()** attaches a BPF program
> > --
> > 2.39.0
> >

[PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Add the testing for kprobe/uprobe attaching in legacy and perf mode.
And the testing passed:

./test_progs -t attach_probe
$5       attach_probe:OK
Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
 .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
 2 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index 9566d9d2f6ee..5f6212ac0b6d 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -23,15 +23,22 @@ static noinline void trigger_func3(void)
 	asm volatile ("");
 }
 
+/* attach point for legacy uprobe */
+static noinline void trigger_func4(void)
+{
+	asm volatile ("");
+}
+
 static char test_data[] = "test_data";
 
 void test_attach_probe(void)
 {
+	ssize_t uprobe_offset, uprobe4_offset, ref_ctr_offset;
 	DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
 	struct bpf_link *kprobe_link, *kretprobe_link;
 	struct bpf_link *uprobe_link, *uretprobe_link;
+	DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts);
 	struct test_attach_probe* skel;
-	ssize_t uprobe_offset, ref_ctr_offset;
 	struct bpf_link *uprobe_err_link;
 	bool legacy;
 	char *mem;
@@ -86,6 +93,25 @@ void test_attach_probe(void)
 		goto cleanup;
 	skel->links.handle_kretprobe = kretprobe_link;
 
+	/* manual-attach kprobe in legacy mode */
+	opts.retprobe = false;
+	opts.mode = PROBE_MODE_LEGACY;
+	kprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kprobe_legacy,
+						      SYS_NANOSLEEP_KPROBE_NAME,
+						      &opts);
+	if (!ASSERT_OK_PTR(kprobe_link, "attach_kprobe_legacy"))
+		goto cleanup;
+	skel->links.handle_kprobe_legacy = kprobe_link;
+
+	/* manual-attach kprobe in perf mode */
+	opts.mode = PROBE_MODE_PERF;
+	kprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kprobe_perf,
+						      SYS_NANOSLEEP_KPROBE_NAME,
+						      &opts);
+	if (!ASSERT_OK_PTR(kprobe_link, "attach_kprobe_perf"))
+		goto cleanup;
+	skel->links.handle_kprobe_perf = kprobe_link;
+
 	/* auto-attachable kprobe and kretprobe */
 	skel->links.handle_kprobe_auto = bpf_program__attach(skel->progs.handle_kprobe_auto);
 	ASSERT_OK_PTR(skel->links.handle_kprobe_auto, "attach_kprobe_auto");
@@ -110,6 +136,32 @@ void test_attach_probe(void)
 	if (!legacy)
 		ASSERT_GT(uprobe_ref_ctr, 0, "uprobe_ref_ctr_after");
 
+	uprobe4_offset = get_uprobe_offset(&trigger_func4);
+	if (!ASSERT_GE(uprobe4_offset, 0, "uprobe4_offset"))
+		goto cleanup;
+
+	uprobe_opts.mode = PROBE_MODE_LEGACY;
+	uprobe_opts.ref_ctr_offset = 0;
+	uprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_legacy,
+						      0 /* self pid */,
+						      "/proc/self/exe",
+						      uprobe4_offset,
+						      &uprobe_opts);
+	if (!ASSERT_OK_PTR(uprobe_link, "attach_uprobe_legacy"))
+		goto cleanup;
+	skel->links.handle_uprobe_legacy = uprobe_link;
+
+	uprobe_opts.mode = PROBE_MODE_PERF;
+	uprobe_opts.ref_ctr_offset = legacy ? 0 : ref_ctr_offset;
+	uprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_perf,
+						      0 /* self pid */,
+						      "/proc/self/exe",
+						      uprobe_offset,
+						      &uprobe_opts);
+	if (!ASSERT_OK_PTR(uprobe_link, "attach_uprobe_perf"))
+		goto cleanup;
+	skel->links.handle_uprobe_perf = uprobe_link;
+
 	/* if uprobe uses ref_ctr, uretprobe has to use ref_ctr as well */
 	uprobe_opts.retprobe = true;
 	uprobe_opts.ref_ctr_offset = legacy ? 0 : ref_ctr_offset;
@@ -207,11 +259,18 @@ void test_attach_probe(void)
 	/* trigger & validate sleepable uprobe attached by name */
 	trigger_func3();
 
+	/* trigger & validate uprobe in legacy mode */
+	trigger_func4();
+
 	ASSERT_EQ(skel->bss->kprobe_res, 1, "check_kprobe_res");
 	ASSERT_EQ(skel->bss->kprobe2_res, 11, "check_kprobe_auto_res");
+	ASSERT_EQ(skel->bss->kprobe3_res, 3, "check_kprobe_legacy_res");
+	ASSERT_EQ(skel->bss->kprobe4_res, 4, "check_kprobe_perf_res");
 	ASSERT_EQ(skel->bss->kretprobe_res, 2, "check_kretprobe_res");
 	ASSERT_EQ(skel->bss->kretprobe2_res, 22, "check_kretprobe_auto_res");
 	ASSERT_EQ(skel->bss->uprobe_res, 3, "check_uprobe_res");
+	ASSERT_EQ(skel->bss->uprobe2_res, 4, "check_uprobe_legacy_res");
+	ASSERT_EQ(skel->bss->uprobe3_res, 5, "check_uprobe_perf_res");
 	ASSERT_EQ(skel->bss->uretprobe_res, 4, "check_uretprobe_res");
 	ASSERT_EQ(skel->bss->uprobe_byname_res, 5, "check_uprobe_byname_res");
 	ASSERT_EQ(skel->bss->uretprobe_byname_res, 6, "check_uretprobe_byname_res");
diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
index a1e45fec8938..6674deea5686 100644
--- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
+++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
@@ -9,9 +9,13 @@
 
 int kprobe_res = 0;
 int kprobe2_res = 0;
+int kprobe3_res = 0;
+int kprobe4_res = 0;
 int kretprobe_res = 0;
 int kretprobe2_res = 0;
 int uprobe_res = 0;
+int uprobe2_res = 0;
+int uprobe3_res = 0;
 int uretprobe_res = 0;
 int uprobe_byname_res = 0;
 int uretprobe_byname_res = 0;
@@ -30,6 +34,20 @@ int handle_kprobe(struct pt_regs *ctx)
 	return 0;
 }
 
+SEC("kprobe")
+int handle_kprobe_legacy(struct pt_regs *ctx)
+{
+	kprobe3_res = 3;
+	return 0;
+}
+
+SEC("kprobe")
+int handle_kprobe_perf(struct pt_regs *ctx)
+{
+	kprobe4_res = 4;
+	return 0;
+}
+
 SEC("ksyscall/nanosleep")
 int BPF_KSYSCALL(handle_kprobe_auto, struct __kernel_timespec *req, struct __kernel_timespec *rem)
 {
@@ -69,6 +87,20 @@ int handle_uprobe(struct pt_regs *ctx)
 	return 0;
 }
 
+SEC("uprobe")
+int handle_uprobe_legacy(struct pt_regs *ctx)
+{
+	uprobe2_res = 4;
+	return 0;
+}
+
+SEC("uprobe")
+int handle_uprobe_perf(struct pt_regs *ctx)
+{
+	uprobe3_res = 5;
+	return 0;
+}
+
 SEC("uretprobe")
 int handle_uretprobe(struct pt_regs *ctx)
 {
-- 
2.39.0

Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Andrii Nakryiko]

On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
>
> From: Menglong Dong <imagedong@tencent.com>
>
> Add the testing for kprobe/uprobe attaching in legacy and perf mode.
> And the testing passed:
>
> ./test_progs -t attach_probe
> $5       attach_probe:OK
> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---

Do you mind refactoring attach_probe test into multiple subtests,
where each subtest will only test one of the attach mode and type. The
reason is that libbpf CI runs tests with latest selftests and libbpf
against old kernels (4.9 and 5.5, currently). Due to attach_probe
testing all these uprobe/kprobe attach modes with extra features (like
cookie, ref count, etc), we had to disable attach_probe test in libbpf
CI on old kernels.

If we can split each individual uprobe/kprobe mode, that will give us
flexibility to selectively allowlist those tests that don't force
libbpf to use newer features (like cookies, LINK or PERF mode, etc).

It would be a great improvement and highly appreciated! If you don't
mind doing this, let's do the split of existing use cases into subtest
in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
of that patch.


>  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
>  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
>  2 files changed, 92 insertions(+), 1 deletion(-)
>

[...]

Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Menglong Dong]

On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
> >
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Add the testing for kprobe/uprobe attaching in legacy and perf mode.
> > And the testing passed:
> >
> > ./test_progs -t attach_probe
> > $5       attach_probe:OK
> > Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> >
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
>
> Do you mind refactoring attach_probe test into multiple subtests,
> where each subtest will only test one of the attach mode and type. The
> reason is that libbpf CI runs tests with latest selftests and libbpf
> against old kernels (4.9 and 5.5, currently). Due to attach_probe
> testing all these uprobe/kprobe attach modes with extra features (like
> cookie, ref count, etc), we had to disable attach_probe test in libbpf
> CI on old kernels.
>
> If we can split each individual uprobe/kprobe mode, that will give us
> flexibility to selectively allowlist those tests that don't force
> libbpf to use newer features (like cookies, LINK or PERF mode, etc).
>
> It would be a great improvement and highly appreciated! If you don't
> mind doing this, let's do the split of existing use cases into subtest
> in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
> of that patch.
>

Of course, with pleasure. For the existing use cases, we split it into
subtests, such as:

  kprobe/kretprobe auto attach
  kprobe/kretprobe manual attach
  uprobe/uretprobe ref_ctr test
  uprobe/uretprobe auto attach
  sleepable kprobe/uprobe
  ......

Am I right?

Thanks!
Dongmeng Long

>
> >  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
> >  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
> >  2 files changed, 92 insertions(+), 1 deletion(-)
> >
>
> [...]

Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Andrii Nakryiko]

On Mon, Feb 6, 2023 at 6:39 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
> On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
> > >
> > > From: Menglong Dong <imagedong@tencent.com>
> > >
> > > Add the testing for kprobe/uprobe attaching in legacy and perf mode.
> > > And the testing passed:
> > >
> > > ./test_progs -t attach_probe
> > > $5       attach_probe:OK
> > > Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> > >
> > > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > > ---
> >
> > Do you mind refactoring attach_probe test into multiple subtests,
> > where each subtest will only test one of the attach mode and type. The
> > reason is that libbpf CI runs tests with latest selftests and libbpf
> > against old kernels (4.9 and 5.5, currently). Due to attach_probe
> > testing all these uprobe/kprobe attach modes with extra features (like
> > cookie, ref count, etc), we had to disable attach_probe test in libbpf
> > CI on old kernels.
> >
> > If we can split each individual uprobe/kprobe mode, that will give us
> > flexibility to selectively allowlist those tests that don't force
> > libbpf to use newer features (like cookies, LINK or PERF mode, etc).
> >
> > It would be a great improvement and highly appreciated! If you don't
> > mind doing this, let's do the split of existing use cases into subtest
> > in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
> > of that patch.
> >
>
> Of course, with pleasure. For the existing use cases, we split it into
> subtests, such as:
>
>   kprobe/kretprobe auto attach
>   kprobe/kretprobe manual attach
>   uprobe/uretprobe ref_ctr test
>   uprobe/uretprobe auto attach
>   sleepable kprobe/uprobe
>   ......
>
> Am I right?

I haven't analysed all the different cases, but roughly it makes
sense. With more granular subtests we can also drop `legacy` flag and
rely on subtest allowlisting in CI.

>
> Thanks!
> Dongmeng Long
>
> >
> > >  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
> > >  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
> > >  2 files changed, 92 insertions(+), 1 deletion(-)
> > >
> >
> > [...]

Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Alan Maguire]

On 07/02/2023 22:50, Andrii Nakryiko wrote:
> On Mon, Feb 6, 2023 at 6:39 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
>>
>> On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
>> <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
>>>>
>>>> From: Menglong Dong <imagedong@tencent.com>
>>>>
>>>> Add the testing for kprobe/uprobe attaching in legacy and perf mode.
>>>> And the testing passed:
>>>>
>>>> ./test_progs -t attach_probe
>>>> $5       attach_probe:OK
>>>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>>>>
>>>> Signed-off-by: Menglong Dong <imagedong@tencent.com>
>>>> ---
>>>
>>> Do you mind refactoring attach_probe test into multiple subtests,
>>> where each subtest will only test one of the attach mode and type. The
>>> reason is that libbpf CI runs tests with latest selftests and libbpf
>>> against old kernels (4.9 and 5.5, currently). Due to attach_probe
>>> testing all these uprobe/kprobe attach modes with extra features (like
>>> cookie, ref count, etc), we had to disable attach_probe test in libbpf
>>> CI on old kernels.
>>>
>>> If we can split each individual uprobe/kprobe mode, that will give us
>>> flexibility to selectively allowlist those tests that don't force
>>> libbpf to use newer features (like cookies, LINK or PERF mode, etc).
>>>
>>> It would be a great improvement and highly appreciated! If you don't
>>> mind doing this, let's do the split of existing use cases into subtest
>>> in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
>>> of that patch.
>>>
>>
>> Of course, with pleasure. For the existing use cases, we split it into
>> subtests, such as:
>>
>>   kprobe/kretprobe auto attach
>>   kprobe/kretprobe manual attach
>>   uprobe/uretprobe ref_ctr test
>>   uprobe/uretprobe auto attach
>>   sleepable kprobe/uprobe
>>   ......
>>
>> Am I right?
> 
> I haven't analysed all the different cases, but roughly it makes
> sense. With more granular subtests we can also drop `legacy` flag and
> rely on subtest allowlisting in CI.
>

I'm probably rusty on the details, but when you talk about subtest
splitting for the [uk]probe manual attach, are we talking about running
the same manual attach test for the different modes, with each as a 
separate subtest, such that each registers as a distinct pass/fail (and
can thus be allowlisted as appropriate)? So in other words

test__start_subtest("manual_attach_kprobe_link");
attach_kprobe_manual(link_options);
test__start_subtest("manual_attach_kprobe_legacy");
attach_kprobe_manual(legay_options);
test__start_subtest("manual_attach_kprobe_perf");
attach_kprobe_manual(perf_options);

?

>>
>> Thanks!
>> Dongmeng Long
>>
>>>
>>>>  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
>>>>  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
>>>>  2 files changed, 92 insertions(+), 1 deletion(-)
>>>>
>>>
>>> [...]

Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Andrii Nakryiko]

On Wed, Feb 8, 2023 at 3:49 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 07/02/2023 22:50, Andrii Nakryiko wrote:
> > On Mon, Feb 6, 2023 at 6:39 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
> >>
> >> On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >>>
> >>> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
> >>>>
> >>>> From: Menglong Dong <imagedong@tencent.com>
> >>>>
> >>>> Add the testing for kprobe/uprobe attaching in legacy and perf mode.
> >>>> And the testing passed:
> >>>>
> >>>> ./test_progs -t attach_probe
> >>>> $5       attach_probe:OK
> >>>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> >>>>
> >>>> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> >>>> ---
> >>>
> >>> Do you mind refactoring attach_probe test into multiple subtests,
> >>> where each subtest will only test one of the attach mode and type. The
> >>> reason is that libbpf CI runs tests with latest selftests and libbpf
> >>> against old kernels (4.9 and 5.5, currently). Due to attach_probe
> >>> testing all these uprobe/kprobe attach modes with extra features (like
> >>> cookie, ref count, etc), we had to disable attach_probe test in libbpf
> >>> CI on old kernels.
> >>>
> >>> If we can split each individual uprobe/kprobe mode, that will give us
> >>> flexibility to selectively allowlist those tests that don't force
> >>> libbpf to use newer features (like cookies, LINK or PERF mode, etc).
> >>>
> >>> It would be a great improvement and highly appreciated! If you don't
> >>> mind doing this, let's do the split of existing use cases into subtest
> >>> in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
> >>> of that patch.
> >>>
> >>
> >> Of course, with pleasure. For the existing use cases, we split it into
> >> subtests, such as:
> >>
> >>   kprobe/kretprobe auto attach
> >>   kprobe/kretprobe manual attach
> >>   uprobe/uretprobe ref_ctr test
> >>   uprobe/uretprobe auto attach
> >>   sleepable kprobe/uprobe
> >>   ......
> >>
> >> Am I right?
> >
> > I haven't analysed all the different cases, but roughly it makes
> > sense. With more granular subtests we can also drop `legacy` flag and
> > rely on subtest allowlisting in CI.
> >
>
> I'm probably rusty on the details, but when you talk about subtest
> splitting for the [uk]probe manual attach, are we talking about running
> the same manual attach test for the different modes, with each as a
> separate subtest, such that each registers as a distinct pass/fail (and
> can thus be allowlisted as appropriate)? So in other words
>
> test__start_subtest("manual_attach_kprobe_link");
> attach_kprobe_manual(link_options);
> test__start_subtest("manual_attach_kprobe_legacy");
> attach_kprobe_manual(legay_options);
> test__start_subtest("manual_attach_kprobe_perf");
> attach_kprobe_manual(perf_options);
>
> ?

Yep. One of the reasons is that on 4.9 kernel there won't be link or
perf method available, so it is expected for such modes to fail. I
want to be able to still test the legacy code path on 4.9, though.
Similarly tests that are using ref_ctr_offset or bpf_cookie won't work
on older kernels as well. Right now because of all of them being in a
single test, I have to block entire test, losing coverage on older
kernels.

>
> >>
> >> Thanks!
> >> Dongmeng Long
> >>
> >>>
> >>>>  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
> >>>>  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
> >>>>  2 files changed, 92 insertions(+), 1 deletion(-)
> >>>>
> >>>
> >>> [...]

Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Menglong Dong]

On Thu, Feb 9, 2023 at 7:31 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Feb 8, 2023 at 3:49 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On 07/02/2023 22:50, Andrii Nakryiko wrote:
> > > On Mon, Feb 6, 2023 at 6:39 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
> > >>
> > >> On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
> > >> <andrii.nakryiko@gmail.com> wrote:
> > >>>
> > >>> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
> > >>>>
> > >>>> From: Menglong Dong <imagedong@tencent.com>
> > >>>>
> > >>>> Add the testing for kprobe/uprobe attaching in legacy and perf mode.
> > >>>> And the testing passed:
> > >>>>
> > >>>> ./test_progs -t attach_probe
> > >>>> $5       attach_probe:OK
> > >>>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> > >>>>
> > >>>> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > >>>> ---
> > >>>
> > >>> Do you mind refactoring attach_probe test into multiple subtests,
> > >>> where each subtest will only test one of the attach mode and type. The
> > >>> reason is that libbpf CI runs tests with latest selftests and libbpf
> > >>> against old kernels (4.9 and 5.5, currently). Due to attach_probe
> > >>> testing all these uprobe/kprobe attach modes with extra features (like
> > >>> cookie, ref count, etc), we had to disable attach_probe test in libbpf
> > >>> CI on old kernels.
> > >>>
> > >>> If we can split each individual uprobe/kprobe mode, that will give us
> > >>> flexibility to selectively allowlist those tests that don't force
> > >>> libbpf to use newer features (like cookies, LINK or PERF mode, etc).
> > >>>
> > >>> It would be a great improvement and highly appreciated! If you don't
> > >>> mind doing this, let's do the split of existing use cases into subtest
> > >>> in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
> > >>> of that patch.
> > >>>
> > >>
> > >> Of course, with pleasure. For the existing use cases, we split it into
> > >> subtests, such as:
> > >>
> > >>   kprobe/kretprobe auto attach
> > >>   kprobe/kretprobe manual attach
> > >>   uprobe/uretprobe ref_ctr test
> > >>   uprobe/uretprobe auto attach
> > >>   sleepable kprobe/uprobe
> > >>   ......
> > >>
> > >> Am I right?
> > >
> > > I haven't analysed all the different cases, but roughly it makes
> > > sense. With more granular subtests we can also drop `legacy` flag and
> > > rely on subtest allowlisting in CI.
> > >
> >
> > I'm probably rusty on the details, but when you talk about subtest
> > splitting for the [uk]probe manual attach, are we talking about running
> > the same manual attach test for the different modes, with each as a
> > separate subtest, such that each registers as a distinct pass/fail (and
> > can thus be allowlisted as appropriate)? So in other words
> >
> > test__start_subtest("manual_attach_kprobe_link");
> > attach_kprobe_manual(link_options);
> > test__start_subtest("manual_attach_kprobe_legacy");
> > attach_kprobe_manual(legay_options);
> > test__start_subtest("manual_attach_kprobe_perf");
> > attach_kprobe_manual(perf_options);
> >
> > ?
>
> Yep. One of the reasons is that on 4.9 kernel there won't be link or
> perf method available, so it is expected for such modes to fail. I
> want to be able to still test the legacy code path on 4.9, though.
> Similarly tests that are using ref_ctr_offset or bpf_cookie won't work
> on older kernels as well. Right now because of all of them being in a
> single test, I have to block entire test, losing coverage on older
> kernels.
>

I think I am beginning to understand it now. So we need 2 patches
for the selftests part. In the 1th patch, we split the existing testings
into multi subtests, such as:

test__start_subtest("manual_attach_probe") // test kprobe/uprobe manual attach
test__start_subtest("auto_attach_probe") // test kprobe/uprobe auto attach
test__start_subtest("bpf_cookie") // test bpf_cookie
test__start_subtest("ref_ctr_offset") test ref_ctr_offset
test__start_subtest("sleepable_probe") // test sleepable
uprobe/uretprobe, and sleepable kprobe
......

And in the 2th patch, we change the subtest "manual_attach_probe" into
the following tests:

test__start_subtest("manual_attach_kprobe_link");
test__start_subtest("manual_attach_kprobe_legacy");
test__start_subtest("manual_attach_kprobe_perf");

Therefore, every feature can be tested in a subtest alone.

Am I right?

Thanks!
Menglong Dong
> >
> > >>
> > >> Thanks!
> > >> Dongmeng Long
> > >>
> > >>>
> > >>>>  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
> > >>>>  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
> > >>>>  2 files changed, 92 insertions(+), 1 deletion(-)
> > >>>>
> > >>>
> > >>> [...]

Re: [PATCH bpf-next 2/2] selftests/bpf: add test for legacy/perf kprobe/uprobe attach mode

[Andrii Nakryiko]

On Thu, Feb 9, 2023 at 6:08 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
> On Thu, Feb 9, 2023 at 7:31 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Feb 8, 2023 at 3:49 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > >
> > > On 07/02/2023 22:50, Andrii Nakryiko wrote:
> > > > On Mon, Feb 6, 2023 at 6:39 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
> > > >>
> > > >> On Tue, Feb 7, 2023 at 4:05 AM Andrii Nakryiko
> > > >> <andrii.nakryiko@gmail.com> wrote:
> > > >>>
> > > >>> On Thu, Feb 2, 2023 at 7:18 PM <menglong8.dong@gmail.com> wrote:
> > > >>>>
> > > >>>> From: Menglong Dong <imagedong@tencent.com>
> > > >>>>
> > > >>>> Add the testing for kprobe/uprobe attaching in legacy and perf mode.
> > > >>>> And the testing passed:
> > > >>>>
> > > >>>> ./test_progs -t attach_probe
> > > >>>> $5       attach_probe:OK
> > > >>>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> > > >>>>
> > > >>>> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > > >>>> ---
> > > >>>
> > > >>> Do you mind refactoring attach_probe test into multiple subtests,
> > > >>> where each subtest will only test one of the attach mode and type. The
> > > >>> reason is that libbpf CI runs tests with latest selftests and libbpf
> > > >>> against old kernels (4.9 and 5.5, currently). Due to attach_probe
> > > >>> testing all these uprobe/kprobe attach modes with extra features (like
> > > >>> cookie, ref count, etc), we had to disable attach_probe test in libbpf
> > > >>> CI on old kernels.
> > > >>>
> > > >>> If we can split each individual uprobe/kprobe mode, that will give us
> > > >>> flexibility to selectively allowlist those tests that don't force
> > > >>> libbpf to use newer features (like cookies, LINK or PERF mode, etc).
> > > >>>
> > > >>> It would be a great improvement and highly appreciated! If you don't
> > > >>> mind doing this, let's do the split of existing use cases into subtest
> > > >>> in a separate patch, and then add PERF/LEGACY/LINK mode tests on top
> > > >>> of that patch.
> > > >>>
> > > >>
> > > >> Of course, with pleasure. For the existing use cases, we split it into
> > > >> subtests, such as:
> > > >>
> > > >>   kprobe/kretprobe auto attach
> > > >>   kprobe/kretprobe manual attach
> > > >>   uprobe/uretprobe ref_ctr test
> > > >>   uprobe/uretprobe auto attach
> > > >>   sleepable kprobe/uprobe
> > > >>   ......
> > > >>
> > > >> Am I right?
> > > >
> > > > I haven't analysed all the different cases, but roughly it makes
> > > > sense. With more granular subtests we can also drop `legacy` flag and
> > > > rely on subtest allowlisting in CI.
> > > >
> > >
> > > I'm probably rusty on the details, but when you talk about subtest
> > > splitting for the [uk]probe manual attach, are we talking about running
> > > the same manual attach test for the different modes, with each as a
> > > separate subtest, such that each registers as a distinct pass/fail (and
> > > can thus be allowlisted as appropriate)? So in other words
> > >
> > > test__start_subtest("manual_attach_kprobe_link");
> > > attach_kprobe_manual(link_options);
> > > test__start_subtest("manual_attach_kprobe_legacy");
> > > attach_kprobe_manual(legay_options);
> > > test__start_subtest("manual_attach_kprobe_perf");
> > > attach_kprobe_manual(perf_options);
> > >
> > > ?
> >
> > Yep. One of the reasons is that on 4.9 kernel there won't be link or
> > perf method available, so it is expected for such modes to fail. I
> > want to be able to still test the legacy code path on 4.9, though.
> > Similarly tests that are using ref_ctr_offset or bpf_cookie won't work
> > on older kernels as well. Right now because of all of them being in a
> > single test, I have to block entire test, losing coverage on older
> > kernels.
> >
>
> I think I am beginning to understand it now. So we need 2 patches
> for the selftests part. In the 1th patch, we split the existing testings
> into multi subtests, such as:
>
> test__start_subtest("manual_attach_probe") // test kprobe/uprobe manual attach
> test__start_subtest("auto_attach_probe") // test kprobe/uprobe auto attach
> test__start_subtest("bpf_cookie") // test bpf_cookie
> test__start_subtest("ref_ctr_offset") test ref_ctr_offset
> test__start_subtest("sleepable_probe") // test sleepable
> uprobe/uretprobe, and sleepable kprobe
> ......
>
> And in the 2th patch, we change the subtest "manual_attach_probe" into
> the following tests:
>
> test__start_subtest("manual_attach_kprobe_link");
> test__start_subtest("manual_attach_kprobe_legacy");
> test__start_subtest("manual_attach_kprobe_perf");
>
> Therefore, every feature can be tested in a subtest alone.
>
> Am I right?

yep, exactly!

>
> Thanks!
> Menglong Dong
> > >
> > > >>
> > > >> Thanks!
> > > >> Dongmeng Long
> > > >>
> > > >>>
> > > >>>>  .../selftests/bpf/prog_tests/attach_probe.c   | 61 ++++++++++++++++++-
> > > >>>>  .../selftests/bpf/progs/test_attach_probe.c   | 32 ++++++++++
> > > >>>>  2 files changed, 92 insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>
> > > >>> [...]