netdev - Re: [PATCH bpf-next 0/5] Introduce libbpf_attach_type_by

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: <20180926190630.0c4af18a@cakuba.netronome.com>
Date:   Wed, 26 Sep 2018 19:06:30 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Andrey Ignatov <rdna@...com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 0/5] Introduce libbpf_attach_type_by_name

On Wed, 26 Sep 2018 23:54:17 +0000, Andrey Ignatov wrote:
> Jakub Kicinski <jakub.kicinski@...ronome.com> [Wed, 2018-09-26 16:20 -0700]:
> > On Wed, 26 Sep 2018 15:24:52 -0700, Andrey Ignatov wrote:  
> > > This patch set introduces libbpf_attach_type_by_name function in libbpf to
> > > identify attach type by section name.
> > > 
> > > This is useful to avoid writing same logic over and over again in user
> > > space applications that leverage libbpf.
> > > 
> > > Patch 1 has more details on the new function and problem being solved.
> > > Patches 2 and 3 add support for new section names.
> > > Patch 4 uses new function in a selftest.
> > > Patch 5 adds selftest for libbpf_{prog,attach}_type_by_name.
> > > 
> > > As a side note there are a lot of inconsistencies now between names used by
> > > libbpf and bpftool (e.g. cgroup/skb vs cgroup_skb, cgroup_device and device
> > > vs cgroup/dev, sockops vs sock_ops, etc). This patch set does not address
> > > it but it tries not to make it harder to address it in the future.  
> > 
> > I was wondering a few times whether I should point it out to people
> > during review, but thought it would be nit picking.  Maybe we should be
> > more strict.
> > 
> > Your series LGTM!  
> 
> Thanks for review!
> 
> IMO having it consistent would be great, e.g. one writes a program with
> section name X and bpftool shows/accepts it in exactly same way in all
> its sub-commands (w/o maybe custom suffix added by program writer).
> 
> But I doubt that keeping a few places in sync manually will work long
> term since it's easy to miss such a thing.
> 
> What do you think of having one source of truth in libbpf so that a
> string for prog_type or attach_type is defined once and all other places
> (e.g. bpftool prog show, bpftool cgroup show) use only corresponding
> enum-s to get those strings, but don't introduce any new strings?
>
> Keeping already existing names in a backward compatible way is a pain
> though.

One source of truth would be nice IMO.  The backward compat ties our
hands a tiny bit, but we could do a fallback strategy, where bpftool
checks if it has a name defined for a type, and if it doesn't (for new
types) it will ask libbpf.

There is another place actually where names are hard coded - for program
load command we use the types as they appear in libbpf already.  And
that requires putting them in help, man page and bash completions, too.

> Another thing, I was wondering, is if there is a way to bypass strings
> completely (at least in libbpf, since bpftool still has to print
> human-readable names) and keep actual bpf_prog_type and bpf_attach_type
> as metadata for a program in ELF file. Maybe some compiler magic ..

Possibly.  I'm far from an ELF expert, but I think attaching arbitrary
metadata to sections may be harder, name is already there and is free
form.