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]

Message-ID: <20171214142809.GF10463@kernel.org>
Date:   Thu, 14 Dec 2017 11:28:09 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Eric Leblond <eric@...it.org>, Martin KaFai Lau <kafai@...com>,
        ast@...com, netdev@...r.kernel.org
Subject: Re: net-next libbpf broken on prev kernel release

Em Thu, Dec 14, 2017 at 10:52:19AM +0100, Daniel Borkmann escreveu:
> [ +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.

tools/perf/ has:

static struct {
        bool sample_id_all;
        bool exclude_guest;
        bool mmap2;
        bool cloexec;
        bool clockid;
        bool clockid_wrong;
        bool lbr_flags;
        bool write_backward;
        bool group_read;
} perf_missing_features;

When the user request something that needs some of these features we try
using it, failing it will mark it as missing and then other events will
not needlessly try using it, i.e. we don't do it at program start, we
leave that to when we actually need it, to avoid uselessly probing at
startup.

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

I'm resuming work on it after I get my current batch tested and
submitted, will reboot with an older kernel and follow your suggestions,
that seems to match Alexei's and Martin's, my patch was just a RFC to
show that we need a fallback for older kernels.

I needed to move on, so I updated my machine to a kernel where interlock
of tools/ with the kernel happens and it worked, so I left this to see
if someone else complained or if I was being too picky. :-)

- Arnaldo