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

[Daniel Borkmann <daniel@iogearbox.net>]

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);
> +}
>