[bpf-next,v4,0/5] Allow storage of flexible metadata information for eBPF programs

Currently, if a user wants to store arbitrary metadata for an eBPF
program, for example, the program build commit hash or version, they
could store it in a map, and conveniently libbpf uses .data section to
populate an internal map. However, if the program does not actually
reference the map, then the map would be de-refcounted and freed.

This patch set introduces a new syscall BPF_PROG_BIND_MAP to add a map
to a program's used_maps, even if the program instructions does not
reference the map.

libbpf is extended to always BPF_PROG_BIND_MAP .rodata section so the
metadata is kept in place.
bpftool is also extended to print metadata in the 'bpftool prog' list.

The variable is considered metadata if it starts with the
magic 'bpf_metadata_' prefix; everything after the prefix is the
metadata name.

An example use of this would be BPF C file declaring:

  const char bpf_metadata_commit_hash[] SEC(".rodata") = "abcdef123456";

and bpftool would emit:

  $ bpftool prog
  [...]
        metadata:
                commit_hash = "abcdef123456"

v4 changes:
* Don't return EEXIST from syscall if already bound (Andrii Nakryiko)
* Removed --metadata argument (Andrii Nakryiko)
* Removed custom .metadata section (Alexei Starovoitov)
* Addressed Andrii's suggestions about btf helpers and vsi (Andrii Nakryiko)
* Moved bpf_prog_find_metadata into bpftool (Alexei Starovoitov)

v3 changes:
* API changes for bpf_prog_find_metadata (Toke Høiland-Jørgensen)

v2 changes:
* Made struct bpf_prog_bind_opts in libbpf so flags is optional.
* Deduped probe_kern_global_data and probe_prog_bind_map into a common
  helper.
* Added comment regarding why EEXIST is ignored in libbpf bind map.
* Froze all LIBBPF_MAP_METADATA internal maps.
* Moved bpf_prog_bind_map into new LIBBPF_0.1.1 in libbpf.map.
* Added p_err() calls on error cases in bpftool show_prog_metadata.
* Reverse christmas tree coding style in bpftool show_prog_metadata.
* Made bpftool gen skeleton recognize .metadata as an internal map and
  generate datasec definition in skeleton.
* Added C test using skeleton to see asset that the metadata is what we
  expect and rebinding causes EEXIST.

v1 changes:
* Fixed a few missing unlocks, and missing close while iterating map fds.
* Move mutex initialization to right after prog aux allocation, and mutex
  destroy to right after prog aux free.
* s/ADD_MAP/BIND_MAP/
* Use mutex only instead of RCU to protect the used_map array & count.

Cc: YiFei Zhu <zhuyifei1999@gmail.com>

YiFei Zhu (5):
  bpf: Mutex protect used_maps array and count
  bpf: Add BPF_PROG_BIND_MAP syscall
  libbpf: Add BPF_PROG_BIND_MAP syscall and use it on .metadata section
  bpftool: support dumping metadata
  selftests/bpf: Test load and dump metadata with btftool and skel

 .../net/ethernet/netronome/nfp/bpf/offload.c  |  18 +-
 include/linux/bpf.h                           |   1 +
 include/uapi/linux/bpf.h                      |   7 +
 kernel/bpf/core.c                             |  15 +-
 kernel/bpf/syscall.c                          |  79 ++++++-
 net/core/dev.c                                |  11 +-
 tools/bpf/bpftool/json_writer.c               |   6 +
 tools/bpf/bpftool/json_writer.h               |   3 +
 tools/bpf/bpftool/prog.c                      | 222 ++++++++++++++++++
 tools/include/uapi/linux/bpf.h                |   7 +
 tools/lib/bpf/bpf.c                           |  13 +
 tools/lib/bpf/bpf.h                           |   8 +
 tools/lib/bpf/libbpf.c                        |  94 ++++++--
 tools/lib/bpf/libbpf.map                      |   1 +
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../selftests/bpf/prog_tests/metadata.c       |  81 +++++++
 .../selftests/bpf/progs/metadata_unused.c     |  15 ++
 .../selftests/bpf/progs/metadata_used.c       |  15 ++
 .../selftests/bpf/test_bpftool_metadata.sh    |  82 +++++++
 19 files changed, 645 insertions(+), 36 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/metadata.c
 create mode 100644 tools/testing/selftests/bpf/progs/metadata_unused.c
 create mode 100644 tools/testing/selftests/bpf/progs/metadata_used.c
 create mode 100755 tools/testing/selftests/bpf/test_bpftool_metadata.sh

On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> From: YiFei Zhu <zhuyifei@google.com>
>
> This syscall binds a map to a program. Returns success if the map is
> already bound to the program.
>
> Cc: YiFei Zhu <zhuyifei1999@gmail.com>
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  include/uapi/linux/bpf.h       |  7 ++++
>  kernel/bpf/syscall.c           | 63 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  7 ++++
>  3 files changed, 77 insertions(+)
>

[...]

On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> From: YiFei Zhu <zhuyifei@google.com>
>
> This is a simple test to check that loading and dumping metadata
> in btftool works, whether or not metadata contents are used by the
> program.
>
> A C test is also added to make sure the skeleton code can read the
> metadata values.
>
> Cc: YiFei Zhu <zhuyifei1999@gmail.com>
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---

It would be good to test that libbpf does bind .rodata even if BPF
program doesn't use it. So for metadata_unused you can get
bpf_prog_info and check that it does contain the id of .rodata?

>  tools/testing/selftests/bpf/Makefile          |  3 +-
>  .../selftests/bpf/prog_tests/metadata.c       | 81 ++++++++++++++++++
>  .../selftests/bpf/progs/metadata_unused.c     | 15 ++++
>  .../selftests/bpf/progs/metadata_used.c       | 15 ++++
>  .../selftests/bpf/test_bpftool_metadata.sh    | 82 +++++++++++++++++++
>  5 files changed, 195 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/metadata.c
>  create mode 100644 tools/testing/selftests/bpf/progs/metadata_unused.c
>  create mode 100644 tools/testing/selftests/bpf/progs/metadata_used.c
>  create mode 100755 tools/testing/selftests/bpf/test_bpftool_metadata.sh
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 65d3d9aaeb31..3c92db8a189a 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -68,7 +68,8 @@ TEST_PROGS := test_kmod.sh \
>         test_tc_edt.sh \
>         test_xdping.sh \
>         test_bpftool_build.sh \
> -       test_bpftool.sh
> +       test_bpftool.sh \
> +       test_bpftool_metadata.sh \
>
>  TEST_PROGS_EXTENDED := with_addr.sh \
>         with_tunnels.sh \
> diff --git a/tools/testing/selftests/bpf/prog_tests/metadata.c b/tools/testing/selftests/bpf/prog_tests/metadata.c
> new file mode 100644
> index 000000000000..dea8fa86b5fb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/metadata.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright 2020 Google LLC.
> + */
> +
> +#include <test_progs.h>
> +#include <cgroup_helpers.h>
> +#include <network_helpers.h>
> +
> +#include "metadata_unused.skel.h"
> +#include "metadata_used.skel.h"
> +
> +static int duration;
> +
> +static void test_metadata_unused(void)
> +{
> +       struct metadata_unused *obj;
> +       int err;
> +
> +       obj = metadata_unused__open_and_load();
> +       if (CHECK(!obj, "skel-load", "errno %d", errno))
> +               return;
> +
> +       /* Assert that we can access the metadata in skel and the values are
> +        * what we expect.
> +        */
> +       if (CHECK(strncmp(obj->rodata->bpf_metadata_a, "foo",
> +                         sizeof(obj->rodata->bpf_metadata_a)),
> +                 "bpf_metadata_a", "expected \"foo\", value differ"))
> +               goto close_bpf_object;
> +       if (CHECK(obj->rodata->bpf_metadata_b != 1, "bpf_metadata_b",
> +                 "expected 1, got %d", obj->rodata->bpf_metadata_b))
> +               goto close_bpf_object;
> +
> +       /* Assert that binding metadata map to prog again succeeds. */
> +       err = bpf_prog_bind_map(bpf_program__fd(obj->progs.prog),
> +                               bpf_map__fd(obj->maps.rodata), NULL);
> +       CHECK(err, "rebind_map", "errno %d, expected 0", errno);
> +
> +close_bpf_object:
> +       metadata_unused__destroy(obj);
> +}
> +
> +static void test_metadata_used(void)
> +{
> +       struct metadata_used *obj;
> +       int err;
> +
> +       obj = metadata_used__open_and_load();
> +       if (CHECK(!obj, "skel-load", "errno %d", errno))
> +               return;
> +
> +       /* Assert that we can access the metadata in skel and the values are
> +        * what we expect.
> +        */
> +       if (CHECK(strncmp(obj->rodata->bpf_metadata_a, "bar",
> +                         sizeof(obj->rodata->bpf_metadata_a)),
> +                 "metadata_a", "expected \"bar\", value differ"))
> +               goto close_bpf_object;
> +       if (CHECK(obj->rodata->bpf_metadata_b != 2, "metadata_b",
> +                 "expected 2, got %d", obj->rodata->bpf_metadata_b))
> +               goto close_bpf_object;
> +
> +       /* Assert that binding metadata map to prog again succeeds. */
> +       err = bpf_prog_bind_map(bpf_program__fd(obj->progs.prog),
> +                               bpf_map__fd(obj->maps.rodata), NULL);
> +       CHECK(err, "rebind_map", "errno %d, expected 0", errno);
> +
> +close_bpf_object:
> +       metadata_used__destroy(obj);
> +}
> +
> +void test_metadata(void)
> +{
> +       if (test__start_subtest("unused"))
> +               test_metadata_unused();
> +
> +       if (test__start_subtest("used"))
> +               test_metadata_used();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/metadata_unused.c b/tools/testing/selftests/bpf/progs/metadata_unused.c
> new file mode 100644
> index 000000000000..db5b804f6f4c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/metadata_unused.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +const char bpf_metadata_a[] SEC(".rodata") = "foo";
> +const int bpf_metadata_b SEC(".rodata") = 1;

please add volatile to ensure these are not optimized away

> +
> +SEC("cgroup_skb/egress")
> +int prog(struct xdp_md *ctx)
> +{
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/metadata_used.c b/tools/testing/selftests/bpf/progs/metadata_used.c
> new file mode 100644
> index 000000000000..0dcb1ba2f0ae
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/metadata_used.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +const char bpf_metadata_a[] SEC(".rodata") = "bar";
> +const int bpf_metadata_b SEC(".rodata") = 2;

same about volatile

> +
> +SEC("cgroup_skb/egress")
> +int prog(struct xdp_md *ctx)
> +{
> +       return bpf_metadata_b ? 1 : 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";

[...]

On Thu, Sep 10, 2020 at 12:59 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > From: YiFei Zhu <zhuyifei@google.com>
> >
> > This is a simple test to check that loading and dumping metadata
> > in btftool works, whether or not metadata contents are used by the
> > program.
> >
> > A C test is also added to make sure the skeleton code can read the
> > metadata values.
> >
> > Cc: YiFei Zhu <zhuyifei1999@gmail.com>
> > Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
>
> It would be good to test that libbpf does bind .rodata even if BPF
> program doesn't use it. So for metadata_unused you can get
> bpf_prog_info and check that it does contain the id of .rodata?
Good idea, will add that.

> > +const char bpf_metadata_a[] SEC(".rodata") = "foo";
> > +const int bpf_metadata_b SEC(".rodata") = 1;
>
> please add volatile to ensure these are not optimized away
Ack, ty!