netdev - Re: [PATCH 3/3] samples: bpf: refactor BPF map in map test with libbpf

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] [day] [month] [year] [list]

Date:   Sat, 27 Jun 2020 07:25:30 +0900
From:   "Daniel T. Lee" <danieltimlee@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Yonghong Song <yhs@...com>, Martin KaFai Lau <kafai@...com>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 3/3] samples: bpf: refactor BPF map in map test with libbpf

On Sat, Jun 27, 2020 at 7:19 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Fri, Jun 26, 2020 at 3:14 PM Daniel T. Lee <danieltimlee@...il.com> wrote:
> >
> > On Sat, Jun 27, 2020 at 5:30 AM Andrii Nakryiko
> > <andrii.nakryiko@...il.com> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@...il.com> wrote:
> > > >
> > > > From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
> > > > support"), a way to define internal map in BTF-defined map has been
> > > > added.
> > > >
> > > > Instead of using previous 'inner_map_idx' definition, the structure to
> > > > be used for the inner map can be directly defined using array directive.
> > > >
> > > >     __array(values, struct inner_map)
> > > >
> > > > This commit refactors map in map test program with libbpf by explicitly
> > > > defining inner map with BTF-defined format.
> > > >
> > > > Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> > > > ---
> > >
> > > Thanks for the clean up, looks good except that prog NULL check.
> > >
> >
> > I'll fix this NULL check as well too.
> >
> > > It also seems like this is the last use of bpf_map_def_legacy, do you
> > > mind removing it as well?
> > >
> >
> > Actually, there is one more place that uses bpf_map_def_legacy.
> > map_perf_test_kern.c is the one, and I'm currently working on it, but
> > I'm having difficulty with refactoring this file at the moment.
> >
> > It has a hash_map map definition named inner_lru_hash_map with
> > BPF_F_NUMA_NODE flag and '.numa_node = 0'.
> >
> > The bpf_map_def in libbpf has the attribute name map_flags but
> > it does not have the numa_node attribute. Because the numa node
>
> It does since 1 or 2 days ago ([0])
>
>   [0] https://patchwork.ozlabs.org/project/netdev/patch/20200621062112.3006313-1-andriin@fb.com/
>
>
> > for bpf_map_def cannot be explicitly specified, this means that there
> > is no way to set the numa node where the map will be placed at the
> > time of bpf_object__load.
> >
> > The only approach currently available is not to use libbbpf to handle
> > everything (bpf_object_load), but instead to create a map directly with
> > specifying numa node (bpf_load approach).
> >
> >     bpf_create_map_in_map_node
> >     bpf_create_map_node
> >
> > I'm trying to stick with the libbpf implementation only, and I'm wondering
> > If I have to create bpf maps manually at _user.c program.
> >
> > Any advice and suggestions will be greatly appreciated.
> >
>
> It should be super straightforward now with a BTF-defined map
> supporting numa_node attribute.
>

Awesome, thanks for letting me know!

I will use this new attribute for the map_perf_test refactoring.
Problem Solved!

Thanks.

> > Thanks for your time and effort for the review.
> > Daniel.
> >
> > >
> > > >  samples/bpf/Makefile               |  2 +-
> > > >  samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
> > > >  samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
> > > >  3 files changed, 91 insertions(+), 49 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > >
> > > >         snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > > > +       obj = bpf_object__open_file(filename, NULL);
> > > > +       if (libbpf_get_error(obj)) {
> > >
> > > this is right, but...
> > >
> > > > +               fprintf(stderr, "ERROR: opening BPF object file failed\n");
> > > > +               return 0;
> > > > +       }
> > > >
> > > > -       if (load_bpf_file(filename)) {
> > > > -               printf("%s", bpf_log_buf);
> > > > -               return 1;
> > > > +       prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
> > > > +       if (libbpf_get_error(prog)) {
> > >
> > > this is wrong. Just NULL check. libbpf APIs are not very consistent
> > > with what they return, unfortunately.
> > >
> > > > +               printf("finding a prog in obj file failed\n");
> > > > +               goto cleanup;
> > > > +       }
> > > > +
> > >
> > > [...]