[PATCH iproute2-next 0/2] Increase BPF verifier verbosity when in verbose mode

[PATCH iproute2-next 0/2] Increase BPF verifier verbosity when in verbose mode

[Shung-Hsi Yu]

When debugging BPF verifier issue, it is useful get as much information
out of the verifier as possible to help diagnostic, but right now that
is not possible because load_bpf_object() does not set the
kernel_log_level in bpf_open_opts, which is addressed in patch 1.

Patch 2 further allows increasing the log level in verbose mode, so even
more information can be retrieved out of the verifier, and most
importlantly, show verifier log even on successful BPF program load.


Shung-Hsi Yu (2):
  libbpf: set kernel_log_level when available
  bpf: increase verifier verbosity when in verbose mode

 include/bpf_util.h |  4 ++--
 ip/ipvrf.c         |  3 ++-
 lib/bpf_legacy.c   | 10 ++++++----
 lib/bpf_libbpf.c   |  6 ++++++
 4 files changed, 16 insertions(+), 7 deletions(-)


base-commit: 575322b09c3c6bc1806f2faa31edcfb64df302bb
-- 
2.42.0

[PATCH iproute2-next 1/2] libbpf: set kernel_log_level when available

[Shung-Hsi Yu]

libbpf allows setting the log_level in struct bpf_open_opts through the
kernel_log_level field since v0.7, use it to set log level to align with
bpf_prog_load_dev() and bpf_btf_load().

Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
---
 lib/bpf_libbpf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c
index e1c211a1..f678a710 100644
--- a/lib/bpf_libbpf.c
+++ b/lib/bpf_libbpf.c
@@ -285,6 +285,9 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts,
 			.relaxed_maps = true,
 			.pin_root_path = root_path,
+#ifdef (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
+			.kernel_log_level = 1,
+#endif
 	);
 
 	obj = bpf_object__open_file(cfg->object, &open_opts);
-- 
2.42.0

[PATCH iproute2-next 2/2] bpf: increase verifier verbosity when in verbose mode

[Shung-Hsi Yu]

The BPF verifier allows setting a higher verbosity level, which is
helpful when it comes to debugging verifier issue, specially when used
to BPF program that loads successfully (but should not have passed the
verifier in the first place). Increase the BPF verifier log level in
when in verbose mode to help with such cases.

Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
---
 include/bpf_util.h |  4 ++--
 ip/ipvrf.c         |  3 ++-
 lib/bpf_legacy.c   | 10 ++++++----
 lib/bpf_libbpf.c   |  9 ++++++---
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/include/bpf_util.h b/include/bpf_util.h
index 1c924f50..8951a5e8 100644
--- a/include/bpf_util.h
+++ b/include/bpf_util.h
@@ -273,10 +273,10 @@ void bpf_print_ops(struct rtattr *bpf_ops, __u16 len);
 
 int bpf_prog_load_dev(enum bpf_prog_type type, const struct bpf_insn *insns,
 		      size_t size_insns, const char *license, __u32 ifindex,
-		      char *log, size_t size_log);
+		      char *log, size_t size_log, bool verbose);
 int bpf_program_load(enum bpf_prog_type type, const struct bpf_insn *insns,
 		     size_t size_insns, const char *license, char *log,
-		     size_t size_log);
+		     size_t size_log, bool verbose);
 
 int bpf_prog_attach_fd(int prog_fd, int target_fd, enum bpf_attach_type type);
 int bpf_prog_detach_fd(int target_fd, enum bpf_attach_type type);
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index 12beaec3..e7c702ab 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -253,7 +253,8 @@ static int prog_load(int idx)
 	};
 
 	return bpf_program_load(BPF_PROG_TYPE_CGROUP_SOCK, prog, sizeof(prog),
-				"GPL", bpf_log_buf, sizeof(bpf_log_buf));
+				"GPL", bpf_log_buf, sizeof(bpf_log_buf),
+				false);
 }
 
 static int vrf_configure_cgroup(const char *path, int ifindex)
diff --git a/lib/bpf_legacy.c b/lib/bpf_legacy.c
index 3542b12f..844974e9 100644
--- a/lib/bpf_legacy.c
+++ b/lib/bpf_legacy.c
@@ -1098,7 +1098,7 @@ int bpf_prog_detach_fd(int target_fd, enum bpf_attach_type type)
 
 int bpf_prog_load_dev(enum bpf_prog_type type, const struct bpf_insn *insns,
 		      size_t size_insns, const char *license, __u32 ifindex,
-		      char *log, size_t size_log)
+		      char *log, size_t size_log, bool verbose)
 {
 	union bpf_attr attr = {};
 
@@ -1112,6 +1112,8 @@ int bpf_prog_load_dev(enum bpf_prog_type type, const struct bpf_insn *insns,
 		attr.log_buf = bpf_ptr_to_u64(log);
 		attr.log_size = size_log;
 		attr.log_level = 1;
+		if (verbose)
+			attr.log_level |= 2;
 	}
 
 	return bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
@@ -1119,9 +1121,9 @@ int bpf_prog_load_dev(enum bpf_prog_type type, const struct bpf_insn *insns,
 
 int bpf_program_load(enum bpf_prog_type type, const struct bpf_insn *insns,
 		     size_t size_insns, const char *license, char *log,
-		     size_t size_log)
+		     size_t size_log, bool verbose)
 {
-	return bpf_prog_load_dev(type, insns, size_insns, license, 0, log, size_log);
+	return bpf_prog_load_dev(type, insns, size_insns, license, 0, log, size_log, verbose);
 }
 
 #ifdef HAVE_ELF
@@ -1543,7 +1545,7 @@ retry:
 	errno = 0;
 	fd = bpf_prog_load_dev(prog->type, prog->insns, prog->size,
 			       prog->license, ctx->ifindex,
-			       ctx->log, ctx->log_size);
+			       ctx->log, ctx->log_size, ctx->verbose);
 	if (fd < 0 || ctx->verbose) {
 		/* The verifier log is pretty chatty, sometimes so chatty
 		 * on larger programs, that we could fail to dump everything
diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c
index f678a710..08692d30 100644
--- a/lib/bpf_libbpf.c
+++ b/lib/bpf_libbpf.c
@@ -285,11 +285,14 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts,
 			.relaxed_maps = true,
 			.pin_root_path = root_path,
-#ifdef (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
-			.kernel_log_level = 1,
-#endif
 	);
 
+#if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
+	open_opts.kernel_log_level = 1;
+	if (cfg->verbose)
+		open_opts.kernel_log_level |= 2;
+#endif
+
 	obj = bpf_object__open_file(cfg->object, &open_opts);
 	if (libbpf_get_error(obj)) {
 		fprintf(stderr, "ERROR: opening BPF object file failed\n");
-- 
2.42.0

Re: [PATCH iproute2-next 2/2] bpf: increase verifier verbosity when in verbose mode

[David Ahern]

On 10/18/23 12:22 AM, Shung-Hsi Yu wrote:
> diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c
> index f678a710..08692d30 100644
> --- a/lib/bpf_libbpf.c
> +++ b/lib/bpf_libbpf.c
> @@ -285,11 +285,14 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
>  	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts,
>  			.relaxed_maps = true,
>  			.pin_root_path = root_path,
> -#ifdef (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> -			.kernel_log_level = 1,
> -#endif
>  	);
>  
> +#if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> +	open_opts.kernel_log_level = 1;
> +	if (cfg->verbose)
> +		open_opts.kernel_log_level |= 2;
> +#endif
> +
>  	obj = bpf_object__open_file(cfg->object, &open_opts);
>  	if (libbpf_get_error(obj)) {
>  		fprintf(stderr, "ERROR: opening BPF object file failed\n");

Why have the first patch if you redo the code here?

Re: [PATCH iproute2-next 2/2] bpf: increase verifier verbosity when in verbose mode

[Shung-Hsi Yu]

On Wed, Oct 18, 2023 at 08:35:30AM -0600, David Ahern wrote:
> On 10/18/23 12:22 AM, Shung-Hsi Yu wrote:
> > diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c
> > index f678a710..08692d30 100644
> > --- a/lib/bpf_libbpf.c
> > +++ b/lib/bpf_libbpf.c
> > @@ -285,11 +285,14 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
> >  	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts,
> >  			.relaxed_maps = true,
> >  			.pin_root_path = root_path,
> > -#ifdef (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> > -			.kernel_log_level = 1,
> > -#endif
> >  	);
> >  
> > +#if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> > +	open_opts.kernel_log_level = 1;
> > +	if (cfg->verbose)
> > +		open_opts.kernel_log_level |= 2;
> > +#endif
> > +
> >  	obj = bpf_object__open_file(cfg->object, &open_opts);
> >  	if (libbpf_get_error(obj)) {
> >  		fprintf(stderr, "ERROR: opening BPF object file failed\n");
> 
> Why have the first patch if you redo the code here?

Ah, good point. I was trying to separate out libbpf-related changes from
verbosity-increasing changes, hence the first patch. And there I add the
.kernel_log_level field within DECLARE_LIBBPF_OPTS() because that seems to
be how it's usually done.

In the second patch I tried to make log-level changes consistent, having
them all done with `|= 2`, which isn't possible within
DECLARE_LIBBPF_OPTS().

Maybe I should have just have `open_opts.kernel_log_level = 1;` outside of
DECLARE_LIBBPF_OPTS() in the first patch to begin with.

+#if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
+	open_opts.kernel_log_level = 1;
+#endif

Followed by

 #if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
 	open_opts.kernel_log_level = 1;
+	if (cfg->verbose)
+		open_opts.kernel_log_level |= 2;
 #endif

Would be better than

 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts,
 			.relaxed_maps = true,
 			.pin_root_path = root_path,
+#ifdef (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
+			.kernel_log_level = 1,
+#endif
 	);

Followed by

 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts,
 			.relaxed_maps = true,
 			.pin_root_path = root_path,
 #ifdef (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
-			.kernel_log_level = 1,
+			.kernel_log_level = cfg->verbose ? (2 | 1) : 1,
 #endif
 	);

I suppose. What do you think?

Re: [PATCH iproute2-next 2/2] bpf: increase verifier verbosity when in verbose mode

[David Ahern]

On 10/18/23 7:18 PM, Shung-Hsi Yu wrote:
> Ah, good point. I was trying to separate out libbpf-related changes from
> verbosity-increasing changes, hence the first patch. And there I add the
> .kernel_log_level field within DECLARE_LIBBPF_OPTS() because that seems to
> be how it's usually done.
> 
> In the second patch I tried to make log-level changes consistent, having
> them all done with `|= 2`, which isn't possible within
> DECLARE_LIBBPF_OPTS().
> 
> Maybe I should have just have `open_opts.kernel_log_level = 1;` outside of
> DECLARE_LIBBPF_OPTS() in the first patch to begin with.
> 
> +#if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> +	open_opts.kernel_log_level = 1;
> +#endif
> 
> Followed by
> 
>  #if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
>  	open_opts.kernel_log_level = 1;
> +	if (cfg->verbose)
> +		open_opts.kernel_log_level |= 2;
>  #endif
> 

that is less confusing for a patch sequence.

Re: [PATCH iproute2-next 2/2] bpf: increase verifier verbosity when in verbose mode

[Shung-Hsi Yu]

On Thu, Oct 19, 2023 at 01:44:34PM -0600, David Ahern wrote:
> On 10/18/23 7:18 PM, Shung-Hsi Yu wrote:
> > Ah, good point. I was trying to separate out libbpf-related changes from
> > verbosity-increasing changes, hence the first patch. And there I add the
> > .kernel_log_level field within DECLARE_LIBBPF_OPTS() because that seems to
> > be how it's usually done.
> > 
> > In the second patch I tried to make log-level changes consistent, having
> > them all done with `|= 2`, which isn't possible within
> > DECLARE_LIBBPF_OPTS().
> > 
> > Maybe I should have just have `open_opts.kernel_log_level = 1;` outside of
> > DECLARE_LIBBPF_OPTS() in the first patch to begin with.
> > 
> > +#if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> > +	open_opts.kernel_log_level = 1;
> > +#endif
> > 
> > Followed by
> > 
> >  #if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> >  	open_opts.kernel_log_level = 1;
> > +	if (cfg->verbose)
> > +		open_opts.kernel_log_level |= 2;
> >  #endif
> > 
> 
> that is less confusing for a patch sequence.

Thanks for the review. Will do this in v2.

Re: [PATCH iproute2-next 0/2] Increase BPF verifier verbosity when in verbose mode

[Shung-Hsi Yu]

On Wed, Oct 18, 2023 at 02:22:32PM +0800, Shung-Hsi Yu wrote:
> When debugging BPF verifier issue, it is useful get as much information
> out of the verifier as possible to help diagnostic, but right now that
> is not possible because load_bpf_object() does not set the
> kernel_log_level in bpf_open_opts, which is addressed in patch 1.
> 
> Patch 2 further allows increasing the log level in verbose mode, so even
> more information can be retrieved out of the verifier, and most
> importlantly, show verifier log even on successful BPF program load.

Got some typos in patch 2, and bpf_open_opts should be bpf_object_open_opts.
Will send v2 to correct these mistakes.

> Shung-Hsi Yu (2):
>   libbpf: set kernel_log_level when available
>   bpf: increase verifier verbosity when in verbose mode
> 
>  include/bpf_util.h |  4 ++--
>  ip/ipvrf.c         |  3 ++-
>  lib/bpf_legacy.c   | 10 ++++++----
>  lib/bpf_libbpf.c   |  6 ++++++
>  4 files changed, 16 insertions(+), 7 deletions(-)
> 
> 
> base-commit: 575322b09c3c6bc1806f2faa31edcfb64df302bb
> -- 
> 2.42.0
>