[PATCH bpf-next] libbpf: prevent overwriting of log_level in bpf_object__load_progs()

[PATCH bpf-next] libbpf: prevent overwriting of log_level in bpf_object__load_progs()

[Quentin Monnet]

There are two functions in libbpf that support passing a log_level
parameter for the verifier for loading programs:
bpf_object__load_xattr() and bpf_load_program_xattr(). Both accept an
attribute object containing the log_level, and apply it to the programs
to load.

It turns out that to effectively load the programs, the latter function
eventually relies on the former. This was not taken into account when
adding support for log_level in bpf_object__load_xattr(), and the
log_level passed to bpf_load_program_xattr() later gets overwritten with
a zero value, thus disabling verifier logs for the program in all cases:

bpf_load_program_xattr()             // prog.log_level = N;
 -> bpf_object__load()               // attr.log_level = 0;
     -> bpf_object__load_xattr()     // <pass prog and attr>
         -> bpf_object__load_progs() // prog.log_level = attr.log_level;

Fix this by OR-ing the log_level in bpf_object__load_progs(), instead of
overwriting it.

Fixes: 60276f984998 ("libbpf: add bpf_object__load_xattr() API function to pass log_level")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 tools/lib/bpf/libbpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ca4432f5b067..30cb08e2eb75 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2232,7 +2232,7 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
 	for (i = 0; i < obj->nr_programs; i++) {
 		if (bpf_program__is_function_storage(&obj->programs[i], obj))
 			continue;
-		obj->programs[i].log_level = log_level;
+		obj->programs[i].log_level |= log_level;
 		err = bpf_program__load(&obj->programs[i],
 					obj->license,
 					obj->kern_version);
-- 
2.17.1

Re: [PATCH bpf-next] libbpf: prevent overwriting of log_level in bpf_object__load_progs()

[Daniel Borkmann]

On 05/29/2019 11:23 AM, Quentin Monnet wrote:
> There are two functions in libbpf that support passing a log_level
> parameter for the verifier for loading programs:
> bpf_object__load_xattr() and bpf_load_program_xattr(). Both accept an
> attribute object containing the log_level, and apply it to the programs
> to load.
> 
> It turns out that to effectively load the programs, the latter function
> eventually relies on the former. This was not taken into account when
> adding support for log_level in bpf_object__load_xattr(), and the
> log_level passed to bpf_load_program_xattr() later gets overwritten with
> a zero value, thus disabling verifier logs for the program in all cases:
> 
> bpf_load_program_xattr()             // prog.log_level = N;

I'm confused with your commit message. How can bpf_load_program_xattr()
make sense here, this is the one doing the bpf syscall. Do you mean to
say bpf_prog_load_xattr()? Because this one sets prog->log_level = attr->log_level
and calls bpf_object__load() which in turn does bpf_object__load_xattr()
with an attr that has attr->log_level of 0 such that bpf_object__load_progs()
then overrides it. Unless I'm not missing something, please fix up this
description properly and resubmit.

>  -> bpf_object__load()               // attr.log_level = 0;
>      -> bpf_object__load_xattr()     // <pass prog and attr>
>          -> bpf_object__load_progs() // prog.log_level = attr.log_level;
> 
> Fix this by OR-ing the log_level in bpf_object__load_progs(), instead of
> overwriting it.
> 
> Fixes: 60276f984998 ("libbpf: add bpf_object__load_xattr() API function to pass log_level")
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
>  tools/lib/bpf/libbpf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ca4432f5b067..30cb08e2eb75 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2232,7 +2232,7 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
>  	for (i = 0; i < obj->nr_programs; i++) {
>  		if (bpf_program__is_function_storage(&obj->programs[i], obj))
>  			continue;
> -		obj->programs[i].log_level = log_level;
> +		obj->programs[i].log_level |= log_level;
>  		err = bpf_program__load(&obj->programs[i],
>  					obj->license,
>  					obj->kern_version);
> 

Re: [oss-drivers] Re: [PATCH bpf-next] libbpf: prevent overwriting of log_level in bpf_object__load_progs()

[Quentin Monnet]

2019-05-29 16:03 UTC+0200 ~ Daniel Borkmann <daniel@iogearbox.net>
> On 05/29/2019 11:23 AM, Quentin Monnet wrote:
>> There are two functions in libbpf that support passing a log_level
>> parameter for the verifier for loading programs:
>> bpf_object__load_xattr() and bpf_load_program_xattr(). Both accept an
>> attribute object containing the log_level, and apply it to the programs
>> to load.
>>
>> It turns out that to effectively load the programs, the latter function
>> eventually relies on the former. This was not taken into account when
>> adding support for log_level in bpf_object__load_xattr(), and the
>> log_level passed to bpf_load_program_xattr() later gets overwritten with
>> a zero value, thus disabling verifier logs for the program in all cases:
>>
>> bpf_load_program_xattr()             // prog.log_level = N;
> 
> I'm confused with your commit message. How can bpf_load_program_xattr()
> make sense here, this is the one doing the bpf syscall. Do you mean to
> say bpf_prog_load_xattr()? Because this one sets prog->log_level = attr->log_level
> and calls bpf_object__load() which in turn does bpf_object__load_xattr()
> with an attr that has attr->log_level of 0 such that bpf_object__load_progs()
> then overrides it. Unless I'm not missing something, please fix up this
> description properly and resubmit.

Ugh. Yeah, I mixed up bpf_load_program_xattr() and bpf_prog_load_xattr()
in the log, should be bpf_prog_load_xattr() everywhere instead, just as
you say. Apologies, I'll fix and resubmit.

Thanks,
Quentin