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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

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.