diff mbox series

[bpf-next,11/11] tools: bpftool: allow reuse of maps with bpftool prog load

Message ID 20180704025417.8848-12-jakub.kicinski@netronome.com
State Changes Requested, archived
Delegated to: BPF Maintainers
Headers show
Series tools: bpf: extend bpftool prog load | expand

Commit Message

Jakub Kicinski July 4, 2018, 2:54 a.m. UTC
Add map parameter to prog load which will allow reuse of existing
maps instead of creating new ones.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 .../bpftool/Documentation/bpftool-prog.rst    |  20 ++-
 tools/bpf/bpftool/bash-completion/bpftool     |  67 +++++++-
 tools/bpf/bpftool/main.h                      |   3 +
 tools/bpf/bpftool/map.c                       |   4 +-
 tools/bpf/bpftool/prog.c                      | 148 ++++++++++++++++--
 5 files changed, 219 insertions(+), 23 deletions(-)

Comments

Daniel Borkmann July 5, 2018, 8:35 a.m. UTC | #1
On 07/04/2018 04:54 AM, Jakub Kicinski wrote:
> Add map parameter to prog load which will allow reuse of existing
> maps instead of creating new ones.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
[...]
> +
> +			fd = map_parse_fd(&argc, &argv);
> +			if (fd < 0)
> +				goto err_free_reuse_maps;
> +
> +			map_replace = reallocarray(map_replace, old_map_fds + 1,
> +						   sizeof(*map_replace));
> +			if (!map_replace) {
> +				p_err("mem alloc failed");
> +				goto err_free_reuse_maps;

Series in general looks good to me. However, above reallocarray() doesn't
exist and hence build fails, please see below. Is that from newest glibc?

You probably need some fallback implementation or in general have something
bpftool internal that doesn't make a bet on its availability.

# make

Auto-detecting system features:
...                        libbfd: [ on  ]
...        disassembler-four-args: [ OFF ]

  CC       bpf_jit_disasm.o
  LINK     bpf_jit_disasm
  CC       bpf_dbg.o
  LINK     bpf_dbg
  CC       bpf_asm.o
  BISON    bpf_exp.yacc.c
  CC       bpf_exp.yacc.o
  FLEX     bpf_exp.lex.c
  CC       bpf_exp.lex.o
  LINK     bpf_asm
  DESCEND  bpftool

Auto-detecting system features:
...                        libbfd: [ on  ]
...        disassembler-four-args: [ OFF ]

  CC       map_perf_ring.o
  CC       xlated_dumper.o
  CC       perf.o
  CC       prog.o
prog.c: In function ‘do_load’:
prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ [-Wimplicit-function-declaration]
    map_replace = reallocarray(map_replace, old_map_fds + 1,
                  ^~~~~~~~~~~~
prog.c:785:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
    map_replace = reallocarray(map_replace, old_map_fds + 1,
                ^
  CC       common.o
  CC       cgroup.o
  CC       main.o
  CC       json_writer.o
  CC       cfg.o
  CC       map.o
  CC       jit_disasm.o
  CC       disasm.o

Auto-detecting system features:
...                        libelf: [ on  ]
...                           bpf: [ on  ]

Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h'
  CC       libbpf.o
  CC       bpf.o
  CC       nlattr.o
  CC       btf.o
  LD       libbpf-in.o
  LINK     libbpf.a
  LINK     bpftool
prog.o: In function `do_load':
prog.c:(.text+0x23d): undefined reference to `reallocarray'
collect2: error: ld returned 1 exit status
Makefile:89: recipe for target 'bpftool' failed
make[1]: *** [bpftool] Error 1
Makefile:99: recipe for target 'bpftool' failed
make: *** [bpftool] Error 2

Thanks,
Daniel
Jakub Kicinski July 5, 2018, 10:57 p.m. UTC | #2
On Thu, 5 Jul 2018 10:35:24 +0200, Daniel Borkmann wrote:
> On 07/04/2018 04:54 AM, Jakub Kicinski wrote:
> > Add map parameter to prog load which will allow reuse of existing
> > maps instead of creating new ones.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>  
> [...]
> > +
> > +			fd = map_parse_fd(&argc, &argv);
> > +			if (fd < 0)
> > +				goto err_free_reuse_maps;
> > +
> > +			map_replace = reallocarray(map_replace, old_map_fds + 1,
> > +						   sizeof(*map_replace));
> > +			if (!map_replace) {
> > +				p_err("mem alloc failed");
> > +				goto err_free_reuse_maps;  
> 
> Series in general looks good to me. However, above reallocarray() doesn't
> exist and hence build fails, please see below. Is that from newest glibc?
> 
> You probably need some fallback implementation or in general have something
> bpftool internal that doesn't make a bet on its availability.
> 
> # make
> 
> Auto-detecting system features:
> ...                        libbfd: [ on  ]
> ...        disassembler-four-args: [ OFF ]
> 
>   CC       bpf_jit_disasm.o
>   LINK     bpf_jit_disasm
>   CC       bpf_dbg.o
>   LINK     bpf_dbg
>   CC       bpf_asm.o
>   BISON    bpf_exp.yacc.c
>   CC       bpf_exp.yacc.o
>   FLEX     bpf_exp.lex.c
>   CC       bpf_exp.lex.o
>   LINK     bpf_asm
>   DESCEND  bpftool
> 
> Auto-detecting system features:
> ...                        libbfd: [ on  ]
> ...        disassembler-four-args: [ OFF ]
> 
>   CC       map_perf_ring.o
>   CC       xlated_dumper.o
>   CC       perf.o
>   CC       prog.o
> prog.c: In function ‘do_load’:
> prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ [-Wimplicit-function-declaration]
>     map_replace = reallocarray(map_replace, old_map_fds + 1,
>                   ^~~~~~~~~~~~
> prog.c:785:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
>     map_replace = reallocarray(map_replace, old_map_fds + 1,
>                 ^
>   CC       common.o
>   CC       cgroup.o
>   CC       main.o
>   CC       json_writer.o
>   CC       cfg.o
>   CC       map.o
>   CC       jit_disasm.o
>   CC       disasm.o
> 
> Auto-detecting system features:
> ...                        libelf: [ on  ]
> ...                           bpf: [ on  ]
> 
> Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h'
>   CC       libbpf.o
>   CC       bpf.o
>   CC       nlattr.o
>   CC       btf.o
>   LD       libbpf-in.o
>   LINK     libbpf.a
>   LINK     bpftool
> prog.o: In function `do_load':
> prog.c:(.text+0x23d): undefined reference to `reallocarray'
> collect2: error: ld returned 1 exit status
> Makefile:89: recipe for target 'bpftool' failed
> make[1]: *** [bpftool] Error 1
> Makefile:99: recipe for target 'bpftool' failed
> make: *** [bpftool] Error 2

Oh no..  Sorry & thanks for catching this.  It would be nice to not
depend on Glibc version defines, in case someone is using a different
library.  Jiong suggested we can just use the feature detection, so I
have something like this:

---

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 0911b00b25cc..20a691659381 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -52,8 +52,8 @@ INSTALL ?= install
 RM ?= rm -f
 
 FEATURE_USER = .bpftool
-FEATURE_TESTS = libbfd disassembler-four-args
-FEATURE_DISPLAY = libbfd disassembler-four-args
+FEATURE_TESTS = libbfd disassembler-four-args reallocarray
+FEATURE_DISPLAY = libbfd disassembler-four-args reallocarray
 
 check_feat := 1
 NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install doc-uninstall
diff --git a/tools/bpf/bpftool/compat.h b/tools/bpf/bpftool/compat.h
new file mode 100644
index 000000000000..7885cedc9efe
--- /dev/null
+++ b/tools/bpf/bpftool/compat.h
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (C) 2018 Netronome Systems, Inc. */
+
+#ifndef __BPF_TOOL_COMPAT_H
+#define __BPF_TOOL_COMPAT_H
+
+#define _GNU_SOURCE
+#include <stdlib.h>
+
+static inline void *reallocarray(void *ptr, size_t nmemb, size_t size)
+{
+	return realloc(ptr, nmemb * size);
+}
+#endif
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 1a9a2aefa014..2106adb73631 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -43,6 +43,7 @@
 #include <linux/kernel.h>
 #include <linux/hashtable.h>
 
+#include "compat.h"
 #include "json_writer.h"
 
 #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index dac9563b5470..0516259be70f 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -14,6 +14,7 @@ FILES=                                          \
          test-libaudit.bin                      \
          test-libbfd.bin                        \
          test-disassembler-four-args.bin        \
+         test-reallocarray.bin			\
          test-liberty.bin                       \
          test-liberty-z.bin                     \
          test-cplus-demangle.bin                \
@@ -204,6 +205,9 @@ FLAGS_PERL_EMBED=$(PERL_EMBED_CCOPTS) $(PERL_EMBED_LDOPTS)
 $(OUTPUT)test-disassembler-four-args.bin:
 	$(BUILD) -DPACKAGE='"perf"' -lbfd -lopcodes
 
+$(OUTPUT)test-reallocarray.bin:
+	$(BUILD)
+
 $(OUTPUT)test-liberty.bin:
 	$(CC) $(CFLAGS) -Wall -Werror -o $@ test-libbfd.c -DPACKAGE='"perf"' $(LDFLAGS) -lbfd -ldl -liberty
 
diff --git a/tools/build/feature/test-reallocarray.c b/tools/build/feature/test-reallocarray.c
new file mode 100644
index 000000000000..8170de35150d
--- /dev/null
+++ b/tools/build/feature/test-reallocarray.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <stdlib.h>
+
+int main(void)
+{
+	return !!reallocarray(NULL, 1, 1);
+}
Daniel Borkmann July 6, 2018, 7:16 a.m. UTC | #3
On 07/06/2018 12:57 AM, Jakub Kicinski wrote:
> On Thu, 5 Jul 2018 10:35:24 +0200, Daniel Borkmann wrote:
>> On 07/04/2018 04:54 AM, Jakub Kicinski wrote:
>>> Add map parameter to prog load which will allow reuse of existing
>>> maps instead of creating new ones.
>>>
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>  
>> [...]
>>> +
>>> +			fd = map_parse_fd(&argc, &argv);
>>> +			if (fd < 0)
>>> +				goto err_free_reuse_maps;
>>> +
>>> +			map_replace = reallocarray(map_replace, old_map_fds + 1,
>>> +						   sizeof(*map_replace));
>>> +			if (!map_replace) {
>>> +				p_err("mem alloc failed");
>>> +				goto err_free_reuse_maps;  
>>
>> Series in general looks good to me. However, above reallocarray() doesn't
>> exist and hence build fails, please see below. Is that from newest glibc?
>>
>> You probably need some fallback implementation or in general have something
>> bpftool internal that doesn't make a bet on its availability.
>>
>> # make
>>
>> Auto-detecting system features:
>> ...                        libbfd: [ on  ]
>> ...        disassembler-four-args: [ OFF ]
>>
>>   CC       bpf_jit_disasm.o
>>   LINK     bpf_jit_disasm
>>   CC       bpf_dbg.o
>>   LINK     bpf_dbg
>>   CC       bpf_asm.o
>>   BISON    bpf_exp.yacc.c
>>   CC       bpf_exp.yacc.o
>>   FLEX     bpf_exp.lex.c
>>   CC       bpf_exp.lex.o
>>   LINK     bpf_asm
>>   DESCEND  bpftool
>>
>> Auto-detecting system features:
y>> ...                        libbfd: [ on  ]
>> ...        disassembler-four-args: [ OFF ]
>>
>>   CC       map_perf_ring.o
>>   CC       xlated_dumper.o
>>   CC       perf.o
>>   CC       prog.o
>> prog.c: In function ‘do_load’:
>> prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ [-Wimplicit-function-declaration]
>>     map_replace = reallocarray(map_replace, old_map_fds + 1,
>>                   ^~~~~~~~~~~~
>> prog.c:785:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
>>     map_replace = reallocarray(map_replace, old_map_fds + 1,
>>                 ^
>>   CC       common.o
>>   CC       cgroup.o
>>   CC       main.o
>>   CC       json_writer.o
>>   CC       cfg.o
>>   CC       map.o
>>   CC       jit_disasm.o
>>   CC       disasm.o
>>
>> Auto-detecting system features:
>> ...                        libelf: [ on  ]
>> ...                           bpf: [ on  ]
>>
>> Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h'
>>   CC       libbpf.o
>>   CC       bpf.o
>>   CC       nlattr.o
>>   CC       btf.o
>>   LD       libbpf-in.o
>>   LINK     libbpf.a
>>   LINK     bpftool
>> prog.o: In function `do_load':
>> prog.c:(.text+0x23d): undefined reference to `reallocarray'
>> collect2: error: ld returned 1 exit status
>> Makefile:89: recipe for target 'bpftool' failed
>> make[1]: *** [bpftool] Error 1
>> Makefile:99: recipe for target 'bpftool' failed
>> make: *** [bpftool] Error 2
> 
> Oh no..  Sorry & thanks for catching this.  It would be nice to not
> depend on Glibc version defines, in case someone is using a different
> library.  Jiong suggested we can just use the feature detection, so I
> have something like this:

Yeah, that would be okay to do if you want to go that route, sure. Other option
I had in mind would have been to import include/linux/overflow.h into the
tools/include/linux/ and have a minor wrapper similar to kmalloc_array() in a
utils.h in bpftool to get to the same for all users. But I think feature test
is totally fine too, and in general some form of reallocarray() would be good
to have rather than plain realloc().

So, below complies for me. Although don't we need to define a CFLAG based on
the outcome of the test similar as in feature-disassembler-four-args? Otherwise
with the below diff the test doesn't really do much, no? Meaning, adding a ...

  ifeq ($(feature-reallocarray), 1)
  CFLAGS += -DHAVE_REALLOCARRAY
  endif

... to the Makefile and in compat.h having it enabled through:

  #ifndef HAVE_REALLOCARRAY
  static inline void *reallocarray(void *ptr, size_t nmemb, size_t size)
  {
          return realloc(ptr, nmemb * size);
  }
  #endif

In any case, we could use reallocarray() also in couple of other places in
bpftool and libbpf.

> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 0911b00b25cc..20a691659381 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -52,8 +52,8 @@ INSTALL ?= install
>  RM ?= rm -f
>  
>  FEATURE_USER = .bpftool
> -FEATURE_TESTS = libbfd disassembler-four-args
> -FEATURE_DISPLAY = libbfd disassembler-four-args
> +FEATURE_TESTS = libbfd disassembler-four-args reallocarray
> +FEATURE_DISPLAY = libbfd disassembler-four-args reallocarray
>  
>  check_feat := 1
>  NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install doc-uninstall
> diff --git a/tools/bpf/bpftool/compat.h b/tools/bpf/bpftool/compat.h
> new file mode 100644
> index 000000000000..7885cedc9efe
> --- /dev/null
> +++ b/tools/bpf/bpftool/compat.h
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2018 Netronome Systems, Inc. */
> +
> +#ifndef __BPF_TOOL_COMPAT_H
> +#define __BPF_TOOL_COMPAT_H
> +
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +
> +static inline void *reallocarray(void *ptr, size_t nmemb, size_t size)
> +{
> +	return realloc(ptr, nmemb * size);
> +}
> +#endif
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 1a9a2aefa014..2106adb73631 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -43,6 +43,7 @@
>  #include <linux/kernel.h>
>  #include <linux/hashtable.h>
>  
> +#include "compat.h"
>  #include "json_writer.h"
>  
>  #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
> diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
> index dac9563b5470..0516259be70f 100644
> --- a/tools/build/feature/Makefile
> +++ b/tools/build/feature/Makefile
> @@ -14,6 +14,7 @@ FILES=                                          \
>           test-libaudit.bin                      \
>           test-libbfd.bin                        \
>           test-disassembler-four-args.bin        \
> +         test-reallocarray.bin			\
>           test-liberty.bin                       \
>           test-liberty-z.bin                     \
>           test-cplus-demangle.bin                \
> @@ -204,6 +205,9 @@ FLAGS_PERL_EMBED=$(PERL_EMBED_CCOPTS) $(PERL_EMBED_LDOPTS)
>  $(OUTPUT)test-disassembler-four-args.bin:
>  	$(BUILD) -DPACKAGE='"perf"' -lbfd -lopcodes
>  
> +$(OUTPUT)test-reallocarray.bin:
> +	$(BUILD)
> +
>  $(OUTPUT)test-liberty.bin:
>  	$(CC) $(CFLAGS) -Wall -Werror -o $@ test-libbfd.c -DPACKAGE='"perf"' $(LDFLAGS) -lbfd -ldl -liberty
>  
> diff --git a/tools/build/feature/test-reallocarray.c b/tools/build/feature/test-reallocarray.c
> new file mode 100644
> index 000000000000..8170de35150d
> --- /dev/null
> +++ b/tools/build/feature/test-reallocarray.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +
> +int main(void)
> +{
> +	return !!reallocarray(NULL, 1, 1);
> +}
>
Jakub Kicinski July 6, 2018, 3:30 p.m. UTC | #4
On Fri, 6 Jul 2018 09:16:24 +0200, Daniel Borkmann wrote:
> On 07/06/2018 12:57 AM, Jakub Kicinski wrote:
> > On Thu, 5 Jul 2018 10:35:24 +0200, Daniel Borkmann wrote:  
> >> On 07/04/2018 04:54 AM, Jakub Kicinski wrote:  
> >>> Add map parameter to prog load which will allow reuse of existing
> >>> maps instead of creating new ones.
> >>>
> >>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >>> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>    
> >> [...]  
> >>> +
> >>> +			fd = map_parse_fd(&argc, &argv);
> >>> +			if (fd < 0)
> >>> +				goto err_free_reuse_maps;
> >>> +
> >>> +			map_replace = reallocarray(map_replace, old_map_fds + 1,
> >>> +						   sizeof(*map_replace));
> >>> +			if (!map_replace) {
> >>> +				p_err("mem alloc failed");
> >>> +				goto err_free_reuse_maps;    
> >>
> >> Series in general looks good to me. However, above reallocarray() doesn't
> >> exist and hence build fails, please see below. Is that from newest glibc?
> >>
> >> You probably need some fallback implementation or in general have something
> >> bpftool internal that doesn't make a bet on its availability.
> >>
> >> # make
> >>
> >> Auto-detecting system features:
> >> ...                        libbfd: [ on  ]
> >> ...        disassembler-four-args: [ OFF ]
> >>
> >>   CC       bpf_jit_disasm.o
> >>   LINK     bpf_jit_disasm
> >>   CC       bpf_dbg.o
> >>   LINK     bpf_dbg
> >>   CC       bpf_asm.o
> >>   BISON    bpf_exp.yacc.c
> >>   CC       bpf_exp.yacc.o
> >>   FLEX     bpf_exp.lex.c
> >>   CC       bpf_exp.lex.o
> >>   LINK     bpf_asm
> >>   DESCEND  bpftool
> >>
> >> Auto-detecting system features:
> y>> ...                        libbfd: [ on  ]
> >> ...        disassembler-four-args: [ OFF ]
> >>
> >>   CC       map_perf_ring.o
> >>   CC       xlated_dumper.o
> >>   CC       perf.o
> >>   CC       prog.o
> >> prog.c: In function ‘do_load’:
> >> prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ [-Wimplicit-function-declaration]
> >>     map_replace = reallocarray(map_replace, old_map_fds + 1,
> >>                   ^~~~~~~~~~~~
> >> prog.c:785:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> >>     map_replace = reallocarray(map_replace, old_map_fds + 1,
> >>                 ^
> >>   CC       common.o
> >>   CC       cgroup.o
> >>   CC       main.o
> >>   CC       json_writer.o
> >>   CC       cfg.o
> >>   CC       map.o
> >>   CC       jit_disasm.o
> >>   CC       disasm.o
> >>
> >> Auto-detecting system features:
> >> ...                        libelf: [ on  ]
> >> ...                           bpf: [ on  ]
> >>
> >> Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h'
> >>   CC       libbpf.o
> >>   CC       bpf.o
> >>   CC       nlattr.o
> >>   CC       btf.o
> >>   LD       libbpf-in.o
> >>   LINK     libbpf.a
> >>   LINK     bpftool
> >> prog.o: In function `do_load':
> >> prog.c:(.text+0x23d): undefined reference to `reallocarray'
> >> collect2: error: ld returned 1 exit status
> >> Makefile:89: recipe for target 'bpftool' failed
> >> make[1]: *** [bpftool] Error 1
> >> Makefile:99: recipe for target 'bpftool' failed
> >> make: *** [bpftool] Error 2  
> > 
> > Oh no..  Sorry & thanks for catching this.  It would be nice to not
> > depend on Glibc version defines, in case someone is using a different
> > library.  Jiong suggested we can just use the feature detection, so I
> > have something like this:  
> 
> Yeah, that would be okay to do if you want to go that route, sure. Other option
> I had in mind would have been to import include/linux/overflow.h into the
> tools/include/linux/ and have a minor wrapper similar to kmalloc_array() in a
> utils.h in bpftool to get to the same for all users. But I think feature test
> is totally fine too, and in general some form of reallocarray() would be good
> to have rather than plain realloc().
> 
> So, below complies for me. Although don't we need to define a CFLAG based on
> the outcome of the test similar as in feature-disassembler-four-args? Otherwise
> with the below diff the test doesn't really do much, no? Meaning, adding a ...
> 
>   ifeq ($(feature-reallocarray), 1)
>   CFLAGS += -DHAVE_REALLOCARRAY
>   endif
> 
> ... to the Makefile and in compat.h having it enabled through:
> 
>   #ifndef HAVE_REALLOCARRAY
>   static inline void *reallocarray(void *ptr, size_t nmemb, size_t size)
>   {
>           return realloc(ptr, nmemb * size);
>   }
>   #endif

Ugh, you're very right, reallocarray is a weak alias in glibc, which is
probably why I didn't see any errors.

> In any case, we could use reallocarray() also in couple of other places in
> bpftool and libbpf.

I'll look into it, too.
diff mbox series

Patch

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index e53e1ad2caf0..64156a16d530 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -24,9 +24,10 @@  MAP COMMANDS
 |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}]
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
-|	**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**dev** *NAME*]
+|	**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
 |	**bpftool** **prog help**
 |
+|	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
 |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
 |	*TYPE* := {
 |		**socket** | **kprobe** | **kretprobe** | **classifier** | **action** |
@@ -73,10 +74,17 @@  DESCRIPTION
 
 		  Note: *FILE* must be located in *bpffs* mount.
 
-	**bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**dev** *NAME*]
+	**bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
 		  Load bpf program from binary *OBJ* and pin as *FILE*.
 		  **type** is optional, if not specified program type will be
 		  inferred from section names.
+		  By default bpftool will create new maps as declared in the ELF
+		  object being loaded.  **map** parameter allows for the reuse
+		  of existing maps.  It can be specified multiple times, each
+		  time for a different map.  *IDX* refers to index of the map
+		  to be replaced in the ELF file counting from 0, while *NAME*
+		  allows to replace a map by name.  *MAP* specifies the map to
+		  use, referring to it by **id** or through a **pinned** file.
 		  If **dev** *NAME* is specified program will be loaded onto
 		  given networking device (offload).
 
@@ -172,6 +180,14 @@  EXAMPLES
     mov    %rbx,0x0(%rbp)
     48 89 5d 00
 
+|
+| **# bpftool prog load xdp1_kern.o /sys/fs/bpf/xdp1 type xdp map name rxcnt id 7**
+| **# bpftool prog show pinned /sys/fs/bpf/xdp1**
+|   9: xdp  name xdp_prog1  tag 539ec6ce11b52f98  gpl
+|	loaded_at 2018-06-25T16:17:31-0700  uid 0
+|	xlated 488B  jited 336B  memlock 4096B  map_ids 7
+| **# rm /sys/fs/bpf/xdp1**
+|
 
 SEE ALSO
 ========
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 9ae33a73d732..626598964cee 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -99,6 +99,29 @@  _bpftool_get_prog_tags()
         command sed -n 's/.*"tag": "\(.*\)",$/\1/p' )" -- "$cur" ) )
 }
 
+_bpftool_get_obj_map_names()
+{
+    local obj
+
+    obj=$1
+
+    maps=$(objdump -j maps -t $obj 2>/dev/null | \
+        command awk '/g     . maps/ {print $NF}')
+
+    COMPREPLY+=( $( compgen -W "$maps" -- "$cur" ) )
+}
+
+_bpftool_get_obj_map_idxs()
+{
+    local obj
+
+    obj=$1
+
+    nmaps=$(objdump -j maps -t $obj 2>/dev/null | grep -c 'g     . maps')
+
+    COMPREPLY+=( $( compgen -W "$(seq 0 $((nmaps - 1)))" -- "$cur" ) )
+}
+
 _sysfs_get_netdevs()
 {
     COMPREPLY+=( $( compgen -W "$( ls /sys/class/net 2>/dev/null )" -- \
@@ -220,12 +243,14 @@  _bpftool()
     # Completion depends on object and command in use
     case $object in
         prog)
-            case $prev in
-                id)
-                    _bpftool_get_prog_ids
-                    return 0
-                    ;;
-            esac
+            if [[ $command != "load" ]]; then
+                case $prev in
+                    id)
+                        _bpftool_get_prog_ids
+                        return 0
+                        ;;
+                esac
+            fi
 
             local PROG_TYPE='id pinned tag'
             case $command in
@@ -268,22 +293,52 @@  _bpftool()
                     return 0
                     ;;
                 load)
+                    local obj
+
                     if [[ ${#words[@]} -lt 6 ]]; then
                         _filedir
                         return 0
                     fi
 
+                    obj=${words[3]}
+
+                    if [[ ${words[-4]} == "map" ]]; then
+                        COMPREPLY=( $( compgen -W "id pinned" -- "$cur" ) )
+                        return 0
+                    fi
+                    if [[ ${words[-3]} == "map" ]]; then
+                        if [[ ${words[-2]} == "idx" ]]; then
+                            _bpftool_get_obj_map_idxs $obj
+                        elif [[ ${words[-2]} == "name" ]]; then
+                            _bpftool_get_obj_map_names $obj
+                        fi
+                        return 0
+                    fi
+                    if [[ ${words[-2]} == "map" ]]; then
+                        COMPREPLY=( $( compgen -W "idx name" -- "$cur" ) )
+                        return 0
+                    fi
+
                     case $prev in
                         type)
                             COMPREPLY=( $( compgen -W "socket kprobe kretprobe classifier action tracepoint raw_tracepoint xdp perf_event cgroup/skb cgroup/sock cgroup/dev lwt_in lwt_out lwt_xmit lwt_seg6local sockops sk_skb sk_msg lirc_mode2 cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6 cgroup/post_bind4 cgroup/post_bind6" -- \
                                                    "$cur" ) )
                             return 0
                             ;;
+                        id)
+                            _bpftool_get_map_ids
+                            return 0
+                            ;;
+                        pinned)
+                            _filedir
+                            return 0
+                            ;;
                         dev)
                             _sysfs_get_netdevs
                             return 0
                             ;;
                         *)
+                            COMPREPLY=( $( compgen -W "map" -- "$cur" ) )
                             _bpftool_once_attr 'type'
                             _bpftool_once_attr 'dev'
                             return 0
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 15b6c49ae533..1a9a2aefa014 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -74,6 +74,8 @@ 
 	"PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }"
 #define HELP_SPEC_OPTIONS						\
 	"OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} }"
+#define HELP_SPEC_MAP							\
+	"MAP := { id MAP_ID | pinned FILE }"
 
 enum bpf_obj_type {
 	BPF_OBJ_UNKNOWN,
@@ -135,6 +137,7 @@  int do_cgroup(int argc, char **arg);
 int do_perf(int argc, char **arg);
 
 int prog_parse_fd(int *argc, char ***argv);
+int map_parse_fd(int *argc, char ***argv);
 int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
 
 void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 5989e1575ae4..e2baec1122fb 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -93,7 +93,7 @@  static void *alloc_value(struct bpf_map_info *info)
 		return malloc(info->value_size);
 }
 
-static int map_parse_fd(int *argc, char ***argv)
+int map_parse_fd(int *argc, char ***argv)
 {
 	int fd;
 
@@ -824,7 +824,7 @@  static int do_help(int argc, char **argv)
 		"       %s %s event_pipe MAP [cpu N index M]\n"
 		"       %s %s help\n"
 		"\n"
-		"       MAP := { id MAP_ID | pinned FILE }\n"
+		"       " HELP_SPEC_MAP "\n"
 		"       DATA := { [hex] BYTES }\n"
 		"       " HELP_SPEC_PROGRAM "\n"
 		"       VALUE := { DATA | MAP | PROG }\n"
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 267d653c93f5..0f8bdab62864 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -31,6 +31,7 @@ 
  * SOFTWARE.
  */
 
+#define _GNU_SOURCE
 #include <errno.h>
 #include <fcntl.h>
 #include <stdarg.h>
@@ -682,18 +683,34 @@  static int do_pin(int argc, char **argv)
 	return err;
 }
 
+struct map_replace {
+	int idx;
+	int fd;
+	char *name;
+};
+
+int map_replace_compar(const void *p1, const void *p2)
+{
+	const struct map_replace *a = p1, *b = p2;
+
+	return a->idx - b->idx;
+}
+
 static int do_load(int argc, char **argv)
 {
 	enum bpf_attach_type expected_attach_type;
 	struct bpf_object_open_attr attr = {
 		.prog_type	= BPF_PROG_TYPE_UNSPEC,
 	};
+	struct map_replace *map_replace = NULL;
 	const char *objfile, *pinfile;
+	unsigned int old_map_fds = 0;
 	struct bpf_program *prog;
 	struct bpf_object *obj;
 	struct bpf_map *map;
+	unsigned int i, j;
 	__u32 ifindex = 0;
-	int err;
+	int idx, err;
 
 	if (!REQ_ARGS(2))
 		return -1;
@@ -708,16 +725,16 @@  static int do_load(int argc, char **argv)
 
 			if (attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
 				p_err("program type already specified");
-				return -1;
+				goto err_free_reuse_maps;
 			}
 			if (!REQ_ARGS(1))
-				return -1;
+				goto err_free_reuse_maps;
 
 			/* Put a '/' at the end of type to appease libbpf */
 			type = malloc(strlen(*argv) + 2);
 			if (!type) {
 				p_err("mem alloc failed");
-				return -1;
+				goto err_free_reuse_maps;
 			}
 			*type = 0;
 			strcat(type, *argv);
@@ -728,37 +745,81 @@  static int do_load(int argc, char **argv)
 			free(type);
 			if (err < 0) {
 				p_err("unknown program type '%s'", *argv);
-				return err;
+				goto err_free_reuse_maps;
 			}
 			NEXT_ARG();
+		} else if (is_prefix(*argv, "map")) {
+			char *endptr, *name;
+			int fd;
+
+			NEXT_ARG();
+
+			if (!REQ_ARGS(4))
+				goto err_free_reuse_maps;
+
+			if (is_prefix(*argv, "idx")) {
+				NEXT_ARG();
+
+				idx = strtoul(*argv, &endptr, 0);
+				if (*endptr) {
+					p_err("can't parse %s as IDX", *argv);
+					goto err_free_reuse_maps;
+				}
+				name = NULL;
+			} else if (is_prefix(*argv, "name")) {
+				NEXT_ARG();
+
+				name = *argv;
+				idx = -1;
+			} else {
+				p_err("expected 'idx' or 'name', got: '%s'?",
+				      *argv);
+				goto err_free_reuse_maps;
+			}
+			NEXT_ARG();
+
+			fd = map_parse_fd(&argc, &argv);
+			if (fd < 0)
+				goto err_free_reuse_maps;
+
+			map_replace = reallocarray(map_replace, old_map_fds + 1,
+						   sizeof(*map_replace));
+			if (!map_replace) {
+				p_err("mem alloc failed");
+				goto err_free_reuse_maps;
+			}
+			map_replace[old_map_fds].idx = idx;
+			map_replace[old_map_fds].name = name;
+			map_replace[old_map_fds].fd = fd;
+			old_map_fds++;
 		} else if (is_prefix(*argv, "dev")) {
 			NEXT_ARG();
 
 			if (ifindex) {
 				p_err("offload device already specified");
-				return -1;
+				goto err_free_reuse_maps;
 			}
 			if (!REQ_ARGS(1))
-				return -1;
+				goto err_free_reuse_maps;
 
 			ifindex = if_nametoindex(*argv);
 			if (!ifindex) {
 				p_err("unrecognized netdevice '%s': %s",
 				      *argv, strerror(errno));
-				return -1;
+				goto err_free_reuse_maps;
 			}
 			NEXT_ARG();
 		} else {
-			p_err("expected no more arguments, 'type' or 'dev', got: '%s'?",
+			p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?",
 			      *argv);
-			return -1;
+			goto err_free_reuse_maps;
 		}
 	}
 
 	obj = bpf_object__open_xattr(objfile, &attr);
 	if (IS_ERR_OR_NULL(obj)) {
 		p_err("failed to open object file");
-		return -1;
+		goto err_free_reuse_maps;
 	}
 
 	prog = bpf_program__next(NULL, obj);
@@ -782,10 +843,62 @@  static int do_load(int argc, char **argv)
 	bpf_program__set_type(prog, attr.prog_type);
 	bpf_program__set_expected_attach_type(prog, expected_attach_type);
 
-	bpf_map__for_each(map, obj)
+	qsort(map_replace, old_map_fds, sizeof(*map_replace),
+	      map_replace_compar);
+
+	/* After the sort maps by name will be first on the list, because they
+	 * have idx == -1.  Resolve them.
+	 */
+	j = 0;
+	while (j < old_map_fds && map_replace[j].name) {
+		i = 0;
+		bpf_map__for_each(map, obj) {
+			if (!strcmp(bpf_map__name(map), map_replace[j].name)) {
+				map_replace[j].idx = i;
+				break;
+			}
+			i++;
+		}
+		if (map_replace[j].idx == -1) {
+			p_err("unable to find map '%s'", map_replace[j].name);
+			goto err_close_obj;
+		}
+		j++;
+	}
+	/* Resort if any names were resolved */
+	if (j)
+		qsort(map_replace, old_map_fds, sizeof(*map_replace),
+		      map_replace_compar);
+
+	/* Set ifindex and name reuse */
+	j = 0;
+	idx = 0;
+	bpf_map__for_each(map, obj) {
 		if (!bpf_map__is_offload_neutral(map))
 			bpf_map__set_ifindex(map, ifindex);
 
+		if (j < old_map_fds && idx == map_replace[j].idx) {
+			err = bpf_map__reuse_fd(map, map_replace[j++].fd);
+			if (err) {
+				p_err("unable to set up map reuse: %d", err);
+				goto err_close_obj;
+			}
+
+			/* Next reuse wants to apply to the same map */
+			if (j < old_map_fds && map_replace[j].idx == idx) {
+				p_err("replacement for map idx %d specified more than once",
+				      idx);
+				goto err_close_obj;
+			}
+		}
+
+		idx++;
+	}
+	if (j < old_map_fds) {
+		p_err("map idx '%d' not used", map_replace[j].idx);
+		goto err_close_obj;
+	}
+
 	err = bpf_object__load(obj);
 	if (err) {
 		p_err("failed to load object file");
@@ -799,11 +912,18 @@  static int do_load(int argc, char **argv)
 		jsonw_null(json_wtr);
 
 	bpf_object__close(obj);
+	for (i = 0; i < old_map_fds; i++)
+		close(map_replace[i].fd);
+	free(map_replace);
 
 	return 0;
 
 err_close_obj:
 	bpf_object__close(obj);
+err_free_reuse_maps:
+	for (i = 0; i < old_map_fds; i++)
+		close(map_replace[i].fd);
+	free(map_replace);
 	return -1;
 }
 
@@ -819,9 +939,11 @@  static int do_help(int argc, char **argv)
 		"       %s %s dump xlated PROG [{ file FILE | opcodes | visual }]\n"
 		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
 		"       %s %s pin   PROG FILE\n"
-		"       %s %s load  OBJ  FILE [type TYPE] [dev NAME]\n"
+		"       %s %s load  OBJ  FILE [type TYPE] [dev NAME] \\\n"
+		"                         [map { idx IDX | name NAME } MAP]\n"
 		"       %s %s help\n"
 		"\n"
+		"       " HELP_SPEC_MAP "\n"
 		"       " HELP_SPEC_PROGRAM "\n"
 		"       TYPE := { socket | kprobe | kretprobe | classifier | action |\n"
 		"                 tracepoint | raw_tracepoint | xdp | perf_event | cgroup/skb |\n"