netdev - Re: net-next libbpf broken on prev kernel release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Dec 2017 10:52:19 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Eric Leblond <eric@...it.org>, Martin KaFai Lau <kafai@...com>,
        acme@...nel.org, ast@...com
Cc:     netdev@...r.kernel.org
Subject: Re: net-next libbpf broken on prev kernel release

[ +acme, +ast ]

On 12/14/2017 10:16 AM, Eric Leblond wrote:
> Hello,
> 
> It seems that the following patch did break libbpf (in net-next
> version) which is not able to load anymore a program on a 4.14:
> 
> tree 5096ddd73981e33a2164606461a45b56a189889c
> parent ad5b177bd73f5107d97c36f56395c4281fb6f089
> author Martin KaFai Lau <kafai@...com> Wed Sep 27 14:37:54 2017 -0700
> committer David S. Miller <davem@...emloft.net> Fri Sep 29 06:17:05 2017 +0100
> 
> bpf: libbpf: Provide basic API support to specify BPF obj name
> 
> The problem comes from
> 
> -int bpf_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
> -		     size_t insns_cnt, const char *license,
> -		     __u32 kern_version, char *log_buf, size_t log_buf_sz)
> +int bpf_load_program_name(enum bpf_prog_type type, const char *name,
> +			  const struct bpf_insn *insns,
> +			  size_t insns_cnt, const char *license,
> +			  __u32 kern_version, char *log_buf,
> +			  size_t log_buf_sz)
>  {
>  	int fd;
>  	union bpf_attr attr;
> +	__u32 name_len = name ? strlen(name) : 0;
>  
>  	bzero(&attr, sizeof(attr));
>  	attr.prog_type = type;
> @@ -130,6 +151,7 @@ int bpf_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
>  	attr.log_size = 0;
>  	attr.log_level = 0;
>  	attr.kern_version = kern_version;
> +	memcpy(attr.prog_name, name, min(name_len, BPF_OBJ_NAME_LEN - 1));
> 
> If I comment the memcpy then the eBPF program is loading correctly.
> 
> Is this a wanted behavior to have libbpf that needs to be in sync with
> kernel ? or should it be fixed ?

Yeah, this was reported recently here: https://lkml.org/lkml/2017/11/28/1246

I agree that given the policy of perf tool is to try to use new features
but if they fail on older kernels, then we should try to fallback whenever
that is feasible. I think for this specific case, we should in-fact fallback
and try w/o map/prog name in order to fix this regression for perf (or
other lib users).

Also agree that this cannot be done for every possible case like the mentioned
prog_ifindex field for offloading to NIC in the thread above, but I imho
the prog_ifindex is a slightly different situation given that a user needs
to specifically ask to offload via some provided API.

I think the fix should be: if a user *specifically* calls bpf_load_program_name()
or bpf_create_map_name() API from the lib, then the intention is very clear
that the bpf object should be created *with* name and otherwise fail. So a
fallback for these APIs to load w/o name would be inappropriate! But for the
existing code that used to load objects before, e.g. bpf_object__create_maps()
or bpf_program__load() it should try to use either mentioned bpf_*_name() APIs
and *iff* they fail, fall-back to the normal ones w/o name attribute. Meaning,
this kind of fall-back should be done, but not on a sys_bpf() layer but from
a higher PoV in the lib instead. I guess it would make sense to probe the
underlying kernel at startup and then based on its capabilities use one out
of the two APIs when we get there, such that we don't need to uselessly retry
APIs for each prog load.

Arnaldo, will there be a rework of your fix that we could route to bpf tree?

Thanks a lot,
Daniel