netdev - Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector

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, 8 Nov 2018 13:20:12 -0800
From:   Stanislav Fomichev <sdf@...ichev.me>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Quentin Monnet <quentin.monnet@...ronome.com>,
        Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        linux-kselftest@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, shuah@...nel.org, guro@...com,
        jiong.wang@...ronome.com, bhole_prashant_q7@....ntt.co.jp,
        john.fastabend@...il.com, jbenc@...hat.com,
        treeze.taeung@...il.com, yhs@...com, osk@...com,
        sandipan@...ux.vnet.ibm.com
Subject: Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector

On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 18:21:24 +0000, Quentin Monnet wrote:
> > >>> @@ -79,8 +82,11 @@ DESCRIPTION
> > >>>   		  contain a dot character ('.'), which is reserved for future
> > >>>   		  extensions of *bpffs*.
> > >>> -	**bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > >>> +	**bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > >>>   		  Load bpf program from binary *OBJ* and pin as *FILE*.
> > >>> +		  **bpftool prog load** will pin only the first bpf program
> > >>> +		  from the *OBJ*, **bpftool prog loadall** will pin all maps
> > >>> +		  and programs from the *OBJ*.  
> > >>
> > >> This could be improved regarding maps: with "bpftool prog load" I think we
> > >> also load and pin all maps, but your description implies this is only the
> > >> case with "loadall"  
> > > I don't think we pin any maps with `bpftool prog load`, we certainly load
> > > them, but we don't pin any afaict. Can you point me to the code where we
> > > pin the maps?
> > >   
> > 
> > My bad. I read "pin" but thought "load". It does not pin them indeed,
> > sorry about that.
> 
> Right, but I don't see much reason why prog loadall should pin maps.
It does seem convenient to have an option to pin everything, so we
don't require anything else to find the ids of the maps.
If we are pinning all progs, might as well pin the maps, why not? See
my example in the commit message, for example, where I just hard code
the expected map name. Convenient :-)

> The reason to pin program(s) is to hold some reference and to be able
> to find them.  Since we have the programs pinned we should be able to
> find their maps with relative ease.
> 
> $ bpftool prog show pinned /sys/fs/bpf/prog
> 7: cgroup_skb  tag 2a142ef67aaad174  gpl
> 	loaded_at 2018-11-08T11:02:25-0800  uid 0
> 	xlated 296B  jited 229B  memlock 4096B  map_ids 6,7
> 
> possibly:
> 
> $ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
> 6
> 
> Moreover, I think program and map names may collide making ELFs
> unloadable..
It doesn't sound like a big problem, sounds like a constraint we can live
with.