Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On Wed, 7 Nov 2018 15:12:07 -0800, Stanislav Fomichev wrote: > > > I agree, constructing the jmp_table is a bit fragile with all the > > > dependencies on the order of the progs. I'll drop that and will send a > > > v2 that pins all the programs from the obj file instead and offloads > > > jmp_table construction to the user. So the supposed use case would be > > > something like the following: > > > > > > bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector > > > > Okay. One more thing - how do we differentiate between mass pin and the > > existing pin first behaviour? Should we perhaps add a loadall command > > or some flag? > In v2 I did by program type: > * flow_dissector -> pin all > * not flow_dissector -> pin first? > > But we can have loadall or something like: > load OBJ [pinfirst|pinall] FILE|DIR [type TYPE] > > If we want to add user control, I'd go with loadall command, > adding more optional flags in between is a mess.. I think user control would be good. Agreed on loadall being better.
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 14:15:06 -0800, Stanislav Fomichev wrote:
> > On 11/07, Quentin Monnet wrote:
> > > 2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski
> > >
> > > > On Wed, 7 Nov 2018 20:08:53 +, Quentin Monnet wrote:
> > > > > > + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
> > > > > > + if (err) {
> > > > > > + p_err("failed to pin program %s",
> > > > > > + bpf_program__title(prog, false));
> > > > > > + goto err_close_obj;
> > > > > > + }
> > > > >
> > > > > I don't have the same opinion as Jakub for pinning :). I was hoping we
> > > > > could also load additional programs (for tail calls) for
> > > > > non-flow_dissector programs. Could this be an occasion to update the
> > > > > code in that direction?
> > > >
> > > > Do you mean having the bpftool construct an array for tail calling
> > > > automatically when loading an object? Or do a "mass pin" of all
> > > > programs in an object file?
> > > >
> > > > I'm not convinced about this strategy of auto assembling a tail call
> > > > array by assuming that a flow dissector object carries programs for
> > > > protocols in order (apart from the main program which doesn't have to
> > > > be first, for some reason).
> > >
> > > Not constructing the prog array, I don't think this should be the role of
> > > bpftool either. Much more a "mass pin", so that you have a link to each
> > > program loaded from the object file and can later add them to a prog array
> > > map with subsequent calls to bpftool.
>
> Makes sense, specific files named after index or program name/title?
> Program name may be nicer?
>
> > I agree, constructing the jmp_table is a bit fragile with all the
> > dependencies on the order of the progs. I'll drop that and will send a
> > v2 that pins all the programs from the obj file instead and offloads
> > jmp_table construction to the user. So the supposed use case would be
> > something like the following:
> >
> > bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector
>
> Okay. One more thing - how do we differentiate between mass pin and the
> existing pin first behaviour? Should we perhaps add a loadall command
> or some flag?
In v2 I did by program type:
* flow_dissector -> pin all
* not flow_dissector -> pin first?
But we can have loadall or something like:
load OBJ [pinfirst|pinall] FILE|DIR [type TYPE]
If we want to add user control, I'd go with loadall command,
adding more optional flags in between is a mess..
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key ... value pinned /sys/fs/bpf/flow//0
>
> Interesting, why $dir/$title/0 ? Perhaps $dir/$title is sufficient?
That's how bpf_program__pin does the pinning, for each program instance.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744
>
> > bpftool map update ...
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
>
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On Wed, 7 Nov 2018 14:15:06 -0800, Stanislav Fomichev wrote:
> On 11/07, Quentin Monnet wrote:
> > 2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski
> > > On Wed, 7 Nov 2018 20:08:53 +, Quentin Monnet wrote:
> > > > > + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
> > > > > + if (err) {
> > > > > + p_err("failed to pin program %s",
> > > > > + bpf_program__title(prog, false));
> > > > > + goto err_close_obj;
> > > > > + }
> > > >
> > > > I don't have the same opinion as Jakub for pinning :). I was hoping we
> > > > could also load additional programs (for tail calls) for
> > > > non-flow_dissector programs. Could this be an occasion to update the
> > > > code in that direction?
> > >
> > > Do you mean having the bpftool construct an array for tail calling
> > > automatically when loading an object? Or do a "mass pin" of all
> > > programs in an object file?
> > >
> > > I'm not convinced about this strategy of auto assembling a tail call
> > > array by assuming that a flow dissector object carries programs for
> > > protocols in order (apart from the main program which doesn't have to
> > > be first, for some reason).
> >
> > Not constructing the prog array, I don't think this should be the role of
> > bpftool either. Much more a "mass pin", so that you have a link to each
> > program loaded from the object file and can later add them to a prog array
> > map with subsequent calls to bpftool.
Makes sense, specific files named after index or program name/title?
Program name may be nicer?
> I agree, constructing the jmp_table is a bit fragile with all the
> dependencies on the order of the progs. I'll drop that and will send a
> v2 that pins all the programs from the obj file instead and offloads
> jmp_table construction to the user. So the supposed use case would be
> something like the following:
>
> bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector
Okay. One more thing - how do we differentiate between mass pin and the
existing pin first behaviour? Should we perhaps add a loadall command
or some flag?
> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> key ... value pinned /sys/fs/bpf/flow//0
Interesting, why $dir/$title/0 ? Perhaps $dir/$title is sufficient?
> bpftool map update ...
> bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On 11/07, Quentin Monnet wrote:
> 2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski
> > On Wed, 7 Nov 2018 20:08:53 +, Quentin Monnet wrote:
> > > > + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
> > > > + if (err) {
> > > > + p_err("failed to pin program %s",
> > > > + bpf_program__title(prog, false));
> > > > + goto err_close_obj;
> > > > + }
> > >
> > > I don't have the same opinion as Jakub for pinning :). I was hoping we
> > > could also load additional programs (for tail calls) for
> > > non-flow_dissector programs. Could this be an occasion to update the
> > > code in that direction?
> >
> > Do you mean having the bpftool construct an array for tail calling
> > automatically when loading an object? Or do a "mass pin" of all
> > programs in an object file?
> >
> > I'm not convinced about this strategy of auto assembling a tail call
> > array by assuming that a flow dissector object carries programs for
> > protocols in order (apart from the main program which doesn't have to
> > be first, for some reason).
>
> Not constructing the prog array, I don't think this should be the role of
> bpftool either. Much more a "mass pin", so that you have a link to each
> program loaded from the object file and can later add them to a prog array
> map with subsequent calls to bpftool.
I agree, constructing the jmp_table is a bit fragile with all the
dependencies on the order of the progs. I'll drop that and will send a
v2 that pins all the programs from the obj file instead and offloads
jmp_table construction to the user. So the supposed use case would be
something like the following:
bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key ... value pinned /sys/fs/bpf/flow//0
bpftool map update ...
bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski
On Wed, 7 Nov 2018 20:08:53 +, Quentin Monnet wrote:
+ err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
+ if (err) {
+ p_err("failed to pin program %s",
+ bpf_program__title(prog, false));
+ goto err_close_obj;
+ }
I don't have the same opinion as Jakub for pinning :). I was hoping we
could also load additional programs (for tail calls) for
non-flow_dissector programs. Could this be an occasion to update the
code in that direction?
Do you mean having the bpftool construct an array for tail calling
automatically when loading an object? Or do a "mass pin" of all
programs in an object file?
I'm not convinced about this strategy of auto assembling a tail call
array by assuming that a flow dissector object carries programs for
protocols in order (apart from the main program which doesn't have to
be first, for some reason).
Not constructing the prog array, I don't think this should be the role
of bpftool either. Much more a "mass pin", so that you have a link to
each program loaded from the object file and can later add them to a
prog array map with subsequent calls to bpftool.
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On 11/07, Quentin Monnet wrote:
> Hi Stanislav,
>
> 2018-11-07 11:35 UTC-0800 ~ Stanislav Fomichev
> > This commit adds support for loading/attaching/detaching flow
> > dissector program. The structure of the flow dissector program is
> > assumed to be the same as in the selftests:
> >
> > * flow_dissector section with the main entry point
> > * a bunch of tail call progs
> > * a jmp_table map that is populated with the tail call progs
> >
> > When `bpftool load` is called with a flow_dissector prog (i.e. when the
> > first section is flow_dissector of 'type flow_dissector' argument is
> > passed), we load and pin all the programs and build the jump table.
> >
> > The last argument of `bpftool attach` is made optional for this use
> > case.
> >
> > Example:
> > bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> > /sys/fs/bpf/flow type flow_dissector
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> >
> > Tested by using the above two lines to load the prog in
> > the test_flow_dissector.sh selftest.
> >
> > Signed-off-by: Stanislav Fomichev
> > ---
> > .../bpftool/Documentation/bpftool-prog.rst| 16 ++-
> > tools/bpf/bpftool/common.c| 32 +++--
> > tools/bpf/bpftool/main.h | 1 +
> > tools/bpf/bpftool/prog.c | 135 +++---
> > 4 files changed, 141 insertions(+), 43 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index ac4e904b10fb..3caa9153435b 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -25,8 +25,8 @@ MAP COMMANDS
> > | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
> > **opcodes**}]
> > | **bpftool** **prog pin** *PROG* *FILE*
> > | **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
> > {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> > | **bpftool** **prog help**
> > |
> > | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > @@ -39,7 +39,9 @@ MAP COMMANDS
> > | **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
> > **cgroup/post_bind6** |
> > | **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
> > | **cgroup/sendmsg6**
> > | }
> > -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** |
> > **skb_parse** }
> > +| *ATTACH_TYPE* := {
> > +| | **msg_verdict** | **skb_verdict** | **skb_parse** |
> > **flow_dissector**
>
> ^
> Nitpick: Could you please remove the above pipe?
Ah, I missed that one, my bad, thanks.
> > +| }
> >
> >
> > DESCRIPTION
> > @@ -97,13 +99,13 @@ DESCRIPTION
> > contain a dot character ('.'), which is reserved for future
> > extensions of *bpffs*.
> >
> > -**bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > +**bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> >Attach bpf program *PROG* (with type specified by
> > *ATTACH_TYPE*)
> > - to the map *MAP*.
> > + to the optional map *MAP*.
> >
> > -**bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +**bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> >Detach bpf program *PROG* (with type specified by
> > *ATTACH_TYPE*)
> > - from the map *MAP*.
> > + from the optional map *MAP*.
> >
> > **bpftool prog help**
> > Print short help message.
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 25af85304ebe..963881142dfb 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -169,34 +169,24 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type
> > exp_type)
> > return fd;
> > }
> >
> > -int do_pin_fd(int fd, const char *name)
> > +int mount_bpffs_for_pin(const char *name)
> > {
> > char err_str[ERR_MAX_LEN];
> > char *file;
> > char *dir;
> > int err = 0;
> >
> > - err = bpf_obj_pin(fd, name);
> > - if (!err)
> > - goto out;
> > -
> > file = malloc(strlen(name) + 1);
> > strcpy(file, name);
> > dir = dirname(file);
> >
> > - if (errno != EPERM || is_bpffs(dir)) {
> > - p_err("can't pin the object (%s): %s", name, strerror(errno));
> > + if (is_bpffs(dir)) {
> > + /* nothing to do if already mounted */
> > goto out_free;
> > }
> >
> > - /* Attempt to mount bpffs, then retry pinning. */
> > err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
> > - if (!err) {
>
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On Wed, 7 Nov 2018 13:00:09 -0800, Stanislav Fomichev wrote:
> > > + }
> > > +
> > > + if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > > + err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
> > > + if (err) {
> > > + p_err("failed to build flow dissector jump table");
> > > + goto err_close_obj;
> > > + }
> > > + /* flow dissector consist of multiple programs,
> > > + * we want to pin them all
> >
> > Why pin them all shouldn't the main program be the only one pinned?
> If I pin only the main program, the tail ones disappear from the jmp_table map
> when bpftool exits.
> Am I missing something?
> Should BPF_MAP_TYPE_PROG_ARRAY hold the referenced progs?
It does.
# bpftool map create /sys/fs/bpf/map \
type prog_array key 4 value 4 entries 4 \
name tail_call_map
# bpftool prog load perf_event_output_stack.o /sys/fs/bpf/prog \
type xdp
# bpftool map update pinned /sys/fs/bpf/map \
key 0 0 0 0 \
value pinned /sys/fs/bpf/prog
# bpftool prog
11: xdp name xdp_prog1 tag 6f1b482b27443edf gpl
loaded_at 2018-11-07T13:09:20-0800 uid 0
xlated 144B jited 130B memlock 4096B map_ids 14
# rm /sys/fs/bpf/prog
# bpftool prog
11: xdp name xdp_prog1 tag 6f1b482b27443edf gpl
loaded_at 2018-11-07T13:09:20-0800 uid 0
xlated 144B jited 130B memlock 4096B map_ids 14
# rm /sys/fs/bpf/map
# bpftool prog
# bpftool prog show id 11
Error: get by id (11): No such file or directory
I think we should remove all this auto tail call construction unless
we have solid annotations in the elf file which can clearly guide the
loader.
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 11:35:52 -0800, Stanislav Fomichev wrote:
> > This commit adds support for loading/attaching/detaching flow
> > dissector program. The structure of the flow dissector program is
> > assumed to be the same as in the selftests:
> >
> > * flow_dissector section with the main entry point
> > * a bunch of tail call progs
> > * a jmp_table map that is populated with the tail call progs
> >
> > When `bpftool load` is called with a flow_dissector prog (i.e. when the
> > first section is flow_dissector of 'type flow_dissector' argument is
> > passed), we load and pin all the programs and build the jump table.
> >
> > The last argument of `bpftool attach` is made optional for this use
> > case.
> >
> > Example:
> > bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> > /sys/fs/bpf/flow type flow_dissector
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> >
> > Tested by using the above two lines to load the prog in
> > the test_flow_dissector.sh selftest.
> >
> > Signed-off-by: Stanislav Fomichev
> > ---
> > .../bpftool/Documentation/bpftool-prog.rst| 16 ++-
> > tools/bpf/bpftool/common.c| 32 +++--
> > tools/bpf/bpftool/main.h | 1 +
> > tools/bpf/bpftool/prog.c | 135 +++---
>
> Please add the new attach type to bash completions.
Thanks for a quick review! Will address everything in the v2.
Answered some of your questions below.
> > 4 files changed, 141 insertions(+), 43 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index ac4e904b10fb..3caa9153435b 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -25,8 +25,8 @@ MAP COMMANDS
> > | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
> > **opcodes**}]
> > | **bpftool** **prog pin** *PROG* *FILE*
> > | **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
> > {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> > | **bpftool** **prog help**
> > |
> > | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > @@ -39,7 +39,9 @@ MAP COMMANDS
> > | **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
> > **cgroup/post_bind6** |
> > | **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
> > | **cgroup/sendmsg6**
> > | }
> > -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** |
> > **skb_parse** }
> > +| *ATTACH_TYPE* := {
> > +| | **msg_verdict** | **skb_verdict** | **skb_parse** |
> > **flow_dissector**
> > +| }
> >
> >
> > DESCRIPTION
> > @@ -97,13 +99,13 @@ DESCRIPTION
> > contain a dot character ('.'), which is reserved for future
> > extensions of *bpffs*.
> >
> > -**bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > +**bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> >Attach bpf program *PROG* (with type specified by
> > *ATTACH_TYPE*)
> > - to the map *MAP*.
> > + to the optional map *MAP*.
>
> Perhaps we can do better on help? Attach BPF program *PROG* (with type
> specified by *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
> parameter, with the exception of *flow_dissector* which is attached to
> current networking name space.
>
> > -**bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +**bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> >Detach bpf program *PROG* (with type specified by
> > *ATTACH_TYPE*)
> > - from the map *MAP*.
> > + from the optional map *MAP*.
> >
> > **bpftool prog help**
> > Print short help message.
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 25af85304ebe..963881142dfb 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
>
> > @@ -204,10 +194,22 @@ int do_pin_fd(int fd, const char *name)
> >
> > out_free:
> > free(file);
> > -out:
> > return err;
> > }
> >
> > +int do_pin_fd(int fd, const char *name)
> > +{
> > + int err = mount_bpffs_for_pin(name);
>
> Please don't initialize the error variable with a non-trivial function
> call.
>
> > + if (err) {
> > + p_err("can't mount bpffs for pin %s: %s",
> > + name, strerror(errno));
>
> I think mount_bpffs_for_pin() will already print an error. We can't
> print two errors, because it will break JSON output.
>
> > +
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On Wed, 7 Nov 2018 20:08:53 +, Quentin Monnet wrote:
> > + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
> > + if (err) {
> > + p_err("failed to pin program %s",
> > + bpf_program__title(prog, false));
> > + goto err_close_obj;
> > + }
>
> I don't have the same opinion as Jakub for pinning :). I was hoping we
> could also load additional programs (for tail calls) for
> non-flow_dissector programs. Could this be an occasion to update the
> code in that direction?
Do you mean having the bpftool construct an array for tail calling
automatically when loading an object? Or do a "mass pin" of all
programs in an object file?
I'm not convinced about this strategy of auto assembling a tail call
array by assuming that a flow dissector object carries programs for
protocols in order (apart from the main program which doesn't have to
be first, for some reason).
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
Hi Stanislav,
2018-11-07 11:35 UTC-0800 ~ Stanislav Fomichev
> This commit adds support for loading/attaching/detaching flow
> dissector program. The structure of the flow dissector program is
> assumed to be the same as in the selftests:
>
> * flow_dissector section with the main entry point
> * a bunch of tail call progs
> * a jmp_table map that is populated with the tail call progs
>
> When `bpftool load` is called with a flow_dissector prog (i.e. when the
> first section is flow_dissector of 'type flow_dissector' argument is
> passed), we load and pin all the programs and build the jump table.
>
> The last argument of `bpftool attach` is made optional for this use
> case.
>
> Example:
> bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> /sys/fs/bpf/flow type flow_dissector
> bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
>
> Tested by using the above two lines to load the prog in
> the test_flow_dissector.sh selftest.
>
> Signed-off-by: Stanislav Fomichev
> ---
> .../bpftool/Documentation/bpftool-prog.rst| 16 ++-
> tools/bpf/bpftool/common.c| 32 +++--
> tools/bpf/bpftool/main.h | 1 +
> tools/bpf/bpftool/prog.c | 135 +++---
> 4 files changed, 141 insertions(+), 43 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index ac4e904b10fb..3caa9153435b 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -25,8 +25,8 @@ MAP COMMANDS
> |**bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
> **opcodes**}]
> |**bpftool** **prog pin** *PROG* *FILE*
> |**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
> {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> |**bpftool** **prog help**
> |
> |*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> @@ -39,7 +39,9 @@ MAP COMMANDS
> |**cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
> **cgroup/post_bind6** |
> |**cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
> | **cgroup/sendmsg6**
> |}
> -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse**
> }
> +| *ATTACH_TYPE* := {
> +|| **msg_verdict** | **skb_verdict** | **skb_parse** |
> **flow_dissector**
^
Nitpick: Could you please remove the above pipe?
> +|}
>
>
> DESCRIPTION
> @@ -97,13 +99,13 @@ DESCRIPTION
> contain a dot character ('.'), which is reserved for future
> extensions of *bpffs*.
>
> -**bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> +**bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>Attach bpf program *PROG* (with type specified by
> *ATTACH_TYPE*)
> - to the map *MAP*.
> + to the optional map *MAP*.
>
> -**bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> +**bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>Detach bpf program *PROG* (with type specified by
> *ATTACH_TYPE*)
> - from the map *MAP*.
> + from the optional map *MAP*.
>
> **bpftool prog help**
> Print short help message.
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 25af85304ebe..963881142dfb 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -169,34 +169,24 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type
> exp_type)
> return fd;
> }
>
> -int do_pin_fd(int fd, const char *name)
> +int mount_bpffs_for_pin(const char *name)
> {
> char err_str[ERR_MAX_LEN];
> char *file;
> char *dir;
> int err = 0;
>
> - err = bpf_obj_pin(fd, name);
> - if (!err)
> - goto out;
> -
> file = malloc(strlen(name) + 1);
> strcpy(file, name);
> dir = dirname(file);
>
> - if (errno != EPERM || is_bpffs(dir)) {
> - p_err("can't pin the object (%s): %s", name, strerror(errno));
> + if (is_bpffs(dir)) {
> + /* nothing to do if already mounted */
> goto out_free;
> }
>
> - /* Attempt to mount bpffs, then retry pinning. */
> err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
> - if (!err) {
> - err = bpf_obj_pin(fd, name);
> - if (err)
> - p_err("can't pin the object (%s): %s", name,
> - strerror(errno));
> - } else {
> + if (err) {
>
Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
On Wed, 7 Nov 2018 11:35:52 -0800, Stanislav Fomichev wrote:
> This commit adds support for loading/attaching/detaching flow
> dissector program. The structure of the flow dissector program is
> assumed to be the same as in the selftests:
>
> * flow_dissector section with the main entry point
> * a bunch of tail call progs
> * a jmp_table map that is populated with the tail call progs
>
> When `bpftool load` is called with a flow_dissector prog (i.e. when the
> first section is flow_dissector of 'type flow_dissector' argument is
> passed), we load and pin all the programs and build the jump table.
>
> The last argument of `bpftool attach` is made optional for this use
> case.
>
> Example:
> bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> /sys/fs/bpf/flow type flow_dissector
> bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
>
> Tested by using the above two lines to load the prog in
> the test_flow_dissector.sh selftest.
>
> Signed-off-by: Stanislav Fomichev
> ---
> .../bpftool/Documentation/bpftool-prog.rst| 16 ++-
> tools/bpf/bpftool/common.c| 32 +++--
> tools/bpf/bpftool/main.h | 1 +
> tools/bpf/bpftool/prog.c | 135 +++---
Please add the new attach type to bash completions.
> 4 files changed, 141 insertions(+), 43 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index ac4e904b10fb..3caa9153435b 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -25,8 +25,8 @@ MAP COMMANDS
> |**bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
> **opcodes**}]
> |**bpftool** **prog pin** *PROG* *FILE*
> |**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
> {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> |**bpftool** **prog help**
> |
> |*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> @@ -39,7 +39,9 @@ MAP COMMANDS
> |**cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
> **cgroup/post_bind6** |
> |**cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
> | **cgroup/sendmsg6**
> |}
> -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse**
> }
> +| *ATTACH_TYPE* := {
> +|| **msg_verdict** | **skb_verdict** | **skb_parse** |
> **flow_dissector**
> +|}
>
>
> DESCRIPTION
> @@ -97,13 +99,13 @@ DESCRIPTION
> contain a dot character ('.'), which is reserved for future
> extensions of *bpffs*.
>
> -**bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> +**bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>Attach bpf program *PROG* (with type specified by
> *ATTACH_TYPE*)
> - to the map *MAP*.
> + to the optional map *MAP*.
Perhaps we can do better on help? Attach BPF program *PROG* (with type
specified by *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
parameter, with the exception of *flow_dissector* which is attached to
current networking name space.
> -**bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> +**bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>Detach bpf program *PROG* (with type specified by
> *ATTACH_TYPE*)
> - from the map *MAP*.
> + from the optional map *MAP*.
>
> **bpftool prog help**
> Print short help message.
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 25af85304ebe..963881142dfb 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -204,10 +194,22 @@ int do_pin_fd(int fd, const char *name)
>
> out_free:
> free(file);
> -out:
> return err;
> }
>
> +int do_pin_fd(int fd, const char *name)
> +{
> + int err = mount_bpffs_for_pin(name);
Please don't initialize the error variable with a non-trivial function
call.
> + if (err) {
> + p_err("can't mount bpffs for pin %s: %s",
> + name, strerror(errno));
I think mount_bpffs_for_pin() will already print an error. We can't
print two errors, because it will break JSON output.
> + return err;
> + }
> +
> + return bpf_obj_pin(fd, name);
> +}
> +
> int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
> {
> unsigned int id;
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 5302ee282409..f3a07ec3a444 100644
> --- a/tools/bpf/bpftool/prog.c
> +++
[PATCH bpf-next 2/2] bpftool: support loading flow dissector
This commit adds support for loading/attaching/detaching flow
dissector program. The structure of the flow dissector program is
assumed to be the same as in the selftests:
* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs
When `bpftool load` is called with a flow_dissector prog (i.e. when the
first section is flow_dissector of 'type flow_dissector' argument is
passed), we load and pin all the programs and build the jump table.
The last argument of `bpftool attach` is made optional for this use
case.
Example:
bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
/sys/fs/bpf/flow type flow_dissector
bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
Tested by using the above two lines to load the prog in
the test_flow_dissector.sh selftest.
Signed-off-by: Stanislav Fomichev
---
.../bpftool/Documentation/bpftool-prog.rst| 16 ++-
tools/bpf/bpftool/common.c| 32 +++--
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/prog.c | 135 +++---
4 files changed, 141 insertions(+), 43 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index ac4e904b10fb..3caa9153435b 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -25,8 +25,8 @@ MAP COMMANDS
| **bpftool** **prog dump jited** *PROG* [{**file** *FILE* |
**opcodes**}]
| **bpftool** **prog pin** *PROG* *FILE*
| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map**
{**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
-| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
-| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
+| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -39,7 +39,9 @@ MAP COMMANDS
| **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** |
**cgroup/post_bind6** |
| **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4**
| **cgroup/sendmsg6**
| }
-| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
+| *ATTACH_TYPE* := {
+| | **msg_verdict** | **skb_verdict** | **skb_parse** |
**flow_dissector**
+| }
DESCRIPTION
@@ -97,13 +99,13 @@ DESCRIPTION
contain a dot character ('.'), which is reserved for future
extensions of *bpffs*.
-**bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
+**bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
Attach bpf program *PROG* (with type specified by
*ATTACH_TYPE*)
- to the map *MAP*.
+ to the optional map *MAP*.
-**bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
+**bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
Detach bpf program *PROG* (with type specified by
*ATTACH_TYPE*)
- from the map *MAP*.
+ from the optional map *MAP*.
**bpftool prog help**
Print short help message.
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 25af85304ebe..963881142dfb 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -169,34 +169,24 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type
exp_type)
return fd;
}
-int do_pin_fd(int fd, const char *name)
+int mount_bpffs_for_pin(const char *name)
{
char err_str[ERR_MAX_LEN];
char *file;
char *dir;
int err = 0;
- err = bpf_obj_pin(fd, name);
- if (!err)
- goto out;
-
file = malloc(strlen(name) + 1);
strcpy(file, name);
dir = dirname(file);
- if (errno != EPERM || is_bpffs(dir)) {
- p_err("can't pin the object (%s): %s", name, strerror(errno));
+ if (is_bpffs(dir)) {
+ /* nothing to do if already mounted */
goto out_free;
}
- /* Attempt to mount bpffs, then retry pinning. */
err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
- if (!err) {
- err = bpf_obj_pin(fd, name);
- if (err)
- p_err("can't pin the object (%s): %s", name,
- strerror(errno));
- } else {
+ if (err) {
err_str[ERR_MAX_LEN - 1] = '\0';
p_err("can't mount BPF file system to pin the object (%s): %s",
name, err_str);
@@ -204,10 +194,22 @@ int do_pin_fd(int fd, const char *name)
out_free:
free(file);
-out:
return err;
}
+int do_pin_fd(int fd, const
