[PATCH v2 bpf-next 0/4] libbpf: deprecate legacy BPF map definitions

[PATCH v2 bpf-next 0/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

Officially deprecate legacy BPF map definitions in libbpf. They've been slated
for deprecation for a while in favor of more powerful BTF-defined map
definitions and this patch set adds warnings and a way to enforce this in
libbpf through LIBBPF_STRICT_MAP_DEFINITIONS strict mode flag.

Selftests are fixed up and updated, BPF documentation is updated, bpftool's
strict mode usage is adjusted to avoid breaking users unnecessarily.

v1->v2:
  - replace missed bpf_map_def case in Documentation/bpf/btf.rst (Alexei).

Andrii Nakryiko (4):
  selftests/bpf: fail build on compilation warning
  selftests/bpf: convert remaining legacy map definitions
  libbpf: deprecate legacy BPF map definitions
  docs/bpf: update BPF map definition example

 Documentation/bpf/btf.rst                     | 32 ++++++++-----------
 tools/bpf/bpftool/main.c                      |  9 +++++-
 tools/lib/bpf/bpf_helpers.h                   |  2 +-
 tools/lib/bpf/libbpf.c                        |  8 +++++
 tools/lib/bpf/libbpf_legacy.h                 |  5 +++
 tools/testing/selftests/bpf/Makefile          |  4 +--
 tools/testing/selftests/bpf/prog_tests/btf.c  |  4 +++
 .../bpf/progs/freplace_cls_redirect.c         | 12 +++----
 .../selftests/bpf/progs/sample_map_ret0.c     | 24 +++++++-------
 .../selftests/bpf/progs/test_btf_haskv.c      |  3 ++
 .../selftests/bpf/progs/test_btf_newkv.c      |  3 ++
 .../selftests/bpf/progs/test_btf_nokv.c       | 12 +++----
 .../bpf/progs/test_skb_cgroup_id_kern.c       | 12 +++----
 .../testing/selftests/bpf/progs/test_tc_edt.c | 12 +++----
 .../bpf/progs/test_tcp_check_syncookie_kern.c | 12 +++----
 15 files changed, 90 insertions(+), 64 deletions(-)

-- 
2.30.2

[PATCH v2 bpf-next 1/4] selftests/bpf: fail build on compilation warning

[Andrii Nakryiko]

It's very easy to miss compilation warnings without -Werror, which is
not set for selftests. libbpf and bpftool are already strict about this,
so make selftests/bpf also treat compilation warnings as errors to catch
such regressions early.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 42ffc24e9e71..945f92d71db3 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -21,7 +21,7 @@ endif
 
 BPF_GCC		?= $(shell command -v bpf-gcc;)
 SAN_CFLAGS	?=
-CFLAGS += -g -O0 -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS)		\
+CFLAGS += -g -O0 -rdynamic -Wall -Werror $(GENFLAGS) $(SAN_CFLAGS)	\
 	  -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)		\
 	  -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
 LDFLAGS += $(SAN_CFLAGS)
@@ -292,7 +292,7 @@ IS_LITTLE_ENDIAN = $(shell $(CC) -dM -E - </dev/null | \
 MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
 
 CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
-BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) 			\
+BPF_CFLAGS = -g -Werror -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) 		\
 	     -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR)			\
 	     -I$(abspath $(OUTPUT)/../usr/include)
 
-- 
2.30.2

[PATCH v2 bpf-next 2/4] selftests/bpf: convert remaining legacy map definitions

[Andrii Nakryiko]

Converted few remaining legacy BPF map definition to BTF-defined ones.
For the remaining two bpf_map_def-based legacy definitions that we want
to keep for testing purposes until libbpf 1.0 release, guard them in
pragma to suppres deprecation warnings which will be added in libbpf in
the next commit.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/progs/freplace_cls_redirect.c         | 12 +++++-----
 .../selftests/bpf/progs/sample_map_ret0.c     | 24 +++++++++----------
 .../selftests/bpf/progs/test_btf_haskv.c      |  3 +++
 .../selftests/bpf/progs/test_btf_newkv.c      |  3 +++
 .../selftests/bpf/progs/test_btf_nokv.c       | 12 +++++-----
 .../bpf/progs/test_skb_cgroup_id_kern.c       | 12 +++++-----
 .../testing/selftests/bpf/progs/test_tc_edt.c | 12 +++++-----
 .../bpf/progs/test_tcp_check_syncookie_kern.c | 12 +++++-----
 8 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/freplace_cls_redirect.c b/tools/testing/selftests/bpf/progs/freplace_cls_redirect.c
index 68a5a9db928a..7e94412d47a5 100644
--- a/tools/testing/selftests/bpf/progs/freplace_cls_redirect.c
+++ b/tools/testing/selftests/bpf/progs/freplace_cls_redirect.c
@@ -7,12 +7,12 @@
 #include <bpf/bpf_endian.h>
 #include <bpf/bpf_helpers.h>
 
-struct bpf_map_def SEC("maps") sock_map = {
-	.type = BPF_MAP_TYPE_SOCKMAP,
-	.key_size = sizeof(int),
-	.value_size = sizeof(int),
-	.max_entries = 2,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKMAP);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, 2);
+} sock_map SEC(".maps");
 
 SEC("freplace/cls_redirect")
 int freplace_cls_redirect_test(struct __sk_buff *skb)
diff --git a/tools/testing/selftests/bpf/progs/sample_map_ret0.c b/tools/testing/selftests/bpf/progs/sample_map_ret0.c
index 1612a32007b6..495990d355ef 100644
--- a/tools/testing/selftests/bpf/progs/sample_map_ret0.c
+++ b/tools/testing/selftests/bpf/progs/sample_map_ret0.c
@@ -2,19 +2,19 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
-struct bpf_map_def SEC("maps") htab = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(__u32),
-	.value_size = sizeof(long),
-	.max_entries = 2,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, __u32);
+	__type(value, long);
+	__uint(max_entries, 2);
+} htab SEC(".maps");
 
-struct bpf_map_def SEC("maps") array = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(__u32),
-	.value_size = sizeof(long),
-	.max_entries = 2,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, __u32);
+	__type(value, long);
+	__uint(max_entries, 2);
+} array SEC(".maps");
 
 /* Sample program which should always load for testing control paths. */
 SEC(".text") int func()
diff --git a/tools/testing/selftests/bpf/progs/test_btf_haskv.c b/tools/testing/selftests/bpf/progs/test_btf_haskv.c
index 160ead6c67b2..07c94df13660 100644
--- a/tools/testing/selftests/bpf/progs/test_btf_haskv.c
+++ b/tools/testing/selftests/bpf/progs/test_btf_haskv.c
@@ -9,12 +9,15 @@ struct ipv_counts {
 	unsigned int v6;
 };
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 struct bpf_map_def SEC("maps") btf_map = {
 	.type = BPF_MAP_TYPE_ARRAY,
 	.key_size = sizeof(int),
 	.value_size = sizeof(struct ipv_counts),
 	.max_entries = 4,
 };
+#pragma GCC diagnostic pop
 
 BPF_ANNOTATE_KV_PAIR(btf_map, int, struct ipv_counts);
 
diff --git a/tools/testing/selftests/bpf/progs/test_btf_newkv.c b/tools/testing/selftests/bpf/progs/test_btf_newkv.c
index 1884a5bd10f5..762671a2e90c 100644
--- a/tools/testing/selftests/bpf/progs/test_btf_newkv.c
+++ b/tools/testing/selftests/bpf/progs/test_btf_newkv.c
@@ -9,6 +9,8 @@ struct ipv_counts {
 	unsigned int v6;
 };
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 /* just to validate we can handle maps in multiple sections */
 struct bpf_map_def SEC("maps") btf_map_legacy = {
 	.type = BPF_MAP_TYPE_ARRAY,
@@ -16,6 +18,7 @@ struct bpf_map_def SEC("maps") btf_map_legacy = {
 	.value_size = sizeof(long long),
 	.max_entries = 4,
 };
+#pragma GCC diagnostic pop
 
 BPF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts);
 
diff --git a/tools/testing/selftests/bpf/progs/test_btf_nokv.c b/tools/testing/selftests/bpf/progs/test_btf_nokv.c
index 15e0f9945fe4..1dabb88f8cb4 100644
--- a/tools/testing/selftests/bpf/progs/test_btf_nokv.c
+++ b/tools/testing/selftests/bpf/progs/test_btf_nokv.c
@@ -8,12 +8,12 @@ struct ipv_counts {
 	unsigned int v6;
 };
 
-struct bpf_map_def SEC("maps") btf_map = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(int),
-	.value_size = sizeof(struct ipv_counts),
-	.max_entries = 4,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(key_size, sizeof(int));
+	__uint(value_size, sizeof(struct ipv_counts));
+	__uint(max_entries, 4);
+} btf_map SEC(".maps");
 
 __attribute__((noinline))
 int test_long_fname_2(void)
diff --git a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c b/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
index c304cd5b8cad..37aacc66cd68 100644
--- a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
@@ -10,12 +10,12 @@
 
 #define NUM_CGROUP_LEVELS	4
 
-struct bpf_map_def SEC("maps") cgroup_ids = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(__u32),
-	.value_size = sizeof(__u64),
-	.max_entries = NUM_CGROUP_LEVELS,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, __u32);
+	__type(value, __u64);
+	__uint(max_entries, NUM_CGROUP_LEVELS);
+} cgroup_ids SEC(".maps");
 
 static __always_inline void log_nth_level(struct __sk_buff *skb, __u32 level)
 {
diff --git a/tools/testing/selftests/bpf/progs/test_tc_edt.c b/tools/testing/selftests/bpf/progs/test_tc_edt.c
index bf28814bfde5..950a70b61e74 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_edt.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_edt.c
@@ -17,12 +17,12 @@
 #define THROTTLE_RATE_BPS (5 * 1000 * 1000)
 
 /* flow_key => last_tstamp timestamp used */
-struct bpf_map_def SEC("maps") flow_map = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(uint32_t),
-	.value_size = sizeof(uint64_t),
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, uint32_t);
+	__type(value, uint64_t);
+	__uint(max_entries, 1);
+} flow_map SEC(".maps");
 
 static inline int throttle_flow(struct __sk_buff *skb)
 {
diff --git a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
index cd747cd93dbe..6edebce563b5 100644
--- a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
@@ -16,12 +16,12 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
-struct bpf_map_def SEC("maps") results = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(__u32),
-	.value_size = sizeof(__u32),
-	.max_entries = 3,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, __u32);
+	__type(value, __u32);
+	__uint(max_entries, 3);
+} results SEC(".maps");
 
 static __always_inline __s64 gen_syncookie(void *data_end, struct bpf_sock *sk,
 					   void *iph, __u32 ip_size,
-- 
2.30.2

[PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
for libbpf strict mode. If it is set, error out on any struct
bpf_map_def-based map definition. If not set, libbpf will print out
a warning for each legacy BPF map to raise awareness that it goes away.

For any use of BPF_ANNOTATE_KV_PAIR() macro providing a legacy way to
associate BTF key/value type information with legacy BPF map definition,
warn through libbpf's pr_warn() error message (but don't fail BPF object
open).

BPF-side struct bpf_map_def is marked as deprecated. User-space struct
bpf_map_def has to be used internally in libbpf, so it is left
untouched. It should be enough for bpf_map__def() to be marked
deprecated to raise awareness that it goes away.

bpftool is an interesting case that utilizes libbpf to open BPF ELF
object to generate skeleton. As such, even though bpftool itself uses
full on strict libbpf mode (LIBBPF_STRICT_ALL), it has to relax it a bit
for BPF map definition handling to minimize unnecessary disruptions. So
opt-out of LIBBPF_STRICT_MAP_DEFINITIONS for bpftool. User's code that
will later use generated skeleton will make its own decision whether to
enforce LIBBPF_STRICT_MAP_DEFINITIONS or not.

There are few tests in selftests/bpf that are consciously using legacy
BPF map definitions to test libbpf functionality. For those, temporary
opt out of LIBBPF_STRICT_MAP_DEFINITIONS mode for the duration of those
tests.

  [0] Closes: https://github.com/libbpf/libbpf/issues/272

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/main.c                     | 9 ++++++++-
 tools/lib/bpf/bpf_helpers.h                  | 2 +-
 tools/lib/bpf/libbpf.c                       | 8 ++++++++
 tools/lib/bpf/libbpf_legacy.h                | 5 +++++
 tools/testing/selftests/bpf/prog_tests/btf.c | 4 ++++
 5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 020e91a542d5..9d01fa9de033 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -478,7 +478,14 @@ int main(int argc, char **argv)
 	}
 
 	if (!legacy_libbpf) {
-		ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
+		enum libbpf_strict_mode mode;
+
+		/* Allow legacy map definitions for skeleton generation.
+		 * It will still be rejected if users use LIBBPF_STRICT_ALL
+		 * mode for loading generated skeleton.
+		 */
+		mode = (__LIBBPF_STRICT_LAST - 1) & ~LIBBPF_STRICT_MAP_DEFINITIONS;
+		ret = libbpf_set_strict_mode(mode);
 		if (ret)
 			p_err("failed to enable libbpf strict mode: %d", ret);
 	}
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 963b1060d944..44df982d2a5c 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -133,7 +133,7 @@ struct bpf_map_def {
 	unsigned int value_size;
 	unsigned int max_entries;
 	unsigned int map_flags;
-};
+} __attribute__((deprecated("use BTF-defined maps in .maps section")));
 
 enum libbpf_pin_type {
 	LIBBPF_PIN_NONE,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fdb3536afa7d..fc6d530e20db 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1937,6 +1937,11 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
 	if (obj->efile.maps_shndx < 0)
 		return 0;
 
+	if (libbpf_mode & LIBBPF_STRICT_MAP_DEFINITIONS) {
+		pr_warn("legacy map definitions in SEC(\"maps\") are not supported\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (!symbols)
 		return -EINVAL;
 
@@ -1999,6 +2004,8 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
 			return -LIBBPF_ERRNO__FORMAT;
 		}
 
+		pr_warn("map '%s' (legacy): legacy map definitions are deprecated, use BTF-defined maps instead\n", map_name);
+
 		if (ELF64_ST_BIND(sym->st_info) == STB_LOCAL) {
 			pr_warn("map '%s' (legacy): static maps are not supported\n", map_name);
 			return -ENOTSUP;
@@ -4190,6 +4197,7 @@ static int bpf_map_find_btf_info(struct bpf_object *obj, struct bpf_map *map)
 		return 0;
 
 	if (!bpf_map__is_internal(map)) {
+		pr_warn("Use of BPF_ANNOTATE_KV_PAIR is deprecated, use BTF-defined maps in .maps section instead\n");
 		ret = btf__get_map_kv_tids(obj->btf, map->name, def->key_size,
 					   def->value_size, &key_type_id,
 					   &value_type_id);
diff --git a/tools/lib/bpf/libbpf_legacy.h b/tools/lib/bpf/libbpf_legacy.h
index 79131f761a27..3c2b281c2bc3 100644
--- a/tools/lib/bpf/libbpf_legacy.h
+++ b/tools/lib/bpf/libbpf_legacy.h
@@ -73,6 +73,11 @@ enum libbpf_strict_mode {
 	 * operation.
 	 */
 	LIBBPF_STRICT_AUTO_RLIMIT_MEMLOCK = 0x10,
+	/*
+	 * Error out on any SEC("maps") map definition, which are deprecated
+	 * in favor of BTF-defined map definitions in SEC(".maps").
+	 */
+	LIBBPF_STRICT_MAP_DEFINITIONS = 0x20,
 
 	__LIBBPF_STRICT_LAST,
 };
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index 8ba53acf9eb4..14f9b6136794 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -4560,6 +4560,8 @@ static void do_test_file(unsigned int test_num)
 	has_btf_ext = btf_ext != NULL;
 	btf_ext__free(btf_ext);
 
+	/* temporary disable LIBBPF_STRICT_MAP_DEFINITIONS to test legacy maps */
+	libbpf_set_strict_mode((__LIBBPF_STRICT_LAST - 1) & ~LIBBPF_STRICT_MAP_DEFINITIONS);
 	obj = bpf_object__open(test->file);
 	err = libbpf_get_error(obj);
 	if (CHECK(err, "obj: %d", err))
@@ -4684,6 +4686,8 @@ static void do_test_file(unsigned int test_num)
 	fprintf(stderr, "OK");
 
 done:
+	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
+
 	btf__free(btf);
 	free(func_info);
 	bpf_object__close(obj);
-- 
2.30.2

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii@kernel.org> writes:

> Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> for libbpf strict mode. If it is set, error out on any struct
> bpf_map_def-based map definition. If not set, libbpf will print out
> a warning for each legacy BPF map to raise awareness that it goes
> away.

We've touched upon this subject before, but I (still) don't think it's a
good idea to remove this support entirely: It makes it impossible to
write a loader that can handle both new and old BPF objects.

So discourage the use of the old map definitions, sure, but please don't
make it completely impossible to load such objects.

-Toke

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii@kernel.org> writes:
>
> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> > for libbpf strict mode. If it is set, error out on any struct
> > bpf_map_def-based map definition. If not set, libbpf will print out
> > a warning for each legacy BPF map to raise awareness that it goes
> > away.
>
> We've touched upon this subject before, but I (still) don't think it's a
> good idea to remove this support entirely: It makes it impossible to
> write a loader that can handle both new and old BPF objects.
>
> So discourage the use of the old map definitions, sure, but please don't
> make it completely impossible to load such objects.

BTF-defined maps have been around for quite a long time now and only
have benefits on top of the bpf_map_def way. The source code
translation is also very straightforward. If someone didn't get around
to update their BPF program in 2 years, I don't think we can do much
about that.

Maybe instead of trying to please everyone (especially those that
refuse to do anything to their BPF programs), let's work together to
nudge laggards to actually modernize their source code a little bit
and gain some benefits from that along the way? It's the same thinking
with stricter section names, and all the other backwards incompatible
changes that libbpf 1.0 will do.

If you absolutely cannot afford to drop support for all the
to-be-removed things from libbpf, you'll have to stick to 0.x libbpf
version. I assume (it will be up to disto maintainers, I suppose)
you'll have that option.


>
> -Toke
>

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii@kernel.org> writes:
>>
>> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
>> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
>> > for libbpf strict mode. If it is set, error out on any struct
>> > bpf_map_def-based map definition. If not set, libbpf will print out
>> > a warning for each legacy BPF map to raise awareness that it goes
>> > away.
>>
>> We've touched upon this subject before, but I (still) don't think it's a
>> good idea to remove this support entirely: It makes it impossible to
>> write a loader that can handle both new and old BPF objects.
>>
>> So discourage the use of the old map definitions, sure, but please don't
>> make it completely impossible to load such objects.
>
> BTF-defined maps have been around for quite a long time now and only
> have benefits on top of the bpf_map_def way. The source code
> translation is also very straightforward. If someone didn't get around
> to update their BPF program in 2 years, I don't think we can do much
> about that.
>
> Maybe instead of trying to please everyone (especially those that
> refuse to do anything to their BPF programs), let's work together to
> nudge laggards to actually modernize their source code a little bit
> and gain some benefits from that along the way?

I'm completely fine with nudging people towards the newer features, and
I think the compile-time deprecation warning when someone is using the
old-style map definitions in their BPF programs is an excellent way to
do that. 

I'm also fine with libbpf *by default* refusing to load programs that
use the old-style map definitions, but if the code is removed completely
it becomes impossible to write general-purpose loaders that can handle
both old and new programs. The obvious example of such a loader is
iproute2, the loader in xdp-tools is another.

> It's the same thinking with stricter section names, and all the other
> backwards incompatible changes that libbpf 1.0 will do.

If the plan is to refuse entirely to load programs that use the older
section names, then I obviously have the same objection to that idea :)

> If you absolutely cannot afford to drop support for all the
> to-be-removed things from libbpf, you'll have to stick to 0.x libbpf
> version. I assume (it will be up to disto maintainers, I suppose)
> you'll have that option.

As in, you expect distributions to package up the old libbpf in a
separate package? Really?

But either way, that doesn't really help; it just makes it a choice
between supporting new or old programs. Can't very well link to two
versions of the same library...

I really don't get why you're so insistent on removing that code either;
it's not like it's code that has a lot of churn (by definition), nor is
it very much code in the first place. But if it's a question of
maintenance burden I'm happy to help maintain it; or we could find some
other way of letting applications hook into the ELF object parsing so
the code doesn't have to live inside libbpf proper if that's more to you
liking?

-Toke

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[David Ahern]

On 1/21/22 1:43 PM, Toke Høiland-Jørgensen wrote:
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> 
>> On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>>
>>> Andrii Nakryiko <andrii@kernel.org> writes:
>>>
>>>> Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
>>>> the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
>>>> for libbpf strict mode. If it is set, error out on any struct
>>>> bpf_map_def-based map definition. If not set, libbpf will print out
>>>> a warning for each legacy BPF map to raise awareness that it goes
>>>> away.
>>>
>>> We've touched upon this subject before, but I (still) don't think it's a
>>> good idea to remove this support entirely: It makes it impossible to
>>> write a loader that can handle both new and old BPF objects.
>>>
>>> So discourage the use of the old map definitions, sure, but please don't
>>> make it completely impossible to load such objects.
>>
>> BTF-defined maps have been around for quite a long time now and only
>> have benefits on top of the bpf_map_def way. The source code
>> translation is also very straightforward. If someone didn't get around
>> to update their BPF program in 2 years, I don't think we can do much
>> about that.
>>
>> Maybe instead of trying to please everyone (especially those that
>> refuse to do anything to their BPF programs), let's work together to
>> nudge laggards to actually modernize their source code a little bit
>> and gain some benefits from that along the way?
> 
> I'm completely fine with nudging people towards the newer features, and
> I think the compile-time deprecation warning when someone is using the
> old-style map definitions in their BPF programs is an excellent way to
> do that. 
> 
> I'm also fine with libbpf *by default* refusing to load programs that
> use the old-style map definitions, but if the code is removed completely
> it becomes impossible to write general-purpose loaders that can handle
> both old and new programs. The obvious example of such a loader is
> iproute2, the loader in xdp-tools is another.
> 

I agree with Toke's response.

2 years is a very small amount of time when it comes to OS and kernel
versions. Many companies base products on enterprise distributions and
run them for 10+ years. During that time there will be needs to update
some components - like kernel version or some tool but that is done with
the least amount of churn possible. Every update has the potential to
bring in unknown behavior changes. Requiring updates to entire tool
chains, multiple tool sets and libraries to accommodate some deprecation
will only hinder being able to update anything.

Further, programs (e.g., debugging as just one example) can and will
need to be used across many OS and kernel versions.

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

On Fri, Jan 21, 2022 at 2:04 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/21/22 1:43 PM, Toke Høiland-Jørgensen wrote:
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >
> >> On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>>
> >>> Andrii Nakryiko <andrii@kernel.org> writes:
> >>>
> >>>> Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> >>>> the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> >>>> for libbpf strict mode. If it is set, error out on any struct
> >>>> bpf_map_def-based map definition. If not set, libbpf will print out
> >>>> a warning for each legacy BPF map to raise awareness that it goes
> >>>> away.
> >>>
> >>> We've touched upon this subject before, but I (still) don't think it's a
> >>> good idea to remove this support entirely: It makes it impossible to
> >>> write a loader that can handle both new and old BPF objects.
> >>>
> >>> So discourage the use of the old map definitions, sure, but please don't
> >>> make it completely impossible to load such objects.
> >>
> >> BTF-defined maps have been around for quite a long time now and only
> >> have benefits on top of the bpf_map_def way. The source code
> >> translation is also very straightforward. If someone didn't get around
> >> to update their BPF program in 2 years, I don't think we can do much
> >> about that.
> >>
> >> Maybe instead of trying to please everyone (especially those that
> >> refuse to do anything to their BPF programs), let's work together to
> >> nudge laggards to actually modernize their source code a little bit
> >> and gain some benefits from that along the way?
> >
> > I'm completely fine with nudging people towards the newer features, and
> > I think the compile-time deprecation warning when someone is using the
> > old-style map definitions in their BPF programs is an excellent way to
> > do that.
> >
> > I'm also fine with libbpf *by default* refusing to load programs that
> > use the old-style map definitions, but if the code is removed completely
> > it becomes impossible to write general-purpose loaders that can handle
> > both old and new programs. The obvious example of such a loader is
> > iproute2, the loader in xdp-tools is another.
> >
>
> I agree with Toke's response.
>
> 2 years is a very small amount of time when it comes to OS and kernel
> versions. Many companies base products on enterprise distributions and
> run them for 10+ years. During that time there will be needs to update
> some components - like kernel version or some tool but that is done with
> the least amount of churn possible. Every update has the potential to
> bring in unknown behavior changes. Requiring updates to entire tool
> chains, multiple tool sets and libraries to accommodate some deprecation
> will only hinder being able to update anything.
>
> Further, programs (e.g., debugging as just one example) can and will
> need to be used across many OS and kernel versions.

Which is why all the things that are being deprecated have better
alternatives that work *right now* with libbpf v0.x and will keep
working with v1.x.

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii@kernel.org> writes:
> >>
> >> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> >> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> >> > for libbpf strict mode. If it is set, error out on any struct
> >> > bpf_map_def-based map definition. If not set, libbpf will print out
> >> > a warning for each legacy BPF map to raise awareness that it goes
> >> > away.
> >>
> >> We've touched upon this subject before, but I (still) don't think it's a
> >> good idea to remove this support entirely: It makes it impossible to
> >> write a loader that can handle both new and old BPF objects.
> >>
> >> So discourage the use of the old map definitions, sure, but please don't
> >> make it completely impossible to load such objects.
> >
> > BTF-defined maps have been around for quite a long time now and only
> > have benefits on top of the bpf_map_def way. The source code
> > translation is also very straightforward. If someone didn't get around
> > to update their BPF program in 2 years, I don't think we can do much
> > about that.
> >
> > Maybe instead of trying to please everyone (especially those that
> > refuse to do anything to their BPF programs), let's work together to
> > nudge laggards to actually modernize their source code a little bit
> > and gain some benefits from that along the way?
>
> I'm completely fine with nudging people towards the newer features, and
> I think the compile-time deprecation warning when someone is using the
> old-style map definitions in their BPF programs is an excellent way to
> do that.
>
> I'm also fine with libbpf *by default* refusing to load programs that
> use the old-style map definitions, but if the code is removed completely
> it becomes impossible to write general-purpose loaders that can handle
> both old and new programs. The obvious example of such a loader is
> iproute2, the loader in xdp-tools is another.

This is because you want to deviate from underlying BPF loader's
behavior and feature set and dictate your own extended feature set in
xdp-tools/iproute2/etc. You can technically do that, but with a lot of
added complexity and headaches. But demanding libbpf to maintain
deprecated and discouraged features/APIs/practices for 10+ years and
accumulate all the internal cruft and maintenance burden isn't a great
solution either.

As of right now, recent 0.x libbpf versions do support "old and new
programs", so there is always that option.

>
> > It's the same thinking with stricter section names, and all the other
> > backwards incompatible changes that libbpf 1.0 will do.
>
> If the plan is to refuse entirely to load programs that use the older
> section names, then I obviously have the same objection to that idea :)

I understand, but I disagree about keeping them in libbpf
indefinitely. That's why we have a major version bump at which point
backwards compatibility isn't guaranteed. And we did a lot to make
this transition smoother (all the libbpf_set_strict_mode()
shenanigans) and prepare to it (it's been almost a year now (!), and
we still have few more months).

>
> > If you absolutely cannot afford to drop support for all the
> > to-be-removed things from libbpf, you'll have to stick to 0.x libbpf
> > version. I assume (it will be up to disto maintainers, I suppose)
> > you'll have that option.
>
> As in, you expect distributions to package up the old libbpf in a
> separate package? Really?

NixOS indicated that they are planning to do just that ([0]). Is it a
problem to keep packaging libbpf.so.0 and libbpf.so.1 together?

  [0] https://github.com/libbpf/libbpf/issues/440#issuecomment-1016084088

>
> But either way, that doesn't really help; it just makes it a choice
> between supporting new or old programs. Can't very well link to two
> versions of the same library...

Oh, you probably can with dynamic shared library loading, but yeah,
big PITA for sure. But again, v0.x libbpf supports "new programs" for
current definition of new, if you absolutely insist on supporting
deprecated BPF object file features. I'd be happy if you could instead
nudge your users to modernize their BPF game and prepare for libbpf
1.0 early, though. They can do that easily do to the extra work that
we did for libbpf 1.0 transition period.

>
> I really don't get why you're so insistent on removing that code either;
> it's not like it's code that has a lot of churn (by definition), nor is
> it very much code in the first place. But if it's a question of

There is enough and it is a maintenance burden. And will be forever if
we don't take this chance to shed it and move everyone to better
designed approaches (BTF-based maps), which, BTW, were around for
about 2 years now. Hardly a novelty.

> maintenance burden I'm happy to help maintain it; or we could find some
> other way of letting applications hook into the ELF object parsing so
> the code doesn't have to live inside libbpf proper if that's more to you
> liking?
>
> -Toke
>

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[David Ahern]

On 1/24/22 9:15 AM, Andrii Nakryiko wrote:
> On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>>> On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>>>
>>>> Andrii Nakryiko <andrii@kernel.org> writes:
>>>>
>>>>> Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
>>>>> the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
>>>>> for libbpf strict mode. If it is set, error out on any struct
>>>>> bpf_map_def-based map definition. If not set, libbpf will print out
>>>>> a warning for each legacy BPF map to raise awareness that it goes
>>>>> away.
>>>>
>>>> We've touched upon this subject before, but I (still) don't think it's a
>>>> good idea to remove this support entirely: It makes it impossible to
>>>> write a loader that can handle both new and old BPF objects.
>>>>
>>>> So discourage the use of the old map definitions, sure, but please don't
>>>> make it completely impossible to load such objects.
>>>
>>> BTF-defined maps have been around for quite a long time now and only
>>> have benefits on top of the bpf_map_def way. The source code
>>> translation is also very straightforward. If someone didn't get around
>>> to update their BPF program in 2 years, I don't think we can do much
>>> about that.
>>>
>>> Maybe instead of trying to please everyone (especially those that
>>> refuse to do anything to their BPF programs), let's work together to
>>> nudge laggards to actually modernize their source code a little bit
>>> and gain some benefits from that along the way?
>>
>> I'm completely fine with nudging people towards the newer features, and
>> I think the compile-time deprecation warning when someone is using the
>> old-style map definitions in their BPF programs is an excellent way to
>> do that.
>>
>> I'm also fine with libbpf *by default* refusing to load programs that
>> use the old-style map definitions, but if the code is removed completely
>> it becomes impossible to write general-purpose loaders that can handle
>> both old and new programs. The obvious example of such a loader is
>> iproute2, the loader in xdp-tools is another.
> 
> This is because you want to deviate from underlying BPF loader's
> behavior and feature set and dictate your own extended feature set in
> xdp-tools/iproute2/etc. You can technically do that, but with a lot of
> added complexity and headaches. But demanding libbpf to maintain
> deprecated and discouraged features/APIs/practices for 10+ years and
> accumulate all the internal cruft and maintenance burden isn't a great
> solution either.
> 
> As of right now, recent 0.x libbpf versions do support "old and new
> programs", so there is always that option.
> 
>>
>>> It's the same thinking with stricter section names, and all the other
>>> backwards incompatible changes that libbpf 1.0 will do.
>>
>> If the plan is to refuse entirely to load programs that use the older
>> section names, then I obviously have the same objection to that idea :)
> 
> I understand, but I disagree about keeping them in libbpf
> indefinitely. That's why we have a major version bump at which point
> backwards compatibility isn't guaranteed. And we did a lot to make
> this transition smoother (all the libbpf_set_strict_mode()
> shenanigans) and prepare to it (it's been almost a year now (!), and
> we still have few more months).
> 
>>
>>> If you absolutely cannot afford to drop support for all the
>>> to-be-removed things from libbpf, you'll have to stick to 0.x libbpf
>>> version. I assume (it will be up to disto maintainers, I suppose)
>>> you'll have that option.
>>
>> As in, you expect distributions to package up the old libbpf in a
>> separate package? Really?
> 
> NixOS indicated that they are planning to do just that ([0]). Is it a
> problem to keep packaging libbpf.so.0 and libbpf.so.1 together?
> 
>   [0] https://github.com/libbpf/libbpf/issues/440#issuecomment-1016084088
> 
>>
>> But either way, that doesn't really help; it just makes it a choice
>> between supporting new or old programs. Can't very well link to two
>> versions of the same library...
> 
> Oh, you probably can with dynamic shared library loading, but yeah,
> big PITA for sure. But again, v0.x libbpf supports "new programs" for
> current definition of new, if you absolutely insist on supporting
> deprecated BPF object file features. I'd be happy if you could instead
> nudge your users to modernize their BPF game and prepare for libbpf
> 1.0 early, though. They can do that easily do to the extra work that
> we did for libbpf 1.0 transition period.
> 
>>
>> I really don't get why you're so insistent on removing that code either;
>> it's not like it's code that has a lot of churn (by definition), nor is
>> it very much code in the first place. But if it's a question of
> 
> There is enough and it is a maintenance burden. And will be forever if
> we don't take this chance to shed it and move everyone to better
> designed approaches (BTF-based maps), which, BTW, were around for
> about 2 years now. Hardly a novelty.
> 

And it does not work everywhere.

When support for libbpf was added to iproute2, my biggest concern was
the stability of the library -- that exported APIs and supported
features would be arbitrarily changed and that is exactly what you are
doing with this push to v1.0. iproute2 cares about forward and backward
compatibility. If a tc program loads and runs on kernel version X with
iproute2 version Y, it should continue to work with kernel version X+M
and iproute2 version Y+N. No change should be required to the program at
all.

In this specific example, you are not removing support for old map
definitions for security reasons or bug reasons; you want to remove it
because there is a new definition and removing support for the older
definition forces people to move to the new style. You are trying to
force people to use a feature they may not care about at all or even need.

Ubuntu 18.04 is an LTS and will be around for a long time. ebpf programs
build and work just fine but the OS does not support BTF. Deprecating
support for older maps means people using say 18.04 and 22.04 can not
use the same object files on both servers which means code bases have to
deal with differences in definitions and build rules. Not user friendly
at all.

glibc manages to retain support for old system calls even as new
variants are added. That is part of the burden a library takes on for
its users. Your forced deprecation in short time windows (and 2 years is
a very short time window for OS'es) is just going to cause headaches -
like splits where code bases have to jump through hoops to stay on pre-1.0.

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

On Mon, Jan 24, 2022 at 4:27 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/24/22 9:15 AM, Andrii Nakryiko wrote:
> > On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >>> On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>>>
> >>>> Andrii Nakryiko <andrii@kernel.org> writes:
> >>>>
> >>>>> Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> >>>>> the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> >>>>> for libbpf strict mode. If it is set, error out on any struct
> >>>>> bpf_map_def-based map definition. If not set, libbpf will print out
> >>>>> a warning for each legacy BPF map to raise awareness that it goes
> >>>>> away.
> >>>>
> >>>> We've touched upon this subject before, but I (still) don't think it's a
> >>>> good idea to remove this support entirely: It makes it impossible to
> >>>> write a loader that can handle both new and old BPF objects.
> >>>>
> >>>> So discourage the use of the old map definitions, sure, but please don't
> >>>> make it completely impossible to load such objects.
> >>>
> >>> BTF-defined maps have been around for quite a long time now and only
> >>> have benefits on top of the bpf_map_def way. The source code
> >>> translation is also very straightforward. If someone didn't get around
> >>> to update their BPF program in 2 years, I don't think we can do much
> >>> about that.
> >>>
> >>> Maybe instead of trying to please everyone (especially those that
> >>> refuse to do anything to their BPF programs), let's work together to
> >>> nudge laggards to actually modernize their source code a little bit
> >>> and gain some benefits from that along the way?
> >>
> >> I'm completely fine with nudging people towards the newer features, and
> >> I think the compile-time deprecation warning when someone is using the
> >> old-style map definitions in their BPF programs is an excellent way to
> >> do that.
> >>
> >> I'm also fine with libbpf *by default* refusing to load programs that
> >> use the old-style map definitions, but if the code is removed completely
> >> it becomes impossible to write general-purpose loaders that can handle
> >> both old and new programs. The obvious example of such a loader is
> >> iproute2, the loader in xdp-tools is another.
> >
> > This is because you want to deviate from underlying BPF loader's
> > behavior and feature set and dictate your own extended feature set in
> > xdp-tools/iproute2/etc. You can technically do that, but with a lot of
> > added complexity and headaches. But demanding libbpf to maintain
> > deprecated and discouraged features/APIs/practices for 10+ years and
> > accumulate all the internal cruft and maintenance burden isn't a great
> > solution either.
> >
> > As of right now, recent 0.x libbpf versions do support "old and new
> > programs", so there is always that option.
> >
> >>
> >>> It's the same thinking with stricter section names, and all the other
> >>> backwards incompatible changes that libbpf 1.0 will do.
> >>
> >> If the plan is to refuse entirely to load programs that use the older
> >> section names, then I obviously have the same objection to that idea :)
> >
> > I understand, but I disagree about keeping them in libbpf
> > indefinitely. That's why we have a major version bump at which point
> > backwards compatibility isn't guaranteed. And we did a lot to make
> > this transition smoother (all the libbpf_set_strict_mode()
> > shenanigans) and prepare to it (it's been almost a year now (!), and
> > we still have few more months).
> >
> >>
> >>> If you absolutely cannot afford to drop support for all the
> >>> to-be-removed things from libbpf, you'll have to stick to 0.x libbpf
> >>> version. I assume (it will be up to disto maintainers, I suppose)
> >>> you'll have that option.
> >>
> >> As in, you expect distributions to package up the old libbpf in a
> >> separate package? Really?
> >
> > NixOS indicated that they are planning to do just that ([0]). Is it a
> > problem to keep packaging libbpf.so.0 and libbpf.so.1 together?
> >
> >   [0] https://github.com/libbpf/libbpf/issues/440#issuecomment-1016084088
> >
> >>
> >> But either way, that doesn't really help; it just makes it a choice
> >> between supporting new or old programs. Can't very well link to two
> >> versions of the same library...
> >
> > Oh, you probably can with dynamic shared library loading, but yeah,
> > big PITA for sure. But again, v0.x libbpf supports "new programs" for
> > current definition of new, if you absolutely insist on supporting
> > deprecated BPF object file features. I'd be happy if you could instead
> > nudge your users to modernize their BPF game and prepare for libbpf
> > 1.0 early, though. They can do that easily do to the extra work that
> > we did for libbpf 1.0 transition period.
> >
> >>
> >> I really don't get why you're so insistent on removing that code either;
> >> it's not like it's code that has a lot of churn (by definition), nor is
> >> it very much code in the first place. But if it's a question of
> >
> > There is enough and it is a maintenance burden. And will be forever if
> > we don't take this chance to shed it and move everyone to better
> > designed approaches (BTF-based maps), which, BTW, were around for
> > about 2 years now. Hardly a novelty.
> >
>
> And it does not work everywhere.

It does, see below.

>
> When support for libbpf was added to iproute2, my biggest concern was
> the stability of the library -- that exported APIs and supported
> features would be arbitrarily changed and that is exactly what you are
> doing with this push to v1.0. iproute2 cares about forward and backward
> compatibility. If a tc program loads and runs on kernel version X with
> iproute2 version Y, it should continue to work with kernel version X+M
> and iproute2 version Y+N. No change should be required to the program at
> all.
>
> In this specific example, you are not removing support for old map
> definitions for security reasons or bug reasons; you want to remove it
> because there is a new definition and removing support for the older
> definition forces people to move to the new style. You are trying to
> force people to use a feature they may not care about at all or even need.

It is a maintenance burden. Its implementation is also more
error-prone due to blind interpretation of bytes (there is still a
TODO comment from *2016-11-15* mentioning the need to detect an array
of maps and report error; guess what, it never got implemented). Don't
know if you'd like to classify it as either security or bug related.
Doesn't really matter.

Also, it's confusing for new BPF users to have two ways of defining
maps when one is strictly better and is more future-proof.

>
> Ubuntu 18.04 is an LTS and will be around for a long time. ebpf programs
> build and work just fine but the OS does not support BTF. Deprecating

Good for Ubuntu 18.04, but kernel support and knowledge of BTF (I
assume that's what you meant by "OS support") has absolutely nothing
to do with BTF-defined maps. BTF in BTF-defined maps is used by libbpf
itself to parse the definition of the map. If the kernel supports BTF
and map definition provides types of keys and values, then yes, that
BTF will be *optionally* provided to the kernel as well to assist
tooling like bpftool. Libbpf goes to great length to do this very
seamlessly and painlessly for end users.

> support for older maps means people using say 18.04 and 22.04 can not
> use the same object files on both servers which means code bases have to
> deal with differences in definitions and build rules. Not user friendly
> at all.

Not true, which I hopefully explained clearly above.

>
> glibc manages to retain support for old system calls even as new
> variants are added. That is part of the burden a library takes on for
> its users. Your forced deprecation in short time windows (and 2 years is
> a very short time window for OS'es) is just going to cause headaches -
> like splits where code bases have to jump through hoops to stay on pre-1.0.

Let's agree to disagree about 2 years being a very short time window.
Also libbpf is not an OS. Also not sure which hoops you mean to stay
on libbpf.so.0, if necessary. I'm not even mentioning how much simpler
and user-friendlier it is to use statically linked libbpf, having full
control over which exact version and features are available to
iproute2 and its users. I doubt we'll ever see eye to eye on this one.

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> Andrii Nakryiko <andrii@kernel.org> writes:
>> >>
>> >> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
>> >> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
>> >> > for libbpf strict mode. If it is set, error out on any struct
>> >> > bpf_map_def-based map definition. If not set, libbpf will print out
>> >> > a warning for each legacy BPF map to raise awareness that it goes
>> >> > away.
>> >>
>> >> We've touched upon this subject before, but I (still) don't think it's a
>> >> good idea to remove this support entirely: It makes it impossible to
>> >> write a loader that can handle both new and old BPF objects.
>> >>
>> >> So discourage the use of the old map definitions, sure, but please don't
>> >> make it completely impossible to load such objects.
>> >
>> > BTF-defined maps have been around for quite a long time now and only
>> > have benefits on top of the bpf_map_def way. The source code
>> > translation is also very straightforward. If someone didn't get around
>> > to update their BPF program in 2 years, I don't think we can do much
>> > about that.
>> >
>> > Maybe instead of trying to please everyone (especially those that
>> > refuse to do anything to their BPF programs), let's work together to
>> > nudge laggards to actually modernize their source code a little bit
>> > and gain some benefits from that along the way?
>>
>> I'm completely fine with nudging people towards the newer features, and
>> I think the compile-time deprecation warning when someone is using the
>> old-style map definitions in their BPF programs is an excellent way to
>> do that.
>>
>> I'm also fine with libbpf *by default* refusing to load programs that
>> use the old-style map definitions, but if the code is removed completely
>> it becomes impossible to write general-purpose loaders that can handle
>> both old and new programs. The obvious example of such a loader is
>> iproute2, the loader in xdp-tools is another.
>
> This is because you want to deviate from underlying BPF loader's
> behavior and feature set and dictate your own extended feature set in
> xdp-tools/iproute2/etc. You can technically do that, but with a lot of
> added complexity and headaches. But demanding libbpf to maintain
> deprecated and discouraged features/APIs/practices for 10+ years and
> accumulate all the internal cruft and maintenance burden isn't a great
> solution either.

Right, so work with me to find a solution? I already suggested several
ideas, and you just keep repeating "just use the old library", which is
tantamount to saying "take a hike".

I'm perfectly fine with having to jump through some more hoops to load
old programs, and moving the old maps section parsing out of libbpf and
into the caller is fine as well; but then we'd need to add some hooks to
libbpf to create the maps inside the bpf_object. I can submit patches to
do this, but I'm not going to bother if you're just going to reject them
because you don't want to accommodate anything other than your way of
doing things :/

-Toke

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[John Fastabend]

Toke Høiland-Jørgensen wrote:
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> 
> > On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >> > On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >>
> >> >> Andrii Nakryiko <andrii@kernel.org> writes:
> >> >>
> >> >> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> >> >> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> >> >> > for libbpf strict mode. If it is set, error out on any struct
> >> >> > bpf_map_def-based map definition. If not set, libbpf will print out
> >> >> > a warning for each legacy BPF map to raise awareness that it goes
> >> >> > away.
> >> >>
> >> >> We've touched upon this subject before, but I (still) don't think it's a
> >> >> good idea to remove this support entirely: It makes it impossible to
> >> >> write a loader that can handle both new and old BPF objects.
> >> >>
> >> >> So discourage the use of the old map definitions, sure, but please don't
> >> >> make it completely impossible to load such objects.
> >> >
> >> > BTF-defined maps have been around for quite a long time now and only
> >> > have benefits on top of the bpf_map_def way. The source code
> >> > translation is also very straightforward. If someone didn't get around
> >> > to update their BPF program in 2 years, I don't think we can do much
> >> > about that.
> >> >
> >> > Maybe instead of trying to please everyone (especially those that
> >> > refuse to do anything to their BPF programs), let's work together to
> >> > nudge laggards to actually modernize their source code a little bit
> >> > and gain some benefits from that along the way?
> >>
> >> I'm completely fine with nudging people towards the newer features, and
> >> I think the compile-time deprecation warning when someone is using the
> >> old-style map definitions in their BPF programs is an excellent way to
> >> do that.
> >>
> >> I'm also fine with libbpf *by default* refusing to load programs that
> >> use the old-style map definitions, but if the code is removed completely
> >> it becomes impossible to write general-purpose loaders that can handle
> >> both old and new programs. The obvious example of such a loader is
> >> iproute2, the loader in xdp-tools is another.
> >
> > This is because you want to deviate from underlying BPF loader's
> > behavior and feature set and dictate your own extended feature set in
> > xdp-tools/iproute2/etc. You can technically do that, but with a lot of
> > added complexity and headaches. But demanding libbpf to maintain
> > deprecated and discouraged features/APIs/practices for 10+ years and
> > accumulate all the internal cruft and maintenance burden isn't a great
> > solution either.
> 
> Right, so work with me to find a solution? I already suggested several
> ideas, and you just keep repeating "just use the old library", which is
> tantamount to saying "take a hike".

I'll just throw my $.02 here as I'm reviewing. On major versions its
fairly common to not force API compat with the libs I'm used to working
with. Most recent example that comes to my mind (just did this yesterday
for example) was porting code into openssl3.x from older version. I
mumbled a bit, but still did it so that I could get my tools working on
latest and greatest.

Going from 0.x -> 1.0 seems reasonable to break compat, users don't
need to update immediately right? They can linger around on 0.x release
until they have some time or reason to jump onto 1.0? Distro's can
carry all versions for as long as necessary. Thats the value add of
distributions in my mind anyways. And a 0.x version somewhat implies
its not stable yet imo.

> 
> I'm perfectly fine with having to jump through some more hoops to load
> old programs, and moving the old maps section parsing out of libbpf and
> into the caller is fine as well; but then we'd need to add some hooks to
> libbpf to create the maps inside the bpf_object. I can submit patches to
> do this, but I'm not going to bother if you're just going to reject them
> because you don't want to accommodate anything other than your way of
> doing things :/

Can't xdp-tools run on 0.x for as long as wanted and flip over when
it is ready? Same for iproute2 'tc' loader? I'm not seeing what would
break except for random people trying to use tools in debug or experiments.
I would think most production use cases are not shelling out to tc
or xdp loaders and if so they must be managing the versioning
somehow for new/old features.

FWIW the dumb netlink based loader I wrote to attach create qdiscs
and attach filters is <100 lines of code so its not a huge lift if
you end up having to roll your own here.

The series is an ACK for me.

> 
> -Toke
> 

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Toke Høiland-Jørgensen]

John Fastabend <john.fastabend@gmail.com> writes:

> Toke Høiland-Jørgensen wrote:
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>> 
>> > On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>> >>
>> >> > On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >> >>
>> >> >> Andrii Nakryiko <andrii@kernel.org> writes:
>> >> >>
>> >> >> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
>> >> >> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
>> >> >> > for libbpf strict mode. If it is set, error out on any struct
>> >> >> > bpf_map_def-based map definition. If not set, libbpf will print out
>> >> >> > a warning for each legacy BPF map to raise awareness that it goes
>> >> >> > away.
>> >> >>
>> >> >> We've touched upon this subject before, but I (still) don't think it's a
>> >> >> good idea to remove this support entirely: It makes it impossible to
>> >> >> write a loader that can handle both new and old BPF objects.
>> >> >>
>> >> >> So discourage the use of the old map definitions, sure, but please don't
>> >> >> make it completely impossible to load such objects.
>> >> >
>> >> > BTF-defined maps have been around for quite a long time now and only
>> >> > have benefits on top of the bpf_map_def way. The source code
>> >> > translation is also very straightforward. If someone didn't get around
>> >> > to update their BPF program in 2 years, I don't think we can do much
>> >> > about that.
>> >> >
>> >> > Maybe instead of trying to please everyone (especially those that
>> >> > refuse to do anything to their BPF programs), let's work together to
>> >> > nudge laggards to actually modernize their source code a little bit
>> >> > and gain some benefits from that along the way?
>> >>
>> >> I'm completely fine with nudging people towards the newer features, and
>> >> I think the compile-time deprecation warning when someone is using the
>> >> old-style map definitions in their BPF programs is an excellent way to
>> >> do that.
>> >>
>> >> I'm also fine with libbpf *by default* refusing to load programs that
>> >> use the old-style map definitions, but if the code is removed completely
>> >> it becomes impossible to write general-purpose loaders that can handle
>> >> both old and new programs. The obvious example of such a loader is
>> >> iproute2, the loader in xdp-tools is another.
>> >
>> > This is because you want to deviate from underlying BPF loader's
>> > behavior and feature set and dictate your own extended feature set in
>> > xdp-tools/iproute2/etc. You can technically do that, but with a lot of
>> > added complexity and headaches. But demanding libbpf to maintain
>> > deprecated and discouraged features/APIs/practices for 10+ years and
>> > accumulate all the internal cruft and maintenance burden isn't a great
>> > solution either.
>> 
>> Right, so work with me to find a solution? I already suggested several
>> ideas, and you just keep repeating "just use the old library", which is
>> tantamount to saying "take a hike".
>
> I'll just throw my $.02 here as I'm reviewing. On major versions its
> fairly common to not force API compat with the libs I'm used to working
> with. Most recent example that comes to my mind (just did this yesterday
> for example) was porting code into openssl3.x from older version. I
> mumbled a bit, but still did it so that I could get my tools working on
> latest and greatest.
>
> Going from 0.x -> 1.0 seems reasonable to break compat, users don't
> need to update immediately right? They can linger around on 0.x release
> until they have some time or reason to jump onto 1.0? Distro's can
> carry all versions for as long as necessary. Thats the value add of
> distributions in my mind anyways. And a 0.x version somewhat implies
> its not stable yet imo.

I'm fine with breaking compatibility of the library. We already handle
that in xdp-tools via standard configure probing. The problem here is
with breaking compatibility the data file format (i.e., BPF ELF files);
in your openssl example that would correspond to new versions of openssl
refusing to read certificate files that were issued before the upgrade.

I really don't get why this distinction is so hard to explain? Is there
some mental model disconnect here somewhere, or something?

>> I'm perfectly fine with having to jump through some more hoops to load
>> old programs, and moving the old maps section parsing out of libbpf and
>> into the caller is fine as well; but then we'd need to add some hooks to
>> libbpf to create the maps inside the bpf_object. I can submit patches to
>> do this, but I'm not going to bother if you're just going to reject them
>> because you don't want to accommodate anything other than your way of
>> doing things :/
>
> Can't xdp-tools run on 0.x for as long as wanted and flip over when
> it is ready? Same for iproute2 'tc' loader? I'm not seeing what would
> break except for random people trying to use tools in debug or
> experiments.

New stuff would break. I.e., then xdp-tools / tc would be stuck on that
version forever, and wouldn't be able to load any BPF programs that rely
on features added to libbpf after 1.0.

> FWIW the dumb netlink based loader I wrote to attach create qdiscs and
> attach filters is <100 lines of code so its not a huge lift if you end
> up having to roll your own here.

We're not just talking about "creating qdiscs and attaching filters"
here, we're talking about the loading of BPF object files. "Rolling my
own" means writing code that parses elf files, populates maps, creates
them in the kernel, does the relocations etc. That's essentially a
rewrite / fork of libbpf, which is what I'm trying to avoid...

-Toke

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[John Fastabend]

Toke Høiland-Jørgensen wrote:
> John Fastabend <john.fastabend@gmail.com> writes:
> 
> > Toke Høiland-Jørgensen wrote:
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >> 
> >> > On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >>
> >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >> >>
> >> >> > On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >> >>
> >> >> >> Andrii Nakryiko <andrii@kernel.org> writes:
> >> >> >>
> >> >> >> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> >> >> >> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> >> >> >> > for libbpf strict mode. If it is set, error out on any struct
> >> >> >> > bpf_map_def-based map definition. If not set, libbpf will print out
> >> >> >> > a warning for each legacy BPF map to raise awareness that it goes
> >> >> >> > away.
> >> >> >>
> >> >> >> We've touched upon this subject before, but I (still) don't think it's a
> >> >> >> good idea to remove this support entirely: It makes it impossible to
> >> >> >> write a loader that can handle both new and old BPF objects.
> >> >> >>
> >> >> >> So discourage the use of the old map definitions, sure, but please don't
> >> >> >> make it completely impossible to load such objects.
> >> >> >
> >> >> > BTF-defined maps have been around for quite a long time now and only
> >> >> > have benefits on top of the bpf_map_def way. The source code
> >> >> > translation is also very straightforward. If someone didn't get around
> >> >> > to update their BPF program in 2 years, I don't think we can do much
> >> >> > about that.
> >> >> >
> >> >> > Maybe instead of trying to please everyone (especially those that
> >> >> > refuse to do anything to their BPF programs), let's work together to
> >> >> > nudge laggards to actually modernize their source code a little bit
> >> >> > and gain some benefits from that along the way?
> >> >>
> >> >> I'm completely fine with nudging people towards the newer features, and
> >> >> I think the compile-time deprecation warning when someone is using the
> >> >> old-style map definitions in their BPF programs is an excellent way to
> >> >> do that.
> >> >>
> >> >> I'm also fine with libbpf *by default* refusing to load programs that
> >> >> use the old-style map definitions, but if the code is removed completely
> >> >> it becomes impossible to write general-purpose loaders that can handle
> >> >> both old and new programs. The obvious example of such a loader is
> >> >> iproute2, the loader in xdp-tools is another.
> >> >
> >> > This is because you want to deviate from underlying BPF loader's
> >> > behavior and feature set and dictate your own extended feature set in
> >> > xdp-tools/iproute2/etc. You can technically do that, but with a lot of
> >> > added complexity and headaches. But demanding libbpf to maintain
> >> > deprecated and discouraged features/APIs/practices for 10+ years and
> >> > accumulate all the internal cruft and maintenance burden isn't a great
> >> > solution either.
> >> 
> >> Right, so work with me to find a solution? I already suggested several
> >> ideas, and you just keep repeating "just use the old library", which is
> >> tantamount to saying "take a hike".
> >
> > I'll just throw my $.02 here as I'm reviewing. On major versions its
> > fairly common to not force API compat with the libs I'm used to working
> > with. Most recent example that comes to my mind (just did this yesterday
> > for example) was porting code into openssl3.x from older version. I
> > mumbled a bit, but still did it so that I could get my tools working on
> > latest and greatest.
> >
> > Going from 0.x -> 1.0 seems reasonable to break compat, users don't
> > need to update immediately right? They can linger around on 0.x release
> > until they have some time or reason to jump onto 1.0? Distro's can
> > carry all versions for as long as necessary. Thats the value add of
> > distributions in my mind anyways. And a 0.x version somewhat implies
> > its not stable yet imo.
> 
> I'm fine with breaking compatibility of the library. We already handle
> that in xdp-tools via standard configure probing. The problem here is
> with breaking compatibility the data file format (i.e., BPF ELF files);
> in your openssl example that would correspond to new versions of openssl
> refusing to read certificate files that were issued before the upgrade.
> 
> I really don't get why this distinction is so hard to explain? Is there
> some mental model disconnect here somewhere, or something?

Ah I think the difference is, in my mental model a BPF Program is the
BPF object file, the loader code, user space components to manage BPF
maps/perf-rings/objects, and a bunch of other user space code to do
something useful with whatever is showing up in maps, perf ring, etc.
These are one program in my model. A BPF object on its own has little
value in my model. (A BPF lib on the other hand implementing common
functionality is very useful though) Even if the BPF object files are
coming from a different team we have to work closely together because
map value/keys have an API, perf-ring events have an API and so on.

I don't see it as a paticularly major problem if we break old things
here because the only things in my model that get loaded over these
loaders are debug progs and experimental code. Super useful stuff
by the way, but something I would expect a human to go 'oh it didn't
load' I guess I shouldn't have ignored the warning for the last
year and then they fix it. Or if it is a program shell'ing out to
the tool they manage the versions carefully so wouldn't upgrade
to latest version until their system is ready. I'm not seeing how
this would end up breaking a deployed production system.

So my mental model doesn't seem to have the same issues here of some
long lived/unmaintained and isolated BPF object file that doesn't have
close ties to the loader. A bit curious how you get these
BPF programs that are not changeable and don't control the loader.

> 
> >> I'm perfectly fine with having to jump through some more hoops to load
> >> old programs, and moving the old maps section parsing out of libbpf and
> >> into the caller is fine as well; but then we'd need to add some hooks to
> >> libbpf to create the maps inside the bpf_object. I can submit patches to
> >> do this, but I'm not going to bother if you're just going to reject them
> >> because you don't want to accommodate anything other than your way of
> >> doing things :/
> >
> > Can't xdp-tools run on 0.x for as long as wanted and flip over when
> > it is ready? Same for iproute2 'tc' loader? I'm not seeing what would
> > break except for random people trying to use tools in debug or
> > experiments.
> 
> New stuff would break. I.e., then xdp-tools / tc would be stuck on that
> version forever, and wouldn't be able to load any BPF programs that rely
> on features added to libbpf after 1.0.
> 
> > FWIW the dumb netlink based loader I wrote to attach create qdiscs and
> > attach filters is <100 lines of code so its not a huge lift if you end
> > up having to roll your own here.
> 
> We're not just talking about "creating qdiscs and attaching filters"
> here, we're talking about the loading of BPF object files. "Rolling my
> own" means writing code that parses elf files, populates maps, creates
> them in the kernel, does the relocations etc. That's essentially a
> rewrite / fork of libbpf, which is what I'm trying to avoid...

In practice I just forked/wrote my own loader code where needed and
libbpf didn't have what was needed. The thinking being my use case was so
niche it didn't make much sense to put in a general purpose lib and
burden everyone with the support/maintenance/cluter cost. I
think its ok for a library to not support all possible use cases.

.John

> 
> -Toke
> 

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

On Tue, Jan 25, 2022 at 2:35 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Toke Høiland-Jørgensen wrote:
> > John Fastabend <john.fastabend@gmail.com> writes:
> >
> > > Toke Høiland-Jørgensen wrote:
> > >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> > >>
> > >> > On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > >> >>
> > >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> > >> >>
> > >> >> > On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > >> >> >>
> > >> >> >> Andrii Nakryiko <andrii@kernel.org> writes:
> > >> >> >>
> > >> >> >> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> > >> >> >> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> > >> >> >> > for libbpf strict mode. If it is set, error out on any struct
> > >> >> >> > bpf_map_def-based map definition. If not set, libbpf will print out
> > >> >> >> > a warning for each legacy BPF map to raise awareness that it goes
> > >> >> >> > away.
> > >> >> >>
> > >> >> >> We've touched upon this subject before, but I (still) don't think it's a
> > >> >> >> good idea to remove this support entirely: It makes it impossible to
> > >> >> >> write a loader that can handle both new and old BPF objects.
> > >> >> >>
> > >> >> >> So discourage the use of the old map definitions, sure, but please don't
> > >> >> >> make it completely impossible to load such objects.
> > >> >> >
> > >> >> > BTF-defined maps have been around for quite a long time now and only
> > >> >> > have benefits on top of the bpf_map_def way. The source code
> > >> >> > translation is also very straightforward. If someone didn't get around
> > >> >> > to update their BPF program in 2 years, I don't think we can do much
> > >> >> > about that.
> > >> >> >
> > >> >> > Maybe instead of trying to please everyone (especially those that
> > >> >> > refuse to do anything to their BPF programs), let's work together to
> > >> >> > nudge laggards to actually modernize their source code a little bit
> > >> >> > and gain some benefits from that along the way?
> > >> >>
> > >> >> I'm completely fine with nudging people towards the newer features, and
> > >> >> I think the compile-time deprecation warning when someone is using the
> > >> >> old-style map definitions in their BPF programs is an excellent way to
> > >> >> do that.
> > >> >>
> > >> >> I'm also fine with libbpf *by default* refusing to load programs that
> > >> >> use the old-style map definitions, but if the code is removed completely
> > >> >> it becomes impossible to write general-purpose loaders that can handle
> > >> >> both old and new programs. The obvious example of such a loader is
> > >> >> iproute2, the loader in xdp-tools is another.
> > >> >
> > >> > This is because you want to deviate from underlying BPF loader's
> > >> > behavior and feature set and dictate your own extended feature set in
> > >> > xdp-tools/iproute2/etc. You can technically do that, but with a lot of
> > >> > added complexity and headaches. But demanding libbpf to maintain
> > >> > deprecated and discouraged features/APIs/practices for 10+ years and
> > >> > accumulate all the internal cruft and maintenance burden isn't a great
> > >> > solution either.
> > >>
> > >> Right, so work with me to find a solution? I already suggested several
> > >> ideas, and you just keep repeating "just use the old library", which is
> > >> tantamount to saying "take a hike".
> > >
> > > I'll just throw my $.02 here as I'm reviewing. On major versions its
> > > fairly common to not force API compat with the libs I'm used to working
> > > with. Most recent example that comes to my mind (just did this yesterday
> > > for example) was porting code into openssl3.x from older version. I
> > > mumbled a bit, but still did it so that I could get my tools working on
> > > latest and greatest.
> > >
> > > Going from 0.x -> 1.0 seems reasonable to break compat, users don't
> > > need to update immediately right? They can linger around on 0.x release
> > > until they have some time or reason to jump onto 1.0? Distro's can
> > > carry all versions for as long as necessary. Thats the value add of
> > > distributions in my mind anyways. And a 0.x version somewhat implies
> > > its not stable yet imo.
> >
> > I'm fine with breaking compatibility of the library. We already handle
> > that in xdp-tools via standard configure probing. The problem here is
> > with breaking compatibility the data file format (i.e., BPF ELF files);
> > in your openssl example that would correspond to new versions of openssl
> > refusing to read certificate files that were issued before the upgrade.

Comparing something so complex and rapidly evolving as BPF ecosystem,
which includes kernel, compiler, libbpf (and other BPF loaders) and
BPF-related tooling (like, bpftool) all interacting with each other to
loading a certificate file is a bit unfair, don't you think? Requiring
something that was written and compiled 2 years ago to work with a
complex BPF loader after a major version bump and a year+ warning
ahead of time is also a bit unfair.

Even extremely widely used programming languages, like C++ and its
standard library, do deprecate and remove stuff over time.

> >
> > I really don't get why this distinction is so hard to explain? Is there
> > some mental model disconnect here somewhere, or something?
>
> Ah I think the difference is, in my mental model a BPF Program is the
> BPF object file, the loader code, user space components to manage BPF
> maps/perf-rings/objects, and a bunch of other user space code to do
> something useful with whatever is showing up in maps, perf ring, etc.
> These are one program in my model. A BPF object on its own has little
> value in my model. (A BPF lib on the other hand implementing common
> functionality is very useful though) Even if the BPF object files are
> coming from a different team we have to work closely together because
> map value/keys have an API, perf-ring events have an API and so on.
>
> I don't see it as a paticularly major problem if we break old things
> here because the only things in my model that get loaded over these
> loaders are debug progs and experimental code. Super useful stuff
> by the way, but something I would expect a human to go 'oh it didn't
> load' I guess I shouldn't have ignored the warning for the last
> year and then they fix it. Or if it is a program shell'ing out to
> the tool they manage the versions carefully so wouldn't upgrade
> to latest version until their system is ready. I'm not seeing how
> this would end up breaking a deployed production system.

Great point, thanks!

If someone is unwilling to fix and recompile their BPF program, they
should be OK not upgrading iproute2 or whatnot.

>
> So my mental model doesn't seem to have the same issues here of some
> long lived/unmaintained and isolated BPF object file that doesn't have
> close ties to the loader. A bit curious how you get these
> BPF programs that are not changeable and don't control the loader.
>
> >
> > >> I'm perfectly fine with having to jump through some more hoops to load
> > >> old programs, and moving the old maps section parsing out of libbpf and
> > >> into the caller is fine as well; but then we'd need to add some hooks to
> > >> libbpf to create the maps inside the bpf_object. I can submit patches to
> > >> do this, but I'm not going to bother if you're just going to reject them
> > >> because you don't want to accommodate anything other than your way of
> > >> doing things :/
> > >
> > > Can't xdp-tools run on 0.x for as long as wanted and flip over when
> > > it is ready? Same for iproute2 'tc' loader? I'm not seeing what would
> > > break except for random people trying to use tools in debug or
> > > experiments.
> >
> > New stuff would break. I.e., then xdp-tools / tc would be stuck on that
> > version forever, and wouldn't be able to load any BPF programs that rely
> > on features added to libbpf after 1.0.
> >
> > > FWIW the dumb netlink based loader I wrote to attach create qdiscs and
> > > attach filters is <100 lines of code so its not a huge lift if you end
> > > up having to roll your own here.
> >
> > We're not just talking about "creating qdiscs and attaching filters"
> > here, we're talking about the loading of BPF object files. "Rolling my
> > own" means writing code that parses elf files, populates maps, creates
> > them in the kernel, does the relocations etc. That's essentially a
> > rewrite / fork of libbpf, which is what I'm trying to avoid...
>
> In practice I just forked/wrote my own loader code where needed and
> libbpf didn't have what was needed. The thinking being my use case was so
> niche it didn't make much sense to put in a general purpose lib and
> burden everyone with the support/maintenance/cluter cost. I
> think its ok for a library to not support all possible use cases.
>
> .John
>
> >
> > -Toke
> >

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[David Ahern]

You keep responding with “tell people to modernize their programs, their
toolchains, their ecosystem” - like they have nothing better to do with
their time than to chase the latest whims of upstream maintainers. You
are trivializing what you apparently have never done - ported a product
from one OS version to another and dealt with production issues with
many OS versions in play. If you have not lived it, no amount of writing
in an email is going to convince you of the work involved and the impact
of your decisions.

###

As far as I can tell, iproute2 does not directly care about this legacy
map case and should have limited exposure to the libbpf 1.0 changes in
general (time will tell if that is correct). My argument to this point
is exactly the 'loading a certificate case' Toke mentioned - an xdp or
tc program that loads on version X should load on version Y. That is
consistent with the general expectation of Linux users.

Re: [PATCH v2 bpf-next 3/4] libbpf: deprecate legacy BPF map definitions

[Andrii Nakryiko]

On Tue, Jan 25, 2022 at 4:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Fri, Jan 21, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >> > On Thu, Jan 20, 2022 at 3:44 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >>
> >> >> Andrii Nakryiko <andrii@kernel.org> writes:
> >> >>
> >> >> > Enact deprecation of legacy BPF map definition in SEC("maps") ([0]). For
> >> >> > the definitions themselves introduce LIBBPF_STRICT_MAP_DEFINITIONS flag
> >> >> > for libbpf strict mode. If it is set, error out on any struct
> >> >> > bpf_map_def-based map definition. If not set, libbpf will print out
> >> >> > a warning for each legacy BPF map to raise awareness that it goes
> >> >> > away.
> >> >>
> >> >> We've touched upon this subject before, but I (still) don't think it's a
> >> >> good idea to remove this support entirely: It makes it impossible to
> >> >> write a loader that can handle both new and old BPF objects.
> >> >>
> >> >> So discourage the use of the old map definitions, sure, but please don't
> >> >> make it completely impossible to load such objects.
> >> >
> >> > BTF-defined maps have been around for quite a long time now and only
> >> > have benefits on top of the bpf_map_def way. The source code
> >> > translation is also very straightforward. If someone didn't get around
> >> > to update their BPF program in 2 years, I don't think we can do much
> >> > about that.
> >> >
> >> > Maybe instead of trying to please everyone (especially those that
> >> > refuse to do anything to their BPF programs), let's work together to
> >> > nudge laggards to actually modernize their source code a little bit
> >> > and gain some benefits from that along the way?
> >>
> >> I'm completely fine with nudging people towards the newer features, and
> >> I think the compile-time deprecation warning when someone is using the
> >> old-style map definitions in their BPF programs is an excellent way to
> >> do that.
> >>
> >> I'm also fine with libbpf *by default* refusing to load programs that
> >> use the old-style map definitions, but if the code is removed completely
> >> it becomes impossible to write general-purpose loaders that can handle
> >> both old and new programs. The obvious example of such a loader is
> >> iproute2, the loader in xdp-tools is another.
> >
> > This is because you want to deviate from underlying BPF loader's
> > behavior and feature set and dictate your own extended feature set in
> > xdp-tools/iproute2/etc. You can technically do that, but with a lot of
> > added complexity and headaches. But demanding libbpf to maintain
> > deprecated and discouraged features/APIs/practices for 10+ years and
> > accumulate all the internal cruft and maintenance burden isn't a great
> > solution either.
>
> Right, so work with me to find a solution? I already suggested several
> ideas, and you just keep repeating "just use the old library", which is
> tantamount to saying "take a hike".

I also proposed a solution: log warning, so that your users will be
aware and can modernize their code base and be ready for libbpf 1.0.
You also keep ignoring this.

Adding more obscure APIs and callbacks to let libxdp or iproute2
emulate old libbpf map definition syntax is not a good solution from
my point of view.

>
> I'm perfectly fine with having to jump through some more hoops to load
> old programs, and moving the old maps section parsing out of libbpf and
> into the caller is fine as well; but then we'd need to add some hooks to
> libbpf to create the maps inside the bpf_object. I can submit patches to
> do this, but I'm not going to bother if you're just going to reject them
> because you don't want to accommodate anything other than your way of
> doing things :/

It's not just parsing the definition. We'll need to define an entire
new protocol to dynamically add new custom BPF maps, and tie them
together to BPF program code, adding/resolving relocations, etc. It's
an overkill and not a good solution.

You keep fighting hard to let users not do anything and use BPF object
files generated years ago without recompilation and any source code
changes. I so far haven't seen any *user* actually complain about
this, only "middleman" libxdp and iproute2 are complaining right now.
Did you check with your users how much of a problem it really is?

In practice I've seen BPF users are quite willing to accommodate much
more radical changes to their code with no problem. And John's reply
just adds to that case.

>
> -Toke
>

[PATCH v2 bpf-next 4/4] docs/bpf: update BPF map definition example

[Andrii Nakryiko]

Use BTF-defined map definition in the documentation example.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 Documentation/bpf/btf.rst | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
index 1ebf4c5c7ddc..ab08852e53ae 100644
--- a/Documentation/bpf/btf.rst
+++ b/Documentation/bpf/btf.rst
@@ -565,18 +565,15 @@ A map can be created with ``btf_fd`` and specified key/value type id.::
 In libbpf, the map can be defined with extra annotation like below:
 ::
 
-    struct bpf_map_def SEC("maps") btf_map = {
-        .type = BPF_MAP_TYPE_ARRAY,
-        .key_size = sizeof(int),
-        .value_size = sizeof(struct ipv_counts),
-        .max_entries = 4,
-    };
-    BPF_ANNOTATE_KV_PAIR(btf_map, int, struct ipv_counts);
+    struct {
+        __uint(type, BPF_MAP_TYPE_ARRAY);
+        __type(key, int);
+        __type(value, struct ipv_counts);
+        __uint(max_entries, 4);
+    } btf_map SEC(".maps");
 
-Here, the parameters for macro BPF_ANNOTATE_KV_PAIR are map name, key and
-value types for the map. During ELF parsing, libbpf is able to extract
-key/value type_id's and assign them to BPF_MAP_CREATE attributes
-automatically.
+During ELF parsing, libbpf is able to extract key/value type_id's and assign
+them to BPF_MAP_CREATE attributes automatically.
 
 .. _BPF_Prog_Load:
 
@@ -824,13 +821,12 @@ structure has bitfields. For example, for the following map,::
            ___A b1:4;
            enum A b2:4;
       };
-      struct bpf_map_def SEC("maps") tmpmap = {
-           .type = BPF_MAP_TYPE_ARRAY,
-           .key_size = sizeof(__u32),
-           .value_size = sizeof(struct tmp_t),
-           .max_entries = 1,
-      };
-      BPF_ANNOTATE_KV_PAIR(tmpmap, int, struct tmp_t);
+      struct {
+           __uint(type, BPF_MAP_TYPE_ARRAY);
+           __type(key, int);
+           __type(value, struct tmp_t);
+           __uint(max_entries, 1);
+      } tmpmap SEC(".maps");
 
 bpftool is able to pretty print like below:
 ::
-- 
2.30.2

RE: [PATCH v2 bpf-next 0/4] libbpf: deprecate legacy BPF map definitions

[John Fastabend]

Andrii Nakryiko wrote:
> Officially deprecate legacy BPF map definitions in libbpf. They've been slated
> for deprecation for a while in favor of more powerful BTF-defined map
> definitions and this patch set adds warnings and a way to enforce this in
> libbpf through LIBBPF_STRICT_MAP_DEFINITIONS strict mode flag.
> 
> Selftests are fixed up and updated, BPF documentation is updated, bpftool's
> strict mode usage is adjusted to avoid breaking users unnecessarily.
> 
> v1->v2:
>   - replace missed bpf_map_def case in Documentation/bpf/btf.rst (Alexei).

LGTM.

Acked-by: John Fastabend <john.fastabend@gmail.com>

> 
> Andrii Nakryiko (4):
>   selftests/bpf: fail build on compilation warning
>   selftests/bpf: convert remaining legacy map definitions
>   libbpf: deprecate legacy BPF map definitions
>   docs/bpf: update BPF map definition example
> 
>  Documentation/bpf/btf.rst                     | 32 ++++++++-----------
>  tools/bpf/bpftool/main.c                      |  9 +++++-
>  tools/lib/bpf/bpf_helpers.h                   |  2 +-
>  tools/lib/bpf/libbpf.c                        |  8 +++++
>  tools/lib/bpf/libbpf_legacy.h                 |  5 +++
>  tools/testing/selftests/bpf/Makefile          |  4 +--
>  tools/testing/selftests/bpf/prog_tests/btf.c  |  4 +++
>  .../bpf/progs/freplace_cls_redirect.c         | 12 +++----
>  .../selftests/bpf/progs/sample_map_ret0.c     | 24 +++++++-------
>  .../selftests/bpf/progs/test_btf_haskv.c      |  3 ++
>  .../selftests/bpf/progs/test_btf_newkv.c      |  3 ++
>  .../selftests/bpf/progs/test_btf_nokv.c       | 12 +++----
>  .../bpf/progs/test_skb_cgroup_id_kern.c       | 12 +++----
>  .../testing/selftests/bpf/progs/test_tc_edt.c | 12 +++----
>  .../bpf/progs/test_tcp_check_syncookie_kern.c | 12 +++----
>  15 files changed, 90 insertions(+), 64 deletions(-)
> 
> -- 
> 2.30.2
>