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:51:36 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
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 Thu, 8 Nov 2018 13:20:12 -0800, Stanislav Fomichev wrote:
> 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 :-)

Sure.  I can see how its convenient to your use case.  A lot of people
will be very used to finding out the maps because if program is loaded
with iproute2 tools neither programs nor maps get pinned.

Please add a "pinmaps MAP_DIR" optional parameter as a separate patch.

> > 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.