[PATCH bpf-next v2 0/6] skip verifier/map tests if kernel support is missing

[PATCH bpf-next v2 0/6] skip verifier/map tests if kernel support is missing

[Stanislav Fomichev]

If test_maps/test_verifier is running against the kernel which doesn't
have _all_ BPF features enabled, it fails with an error. This patch
series tries to probe kernel support for each failed test and skip
it instead. This lets users run BPF selftests in the not-all-bpf-yes
environments and received correct PASS/NON-PASS result.

See https://www.spinics.net/lists/netdev/msg539331.html for more
context.

The series goes like this:

* patch #1 adds bpf_prog_type_supported() and
  bpf_map_type_supported() which query the kernel (insert 'return 0'
  program or try to create empty map with correct key/value sizes) and
  return supported/unsupported.
  Note: this functionality can later be reimplemented on top of Quentin's
  recent 'bpftool feature' patchset if he decides to move the probes
  into libbpf.
* patch #2 skips sockmap tests in test_maps.c if BPF_MAP_TYPE_SOCKMAP
  map is not supported (if bpf_create_map fails, we probe the kernel
  for support)
* patch #3 skips verifier tests if test->prog_type is not supported (if
  bpf_verify_program fails, we probe the kernel for support)
* patch #4 skips verifier tests if test fixup map is not supported (if
  create_map fails, we probe the kernel for support)
  Note: we can probably move this probe into create_map helper and
  return some argument instead of adding skip_unsupported_map()
  to each fixup; but I'm not sure it's better.
  Also note: in current implementation we still print 'Failed to
  create hash map' from the create_map, but still skip the test.
* next patches fix various small issues that arise from the first four:
  * patch #5 sets "unknown func bpf_trace_printk#6" prog_type to
    BPF_PROG_TYPE_TRACEPOINT so it is correctly skipped in
    CONFIG_BPF_EVENTS=n case
  * patch #6 exposes BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} only when
    CONFIG_CGROUP_BPF=y, this makes verifier correctly skip appropriate
    tests

v2 changes:

* don't sprinkle "ifdef CONFIG_CGROUP_BPF" all around net/core/filter.c,
  doing it only in the bpf_types.h is enough to disable
  BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} prog types for non-cgroup
  enabled kernels

Stanislav Fomichev (6):
  selftests/bpf: add map/prog type probe helpers
  selftests/bpf: skip sockmap in test_maps if kernel doesn't have
    support
  selftests/bpf: skip verifier tests for unsupported program types
  selftests/bpf: skip verifier tests for unsupported map types
  selftests/bpf: mark verifier test that uses bpf_trace_printk as
    BPF_PROG_TYPE_TRACEPOINT
  bpf: BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} require cgroups enabled

 include/linux/bpf_types.h                   |  2 +
 tools/testing/selftests/bpf/Makefile        |  2 +
 tools/testing/selftests/bpf/probe_helpers.c | 68 +++++++++++++++++++++
 tools/testing/selftests/bpf/probe_helpers.h | 10 +++
 tools/testing/selftests/bpf/test_maps.c     | 14 ++++-
 tools/testing/selftests/bpf/test_verifier.c | 44 +++++++++++--
 6 files changed, 135 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/probe_helpers.c
 create mode 100644 tools/testing/selftests/bpf/probe_helpers.h

-- 
2.20.0.405.gbc1bbc6f85-goog

[PATCH bpf-next v2 1/6] selftests/bpf: add map/prog type probe helpers

[Stanislav Fomichev]

Export bpf_map_type_supported() and bpf_prog_type_supported() which
return true/false to indicate kernel support for the appropriate
program or map type. These helpers will be used in the next commits
to selectively skip test_verifier/test_maps tests.

bpf_map_type_supported() supports only limited set of maps for which we
do fixups in the test_verifier, for unknown maps it falls back to
'supported'.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/probe_helpers.c | 68 +++++++++++++++++++++
 tools/testing/selftests/bpf/probe_helpers.h | 10 +++
 2 files changed, 78 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/probe_helpers.c
 create mode 100644 tools/testing/selftests/bpf/probe_helpers.h

diff --git a/tools/testing/selftests/bpf/probe_helpers.c b/tools/testing/selftests/bpf/probe_helpers.c
new file mode 100644
index 000000000000..00467fdda813
--- /dev/null
+++ b/tools/testing/selftests/bpf/probe_helpers.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+#include <unistd.h>
+#include <bpf/bpf.h>
+
+#include "cgroup_helpers.h"
+#include "bpf_util.h"
+#include "../../../include/linux/filter.h"
+
+bool bpf_prog_type_supported(enum bpf_prog_type prog_type)
+{
+	struct bpf_load_program_attr attr;
+	struct bpf_insn insns[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+	int ret;
+
+	if (prog_type == BPF_PROG_TYPE_UNSPEC)
+		return true;
+
+	memset(&attr, 0, sizeof(attr));
+	attr.prog_type = prog_type;
+	attr.insns = insns;
+	attr.insns_cnt = ARRAY_SIZE(insns);
+	attr.license = "GPL";
+
+	ret = bpf_load_program_xattr(&attr, NULL, 0);
+	if (ret < 0)
+		return false;
+	close(ret);
+
+	return true;
+}
+
+bool bpf_map_type_supported(enum bpf_map_type map_type)
+{
+	int key_size, value_size, max_entries;
+	int fd;
+
+	key_size = sizeof(__u32);
+	value_size = sizeof(__u32);
+	max_entries = 1;
+
+	/* limited set of maps for test_verifier.c and test_maps.c */
+	switch (map_type) {
+	case BPF_MAP_TYPE_SOCKMAP:
+	case BPF_MAP_TYPE_SOCKHASH:
+	case BPF_MAP_TYPE_XSKMAP:
+		break;
+	case BPF_MAP_TYPE_STACK_TRACE:
+		value_size = sizeof(__u64);
+	case BPF_MAP_TYPE_CGROUP_STORAGE:
+	case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE:
+		key_size = sizeof(struct bpf_cgroup_storage_key);
+		value_size = sizeof(__u64);
+		max_entries = 0;
+		break;
+	default:
+		return true;
+	}
+
+	fd = bpf_create_map(map_type, key_size, value_size, max_entries, 0);
+	if (fd < 0)
+		return false;
+	close(fd);
+
+	return true;
+}
diff --git a/tools/testing/selftests/bpf/probe_helpers.h b/tools/testing/selftests/bpf/probe_helpers.h
new file mode 100644
index 000000000000..9a107d6fe936
--- /dev/null
+++ b/tools/testing/selftests/bpf/probe_helpers.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+#ifndef __PROBE_HELPERS_H
+#define __PROBE_HELPERS_H
+
+#include <linux/bpf.h>
+
+bool bpf_prog_type_supported(enum bpf_prog_type prog_type);
+bool bpf_map_type_supported(enum bpf_map_type map_type);
+
+#endif
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH bpf-next v2 1/6] selftests/bpf: add map/prog type probe helpers

[Daniel Borkmann]

On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
> Export bpf_map_type_supported() and bpf_prog_type_supported() which
> return true/false to indicate kernel support for the appropriate
> program or map type. These helpers will be used in the next commits
> to selectively skip test_verifier/test_maps tests.
> 
> bpf_map_type_supported() supports only limited set of maps for which we
> do fixups in the test_verifier, for unknown maps it falls back to
> 'supported'.
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/testing/selftests/bpf/probe_helpers.c | 68 +++++++++++++++++++++
>  tools/testing/selftests/bpf/probe_helpers.h | 10 +++
>  2 files changed, 78 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/probe_helpers.c
>  create mode 100644 tools/testing/selftests/bpf/probe_helpers.h
> 
> diff --git a/tools/testing/selftests/bpf/probe_helpers.c b/tools/testing/selftests/bpf/probe_helpers.c
> new file mode 100644
> index 000000000000..00467fdda813
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/probe_helpers.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +#include <unistd.h>
> +#include <bpf/bpf.h>
> +
> +#include "cgroup_helpers.h"
> +#include "bpf_util.h"
> +#include "../../../include/linux/filter.h"
> +
> +bool bpf_prog_type_supported(enum bpf_prog_type prog_type)
> +{
> +	struct bpf_load_program_attr attr;
> +	struct bpf_insn insns[] = {
> +		BPF_MOV64_IMM(BPF_REG_0, 0),
> +		BPF_EXIT_INSN(),
> +	};
> +	int ret;
> +
> +	if (prog_type == BPF_PROG_TYPE_UNSPEC)
> +		return true;
> +
> +	memset(&attr, 0, sizeof(attr));
> +	attr.prog_type = prog_type;
> +	attr.insns = insns;
> +	attr.insns_cnt = ARRAY_SIZE(insns);
> +	attr.license = "GPL";
> +
> +	ret = bpf_load_program_xattr(&attr, NULL, 0);
> +	if (ret < 0)
> +		return false;
> +	close(ret);
> +
> +	return true;
> +}
> +
> +bool bpf_map_type_supported(enum bpf_map_type map_type)
> +{
> +	int key_size, value_size, max_entries;
> +	int fd;
> +
> +	key_size = sizeof(__u32);
> +	value_size = sizeof(__u32);
> +	max_entries = 1;
> +
> +	/* limited set of maps for test_verifier.c and test_maps.c */
> +	switch (map_type) {
> +	case BPF_MAP_TYPE_SOCKMAP:
> +	case BPF_MAP_TYPE_SOCKHASH:
> +	case BPF_MAP_TYPE_XSKMAP:
> +		break;
> +	case BPF_MAP_TYPE_STACK_TRACE:
> +		value_size = sizeof(__u64);
> +	case BPF_MAP_TYPE_CGROUP_STORAGE:
> +	case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE:
> +		key_size = sizeof(struct bpf_cgroup_storage_key);
> +		value_size = sizeof(__u64);
> +		max_entries = 0;
> +		break;
> +	default:
> +		return true;

Given bpf_map_type_supported() is mainly called in the "unexpected" failure
case, it's probably okay to have this less restricted here. Also, ideally we
shouldn't need an explicit opt-in for every newly added map type to probe in
future. But presumably both limitation will be removed when using libbpf API.

> +	}
> +
> +	fd = bpf_create_map(map_type, key_size, value_size, max_entries, 0);
> +	if (fd < 0)
> +		return false;
> +	close(fd);
> +
> +	return true;
> +}
> diff --git a/tools/testing/selftests/bpf/probe_helpers.h b/tools/testing/selftests/bpf/probe_helpers.h
> new file mode 100644
> index 000000000000..9a107d6fe936
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/probe_helpers.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +#ifndef __PROBE_HELPERS_H
> +#define __PROBE_HELPERS_H
> +
> +#include <linux/bpf.h>
> +
> +bool bpf_prog_type_supported(enum bpf_prog_type prog_type);
> +bool bpf_map_type_supported(enum bpf_map_type map_type);
> +
> +#endif
> 

Re: [PATCH bpf-next v2 1/6] selftests/bpf: add map/prog type probe helpers

[Stanislav Fomichev]

On 12/19, Daniel Borkmann wrote:
> On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
> > Export bpf_map_type_supported() and bpf_prog_type_supported() which
> > return true/false to indicate kernel support for the appropriate
> > program or map type. These helpers will be used in the next commits
> > to selectively skip test_verifier/test_maps tests.
> > 
> > bpf_map_type_supported() supports only limited set of maps for which we
> > do fixups in the test_verifier, for unknown maps it falls back to
> > 'supported'.
> > 
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  tools/testing/selftests/bpf/probe_helpers.c | 68 +++++++++++++++++++++
> >  tools/testing/selftests/bpf/probe_helpers.h | 10 +++
> >  2 files changed, 78 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/probe_helpers.c
> >  create mode 100644 tools/testing/selftests/bpf/probe_helpers.h
> > 
> > diff --git a/tools/testing/selftests/bpf/probe_helpers.c b/tools/testing/selftests/bpf/probe_helpers.c
> > new file mode 100644
> > index 000000000000..00467fdda813
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/probe_helpers.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +#include <unistd.h>
> > +#include <bpf/bpf.h>
> > +
> > +#include "cgroup_helpers.h"
> > +#include "bpf_util.h"
> > +#include "../../../include/linux/filter.h"
> > +
> > +bool bpf_prog_type_supported(enum bpf_prog_type prog_type)
> > +{
> > +	struct bpf_load_program_attr attr;
> > +	struct bpf_insn insns[] = {
> > +		BPF_MOV64_IMM(BPF_REG_0, 0),
> > +		BPF_EXIT_INSN(),
> > +	};
> > +	int ret;
> > +
> > +	if (prog_type == BPF_PROG_TYPE_UNSPEC)
> > +		return true;
> > +
> > +	memset(&attr, 0, sizeof(attr));
> > +	attr.prog_type = prog_type;
> > +	attr.insns = insns;
> > +	attr.insns_cnt = ARRAY_SIZE(insns);
> > +	attr.license = "GPL";
> > +
> > +	ret = bpf_load_program_xattr(&attr, NULL, 0);
> > +	if (ret < 0)
> > +		return false;
> > +	close(ret);
> > +
> > +	return true;
> > +}
> > +
> > +bool bpf_map_type_supported(enum bpf_map_type map_type)
> > +{
> > +	int key_size, value_size, max_entries;
> > +	int fd;
> > +
> > +	key_size = sizeof(__u32);
> > +	value_size = sizeof(__u32);
> > +	max_entries = 1;
> > +
> > +	/* limited set of maps for test_verifier.c and test_maps.c */
> > +	switch (map_type) {
> > +	case BPF_MAP_TYPE_SOCKMAP:
> > +	case BPF_MAP_TYPE_SOCKHASH:
> > +	case BPF_MAP_TYPE_XSKMAP:
> > +		break;
> > +	case BPF_MAP_TYPE_STACK_TRACE:
> > +		value_size = sizeof(__u64);
> > +	case BPF_MAP_TYPE_CGROUP_STORAGE:
> > +	case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE:
> > +		key_size = sizeof(struct bpf_cgroup_storage_key);
> > +		value_size = sizeof(__u64);
> > +		max_entries = 0;
> > +		break;
> > +	default:
> > +		return true;
> 
> Given bpf_map_type_supported() is mainly called in the "unexpected" failure
> case, it's probably okay to have this less restricted here. Also, ideally we
> shouldn't need an explicit opt-in for every newly added map type to probe in
> future. But presumably both limitation will be removed when using libbpf API.
Yes, both of these problems should go away when converted to libbpf API.
I wanted to have something simple and tailed to a particular
usecase here.
> 
> > +	}
> > +
> > +	fd = bpf_create_map(map_type, key_size, value_size, max_entries, 0);
> > +	if (fd < 0)
> > +		return false;
> > +	close(fd);
> > +
> > +	return true;
> > +}
> > diff --git a/tools/testing/selftests/bpf/probe_helpers.h b/tools/testing/selftests/bpf/probe_helpers.h
> > new file mode 100644
> > index 000000000000..9a107d6fe936
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/probe_helpers.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +#ifndef __PROBE_HELPERS_H
> > +#define __PROBE_HELPERS_H
> > +
> > +#include <linux/bpf.h>
> > +
> > +bool bpf_prog_type_supported(enum bpf_prog_type prog_type);
> > +bool bpf_map_type_supported(enum bpf_map_type map_type);
> > +
> > +#endif
> > 
> 

[PATCH bpf-next v2 2/6] selftests/bpf: skip sockmap in test_maps if kernel doesn't have support

[Stanislav Fomichev]

Use recently introduced bpf_map_type_supported() to skip test_sockmap()
if map creation fails. The skipped test is indicated in the output.

Example:

test_sockmap SKIP (unsupported map type BPF_MAP_TYPE_SOCKMAP)
Fork 1024 tasks to 'test_update_delete'
...
test_sockmap SKIP (unsupported map type BPF_MAP_TYPE_SOCKMAP)
Fork 1024 tasks to 'test_update_delete'
...
test_maps: OK, 2 SKIPPED

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/Makefile    |  1 +
 tools/testing/selftests/bpf/test_maps.c | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 73aa6d8f4a2f..52a0654145ad 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -81,6 +81,7 @@ $(OUTPUT)/test_progs: trace_helpers.c
 $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
 $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
 $(OUTPUT)/test_netcnt: cgroup_helpers.c
+$(OUTPUT)/test_maps: probe_helpers.c
 
 .PHONY: force
 
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 9c79ee017df3..bdf9bdaea73e 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -27,11 +27,14 @@
 
 #include "bpf_util.h"
 #include "bpf_rlimit.h"
+#include "probe_helpers.h"
 
 #ifndef ENOTSUPP
 #define ENOTSUPP 524
 #endif
 
+static int skips;
+
 static int map_flags;
 
 #define CHECK(condition, tag, format...) ({				\
@@ -725,6 +728,15 @@ static void test_sockmap(int tasks, void *data)
 			    sizeof(key), sizeof(value),
 			    6, 0);
 	if (fd < 0) {
+		if (!bpf_map_type_supported(BPF_MAP_TYPE_SOCKMAP)) {
+			printf("%s SKIP (unsupported map type BPF_MAP_TYPE_SOCKMAP)\n",
+			       __func__);
+			skips++;
+			for (i = 0; i < 6; i++)
+				close(sfd[i]);
+			return;
+		}
+
 		printf("Failed to create sockmap %i\n", fd);
 		goto out_sockmap;
 	}
@@ -1702,6 +1714,6 @@ int main(void)
 	map_flags = BPF_F_NO_PREALLOC;
 	run_all_tests();
 
-	printf("test_maps: OK\n");
+	printf("test_maps: OK, %d SKIPPED\n", skips);
 	return 0;
 }
-- 
2.20.0.405.gbc1bbc6f85-goog

[PATCH bpf-next v2 3/6] selftests/bpf: skip verifier tests for unsupported program types

[Stanislav Fomichev]

Use recently introduced bpf_prog_type_supported() to skip tests in the
test_verifier() if bpf_verify_program() fails. The skipped test is
indicated in the output.

Example:

...
679/p bpf_get_stack return R0 within range SKIP (unsupported program
type 5)
680/p ld_abs: invalid op 1 OK
...
Summary: 863 PASSED, 165 SKIPPED, 3 FAILED

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/Makefile        | 1 +
 tools/testing/selftests/bpf/test_verifier.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 52a0654145ad..9e9c5ada3e9b 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -82,6 +82,7 @@ $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
 $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
 $(OUTPUT)/test_netcnt: cgroup_helpers.c
 $(OUTPUT)/test_maps: probe_helpers.c
+$(OUTPUT)/test_verifier: probe_helpers.c
 
 .PHONY: force
 
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index a08c67c8767e..124d21306c27 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -45,6 +45,7 @@
 #include "bpf_rand.h"
 #include "bpf_util.h"
 #include "../../../include/linux/filter.h"
+#include "probe_helpers.h"
 
 #define MAX_INSNS	BPF_MAXINSNS
 #define MAX_FIXUPS	8
@@ -57,6 +58,7 @@
 
 #define UNPRIV_SYSCTL "kernel/unprivileged_bpf_disabled"
 static bool unpriv_disabled = false;
+static int skips;
 
 struct bpf_test {
 	const char *descr;
@@ -14409,6 +14411,11 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 		pflags |= BPF_F_ANY_ALIGNMENT;
 	fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
 				     "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 1);
+	if (fd_prog < 0 && !bpf_prog_type_supported(prog_type)) {
+		printf("SKIP (unsupported program type %d)\n", prog_type);
+		skips++;
+		goto close_fds;
+	}
 
 	expected_ret = unpriv && test->result_unpriv != UNDEF ?
 		       test->result_unpriv : test->result;
@@ -14532,7 +14539,7 @@ static bool test_as_unpriv(struct bpf_test *test)
 
 static int do_test(bool unpriv, unsigned int from, unsigned int to)
 {
-	int i, passes = 0, errors = 0, skips = 0;
+	int i, passes = 0, errors = 0;
 
 	for (i = from; i < to; i++) {
 		struct bpf_test *test = &tests[i];
-- 
2.20.0.405.gbc1bbc6f85-goog

[PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types

[Stanislav Fomichev]

Use recently introduced bpf_map_type_supported() to skip tests in the
test_verifier if map creation (create_map) fails. It's handled
explicitly for each fixup, i.e. if bpf_create_map returns negative fd,
we probe the kernel for the appropriate map support and skip the
test is map type is not supported.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++--
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 124d21306c27..d267f5248b5d 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu)
 	return fd;
 }
 
+static bool skip_unsupported_map(int ret, enum bpf_map_type map_type)
+{
+	if (ret < 0 && !bpf_map_type_supported(map_type)) {
+		printf("SKIP (unsupported map type %d)\n", map_type);
+		skips++;
+		return true;
+	}
+	return false;
+}
+
 static char bpf_vlog[UINT_MAX >> 8];
 
-static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
-			  struct bpf_insn *prog, int *map_fds)
+static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
+			 struct bpf_insn *prog, int *map_fds)
 {
 	int *fixup_map_hash_8b = test->fixup_map_hash_8b;
 	int *fixup_map_hash_48b = test->fixup_map_hash_48b;
@@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
 
 	if (*fixup_cgroup_storage) {
 		map_fds[7] = create_cgroup_storage(false);
+		if (skip_unsupported_map(map_fds[7],
+					 BPF_MAP_TYPE_CGROUP_STORAGE))
+			return -1;
 		do {
 			prog[*fixup_cgroup_storage].imm = map_fds[7];
 			fixup_cgroup_storage++;
@@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
 
 	if (*fixup_percpu_cgroup_storage) {
 		map_fds[8] = create_cgroup_storage(true);
+		if (skip_unsupported_map(map_fds[8],
+					 BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE))
+			return -1;
 		do {
 			prog[*fixup_percpu_cgroup_storage].imm = map_fds[8];
 			fixup_percpu_cgroup_storage++;
@@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
 	if (*fixup_map_sockmap) {
 		map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
 					sizeof(int), 1);
+		if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP))
+			return -1;
 		do {
 			prog[*fixup_map_sockmap].imm = map_fds[9];
 			fixup_map_sockmap++;
@@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
 	if (*fixup_map_sockhash) {
 		map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
 					sizeof(int), 1);
+		if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH))
+			return -1;
 		do {
 			prog[*fixup_map_sockhash].imm = map_fds[10];
 			fixup_map_sockhash++;
@@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
 	if (*fixup_map_xskmap) {
 		map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
 					sizeof(int), 1);
+		if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP))
+			return -1;
 		do {
 			prog[*fixup_map_xskmap].imm = map_fds[11];
 			fixup_map_xskmap++;
@@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
 	if (*fixup_map_stacktrace) {
 		map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
 					 sizeof(u64), 1);
+		if (skip_unsupported_map(map_fds[12],
+					 BPF_MAP_TYPE_STACK_TRACE))
+			return -1;
 		do {
 			prog[*fixup_map_stacktrace].imm = map_fds[12];
 			fixup_map_stacktrace++;
 		} while (fixup_map_stacktrace);
 	}
+
+	return 0;
 }
 
 static int set_admin(bool admin)
@@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 
 	if (!prog_type)
 		prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
-	do_test_fixup(test, prog_type, prog, map_fds);
+	if (do_test_fixup(test, prog_type, prog, map_fds) < 0)
+		return;
 	prog_len = probe_filter_length(prog);
 
 	pflags = 0;
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types

[Daniel Borkmann]

On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
> Use recently introduced bpf_map_type_supported() to skip tests in the
> test_verifier if map creation (create_map) fails. It's handled
> explicitly for each fixup, i.e. if bpf_create_map returns negative fd,
> we probe the kernel for the appropriate map support and skip the
> test is map type is not supported.
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++--
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 124d21306c27..d267f5248b5d 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu)
>  	return fd;
>  }
>  
> +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type)
> +{
> +	if (ret < 0 && !bpf_map_type_supported(map_type)) {
> +		printf("SKIP (unsupported map type %d)\n", map_type);
> +		skips++;
> +		return true;
> +	}
> +	return false;
> +}
> +
>  static char bpf_vlog[UINT_MAX >> 8];
>  
> -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> -			  struct bpf_insn *prog, int *map_fds)
> +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> +			 struct bpf_insn *prog, int *map_fds)
>  {
>  	int *fixup_map_hash_8b = test->fixup_map_hash_8b;
>  	int *fixup_map_hash_48b = test->fixup_map_hash_48b;
> @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>  
>  	if (*fixup_cgroup_storage) {
>  		map_fds[7] = create_cgroup_storage(false);
> +		if (skip_unsupported_map(map_fds[7],
> +					 BPF_MAP_TYPE_CGROUP_STORAGE))
> +			return -1;
>  		do {
>  			prog[*fixup_cgroup_storage].imm = map_fds[7];
>  			fixup_cgroup_storage++;
> @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>  
>  	if (*fixup_percpu_cgroup_storage) {
>  		map_fds[8] = create_cgroup_storage(true);
> +		if (skip_unsupported_map(map_fds[8],
> +					 BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE))
> +			return -1;
>  		do {
>  			prog[*fixup_percpu_cgroup_storage].imm = map_fds[8];
>  			fixup_percpu_cgroup_storage++;
> @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>  	if (*fixup_map_sockmap) {
>  		map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
>  					sizeof(int), 1);
> +		if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP))
> +			return -1;
>  		do {
>  			prog[*fixup_map_sockmap].imm = map_fds[9];
>  			fixup_map_sockmap++;
> @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>  	if (*fixup_map_sockhash) {
>  		map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
>  					sizeof(int), 1);
> +		if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH))
> +			return -1;
>  		do {
>  			prog[*fixup_map_sockhash].imm = map_fds[10];
>  			fixup_map_sockhash++;
> @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>  	if (*fixup_map_xskmap) {
>  		map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
>  					sizeof(int), 1);
> +		if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP))
> +			return -1;
>  		do {
>  			prog[*fixup_map_xskmap].imm = map_fds[11];
>  			fixup_map_xskmap++;
> @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>  	if (*fixup_map_stacktrace) {
>  		map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
>  					 sizeof(u64), 1);
> +		if (skip_unsupported_map(map_fds[12],
> +					 BPF_MAP_TYPE_STACK_TRACE))
> +			return -1;

Nit: Could probably be slightly simplified by moving this into and by reworking
create_{map,cgroup_storage}() a bit.

>  		do {
>  			prog[*fixup_map_stacktrace].imm = map_fds[12];
>  			fixup_map_stacktrace++;
>  		} while (fixup_map_stacktrace);
>  	}
> +
> +	return 0;
>  }
>  
>  static int set_admin(bool admin)
> @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>  
>  	if (!prog_type)
>  		prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> -	do_test_fixup(test, prog_type, prog, map_fds);
> +	if (do_test_fixup(test, prog_type, prog, map_fds) < 0)
> +		return;
>  	prog_len = probe_filter_length(prog);
>  
>  	pflags = 0;
> 

Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types

[Stanislav Fomichev]

On 12/19, Daniel Borkmann wrote:
> On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
> > Use recently introduced bpf_map_type_supported() to skip tests in the
> > test_verifier if map creation (create_map) fails. It's handled
> > explicitly for each fixup, i.e. if bpf_create_map returns negative fd,
> > we probe the kernel for the appropriate map support and skip the
> > test is map type is not supported.
> > 
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++--
> >  1 file changed, 31 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 124d21306c27..d267f5248b5d 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu)
> >  	return fd;
> >  }
> >  
> > +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type)
> > +{
> > +	if (ret < 0 && !bpf_map_type_supported(map_type)) {
> > +		printf("SKIP (unsupported map type %d)\n", map_type);
> > +		skips++;
> > +		return true;
> > +	}
> > +	return false;
> > +}
> > +
> >  static char bpf_vlog[UINT_MAX >> 8];
> >  
> > -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > -			  struct bpf_insn *prog, int *map_fds)
> > +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > +			 struct bpf_insn *prog, int *map_fds)
> >  {
> >  	int *fixup_map_hash_8b = test->fixup_map_hash_8b;
> >  	int *fixup_map_hash_48b = test->fixup_map_hash_48b;
> > @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >  
> >  	if (*fixup_cgroup_storage) {
> >  		map_fds[7] = create_cgroup_storage(false);
> > +		if (skip_unsupported_map(map_fds[7],
> > +					 BPF_MAP_TYPE_CGROUP_STORAGE))
> > +			return -1;
> >  		do {
> >  			prog[*fixup_cgroup_storage].imm = map_fds[7];
> >  			fixup_cgroup_storage++;
> > @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >  
> >  	if (*fixup_percpu_cgroup_storage) {
> >  		map_fds[8] = create_cgroup_storage(true);
> > +		if (skip_unsupported_map(map_fds[8],
> > +					 BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE))
> > +			return -1;
> >  		do {
> >  			prog[*fixup_percpu_cgroup_storage].imm = map_fds[8];
> >  			fixup_percpu_cgroup_storage++;
> > @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >  	if (*fixup_map_sockmap) {
> >  		map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
> >  					sizeof(int), 1);
> > +		if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP))
> > +			return -1;
> >  		do {
> >  			prog[*fixup_map_sockmap].imm = map_fds[9];
> >  			fixup_map_sockmap++;
> > @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >  	if (*fixup_map_sockhash) {
> >  		map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
> >  					sizeof(int), 1);
> > +		if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH))
> > +			return -1;
> >  		do {
> >  			prog[*fixup_map_sockhash].imm = map_fds[10];
> >  			fixup_map_sockhash++;
> > @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >  	if (*fixup_map_xskmap) {
> >  		map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
> >  					sizeof(int), 1);
> > +		if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP))
> > +			return -1;
> >  		do {
> >  			prog[*fixup_map_xskmap].imm = map_fds[11];
> >  			fixup_map_xskmap++;
> > @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >  	if (*fixup_map_stacktrace) {
> >  		map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
> >  					 sizeof(u64), 1);
> > +		if (skip_unsupported_map(map_fds[12],
> > +					 BPF_MAP_TYPE_STACK_TRACE))
> > +			return -1;
> 
> Nit: Could probably be slightly simplified by moving this into and by reworking
> create_{map,cgroup_storage}() a bit.
Yeah, I stated that option in the cover letter. I did that initially,
but it required some additional argument (skip/supported) to the
create_{map,cgroup_storage} and I scrapped this approach due to too
much plumbing.

But I think since we are not doing any parallel tests in the verifier,
we can do something like the following patch below. WDYT?

---

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 124d21306c27..43f71783f7b5 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -14113,6 +14113,16 @@ static int probe_filter_length(const struct bpf_insn *fp)
 	return len + 1;
 }
 
+static bool skip_unsupported_map(enum bpf_map_type map_type)
+{
+	if (!bpf_map_type_supported(map_type)) {
+		printf("SKIP (unsupported map type %d)\n", map_type);
+		skips++;
+		return true;
+	}
+	return false;
+}
+
 static int create_map(uint32_t type, uint32_t size_key,
 		      uint32_t size_value, uint32_t max_elem)
 {
@@ -14120,8 +14130,11 @@ static int create_map(uint32_t type, uint32_t size_key,
 
 	fd = bpf_create_map(type, size_key, size_value, max_elem,
 			    type == BPF_MAP_TYPE_HASH ? BPF_F_NO_PREALLOC : 0);
-	if (fd < 0)
+	if (fd < 0) {
+		if (skip_unsupported_map(type))
+			return -1;
 		printf("Failed to create hash map '%s'!\n", strerror(errno));
+	}
 
 	return fd;
 }
@@ -14161,6 +14174,8 @@ static int create_prog_array(enum bpf_map_type prog_type, uint32_t max_elem,
 	mfd = bpf_create_map(BPF_MAP_TYPE_PROG_ARRAY, sizeof(int),
 			     sizeof(int), max_elem, 0);
 	if (mfd < 0) {
+		if (skip_unsupported_map(BPF_MAP_TYPE_PROG_ARRAY))
+			return -1;
 		printf("Failed to create prog array '%s'!\n", strerror(errno));
 		return -1;
 	}
@@ -14191,15 +14206,20 @@ static int create_map_in_map(void)
 	inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(int),
 				      sizeof(int), 1, 0);
 	if (inner_map_fd < 0) {
+		if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY))
+			return -1;
 		printf("Failed to create array '%s'!\n", strerror(errno));
 		return inner_map_fd;
 	}
 
 	outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL,
 					     sizeof(int), inner_map_fd, 1, 0);
-	if (outer_map_fd < 0)
+	if (outer_map_fd < 0) {
+		if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY_OF_MAPS))
+			return -1;
 		printf("Failed to create array of maps '%s'!\n",
 		       strerror(errno));
+	}
 
 	close(inner_map_fd);
 
@@ -14214,9 +14234,12 @@ static int create_cgroup_storage(bool percpu)
 
 	fd = bpf_create_map(type, sizeof(struct bpf_cgroup_storage_key),
 			    TEST_DATA_LEN, 0, 0);
-	if (fd < 0)
+	if (fd < 0) {
+		if (skip_unsupported_map(type))
+			return -1;
 		printf("Failed to create cgroup storage '%s'!\n",
 		       strerror(errno));
+	}
 
 	return fd;
 }
@@ -14392,6 +14415,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 	int map_fds[MAX_NR_MAPS];
 	const char *expected_err;
 	uint32_t expected_val;
+	int fixup_skips;
 	uint32_t retval;
 	__u32 pflags;
 	int i, err;
@@ -14401,7 +14425,13 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 
 	if (!prog_type)
 		prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+	fixup_skips = skips;
 	do_test_fixup(test, prog_type, prog, map_fds);
+	/* If there were some map skips during fixup due to missing bpf
+	 * features, skip this test.
+	 */
+	if (fixup_skips != skips)
+		return;
 	prog_len = probe_filter_length(prog);
 
 	pflags = 0;

> 
> >  		do {
> >  			prog[*fixup_map_stacktrace].imm = map_fds[12];
> >  			fixup_map_stacktrace++;
> >  		} while (fixup_map_stacktrace);
> >  	}
> > +
> > +	return 0;
> >  }
> >  
> >  static int set_admin(bool admin)
> > @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >  
> >  	if (!prog_type)
> >  		prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> > -	do_test_fixup(test, prog_type, prog, map_fds);
> > +	if (do_test_fixup(test, prog_type, prog, map_fds) < 0)
> > +		return;
> >  	prog_len = probe_filter_length(prog);
> >  
> >  	pflags = 0;
> > 
> 

Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types

[Stanislav Fomichev]

On Tue, Dec 18, 2018 at 4:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 12/19, Daniel Borkmann wrote:
> > On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
> > > Use recently introduced bpf_map_type_supported() to skip tests in the
> > > test_verifier if map creation (create_map) fails. It's handled
> > > explicitly for each fixup, i.e. if bpf_create_map returns negative fd,
> > > we probe the kernel for the appropriate map support and skip the
> > > test is map type is not supported.
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > >  tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++--
> > >  1 file changed, 31 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > index 124d21306c27..d267f5248b5d 100644
> > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu)
> > >     return fd;
> > >  }
> > >
> > > +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type)
> > > +{
> > > +   if (ret < 0 && !bpf_map_type_supported(map_type)) {
> > > +           printf("SKIP (unsupported map type %d)\n", map_type);
> > > +           skips++;
> > > +           return true;
> > > +   }
> > > +   return false;
> > > +}
> > > +
> > >  static char bpf_vlog[UINT_MAX >> 8];
> > >
> > > -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > -                     struct bpf_insn *prog, int *map_fds)
> > > +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > +                    struct bpf_insn *prog, int *map_fds)
> > >  {
> > >     int *fixup_map_hash_8b = test->fixup_map_hash_8b;
> > >     int *fixup_map_hash_48b = test->fixup_map_hash_48b;
> > > @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >
> > >     if (*fixup_cgroup_storage) {
> > >             map_fds[7] = create_cgroup_storage(false);
> > > +           if (skip_unsupported_map(map_fds[7],
> > > +                                    BPF_MAP_TYPE_CGROUP_STORAGE))
> > > +                   return -1;
> > >             do {
> > >                     prog[*fixup_cgroup_storage].imm = map_fds[7];
> > >                     fixup_cgroup_storage++;
> > > @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >
> > >     if (*fixup_percpu_cgroup_storage) {
> > >             map_fds[8] = create_cgroup_storage(true);
> > > +           if (skip_unsupported_map(map_fds[8],
> > > +                                    BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE))
> > > +                   return -1;
> > >             do {
> > >                     prog[*fixup_percpu_cgroup_storage].imm = map_fds[8];
> > >                     fixup_percpu_cgroup_storage++;
> > > @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >     if (*fixup_map_sockmap) {
> > >             map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
> > >                                     sizeof(int), 1);
> > > +           if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP))
> > > +                   return -1;
> > >             do {
> > >                     prog[*fixup_map_sockmap].imm = map_fds[9];
> > >                     fixup_map_sockmap++;
> > > @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >     if (*fixup_map_sockhash) {
> > >             map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
> > >                                     sizeof(int), 1);
> > > +           if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH))
> > > +                   return -1;
> > >             do {
> > >                     prog[*fixup_map_sockhash].imm = map_fds[10];
> > >                     fixup_map_sockhash++;
> > > @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >     if (*fixup_map_xskmap) {
> > >             map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
> > >                                     sizeof(int), 1);
> > > +           if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP))
> > > +                   return -1;
> > >             do {
> > >                     prog[*fixup_map_xskmap].imm = map_fds[11];
> > >                     fixup_map_xskmap++;
> > > @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >     if (*fixup_map_stacktrace) {
> > >             map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
> > >                                      sizeof(u64), 1);
> > > +           if (skip_unsupported_map(map_fds[12],
> > > +                                    BPF_MAP_TYPE_STACK_TRACE))
> > > +                   return -1;
> >
> > Nit: Could probably be slightly simplified by moving this into and by reworking
> > create_{map,cgroup_storage}() a bit.
> Yeah, I stated that option in the cover letter. I did that initially,
> but it required some additional argument (skip/supported) to the
> create_{map,cgroup_storage} and I scrapped this approach due to too
> much plumbing.
>
> But I think since we are not doing any parallel tests in the verifier,
> we can do something like the following patch below. WDYT?
Daniel, should this go as is or you'd like me to respin to the version
from my last reply (or something similar)?

>
> ---
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 124d21306c27..43f71783f7b5 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -14113,6 +14113,16 @@ static int probe_filter_length(const struct bpf_insn *fp)
>         return len + 1;
>  }
>
> +static bool skip_unsupported_map(enum bpf_map_type map_type)
> +{
> +       if (!bpf_map_type_supported(map_type)) {
> +               printf("SKIP (unsupported map type %d)\n", map_type);
> +               skips++;
> +               return true;
> +       }
> +       return false;
> +}
> +
>  static int create_map(uint32_t type, uint32_t size_key,
>                       uint32_t size_value, uint32_t max_elem)
>  {
> @@ -14120,8 +14130,11 @@ static int create_map(uint32_t type, uint32_t size_key,
>
>         fd = bpf_create_map(type, size_key, size_value, max_elem,
>                             type == BPF_MAP_TYPE_HASH ? BPF_F_NO_PREALLOC : 0);
> -       if (fd < 0)
> +       if (fd < 0) {
> +               if (skip_unsupported_map(type))
> +                       return -1;
>                 printf("Failed to create hash map '%s'!\n", strerror(errno));
> +       }
>
>         return fd;
>  }
> @@ -14161,6 +14174,8 @@ static int create_prog_array(enum bpf_map_type prog_type, uint32_t max_elem,
>         mfd = bpf_create_map(BPF_MAP_TYPE_PROG_ARRAY, sizeof(int),
>                              sizeof(int), max_elem, 0);
>         if (mfd < 0) {
> +               if (skip_unsupported_map(BPF_MAP_TYPE_PROG_ARRAY))
> +                       return -1;
>                 printf("Failed to create prog array '%s'!\n", strerror(errno));
>                 return -1;
>         }
> @@ -14191,15 +14206,20 @@ static int create_map_in_map(void)
>         inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(int),
>                                       sizeof(int), 1, 0);
>         if (inner_map_fd < 0) {
> +               if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY))
> +                       return -1;
>                 printf("Failed to create array '%s'!\n", strerror(errno));
>                 return inner_map_fd;
>         }
>
>         outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL,
>                                              sizeof(int), inner_map_fd, 1, 0);
> -       if (outer_map_fd < 0)
> +       if (outer_map_fd < 0) {
> +               if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY_OF_MAPS))
> +                       return -1;
>                 printf("Failed to create array of maps '%s'!\n",
>                        strerror(errno));
> +       }
>
>         close(inner_map_fd);
>
> @@ -14214,9 +14234,12 @@ static int create_cgroup_storage(bool percpu)
>
>         fd = bpf_create_map(type, sizeof(struct bpf_cgroup_storage_key),
>                             TEST_DATA_LEN, 0, 0);
> -       if (fd < 0)
> +       if (fd < 0) {
> +               if (skip_unsupported_map(type))
> +                       return -1;
>                 printf("Failed to create cgroup storage '%s'!\n",
>                        strerror(errno));
> +       }
>
>         return fd;
>  }
> @@ -14392,6 +14415,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>         int map_fds[MAX_NR_MAPS];
>         const char *expected_err;
>         uint32_t expected_val;
> +       int fixup_skips;
>         uint32_t retval;
>         __u32 pflags;
>         int i, err;
> @@ -14401,7 +14425,13 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>
>         if (!prog_type)
>                 prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> +       fixup_skips = skips;
>         do_test_fixup(test, prog_type, prog, map_fds);
> +       /* If there were some map skips during fixup due to missing bpf
> +        * features, skip this test.
> +        */
> +       if (fixup_skips != skips)
> +               return;
>         prog_len = probe_filter_length(prog);
>
>         pflags = 0;
>
> >
> > >             do {
> > >                     prog[*fixup_map_stacktrace].imm = map_fds[12];
> > >                     fixup_map_stacktrace++;
> > >             } while (fixup_map_stacktrace);
> > >     }
> > > +
> > > +   return 0;
> > >  }
> > >
> > >  static int set_admin(bool admin)
> > > @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> > >
> > >     if (!prog_type)
> > >             prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> > > -   do_test_fixup(test, prog_type, prog, map_fds);
> > > +   if (do_test_fixup(test, prog_type, prog, map_fds) < 0)
> > > +           return;
> > >     prog_len = probe_filter_length(prog);
> > >
> > >     pflags = 0;
> > >
> >

Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types

[Daniel Borkmann]

On 12/20/2018 09:51 PM, Stanislav Fomichev wrote:
> On Tue, Dec 18, 2018 at 4:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>> On 12/19, Daniel Borkmann wrote:
>>> On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
>>>> Use recently introduced bpf_map_type_supported() to skip tests in the
>>>> test_verifier if map creation (create_map) fails. It's handled
>>>> explicitly for each fixup, i.e. if bpf_create_map returns negative fd,
>>>> we probe the kernel for the appropriate map support and skip the
>>>> test is map type is not supported.
>>>>
>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>>> ---
>>>>  tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++--
>>>>  1 file changed, 31 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
>>>> index 124d21306c27..d267f5248b5d 100644
>>>> --- a/tools/testing/selftests/bpf/test_verifier.c
>>>> +++ b/tools/testing/selftests/bpf/test_verifier.c
>>>> @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu)
>>>>     return fd;
>>>>  }
>>>>
>>>> +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type)
>>>> +{
>>>> +   if (ret < 0 && !bpf_map_type_supported(map_type)) {
>>>> +           printf("SKIP (unsupported map type %d)\n", map_type);
>>>> +           skips++;
>>>> +           return true;
>>>> +   }
>>>> +   return false;
>>>> +}
>>>> +
>>>>  static char bpf_vlog[UINT_MAX >> 8];
>>>>
>>>> -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>> -                     struct bpf_insn *prog, int *map_fds)
>>>> +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>> +                    struct bpf_insn *prog, int *map_fds)
>>>>  {
>>>>     int *fixup_map_hash_8b = test->fixup_map_hash_8b;
>>>>     int *fixup_map_hash_48b = test->fixup_map_hash_48b;
>>>> @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>>
>>>>     if (*fixup_cgroup_storage) {
>>>>             map_fds[7] = create_cgroup_storage(false);
>>>> +           if (skip_unsupported_map(map_fds[7],
>>>> +                                    BPF_MAP_TYPE_CGROUP_STORAGE))
>>>> +                   return -1;
>>>>             do {
>>>>                     prog[*fixup_cgroup_storage].imm = map_fds[7];
>>>>                     fixup_cgroup_storage++;
>>>> @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>>
>>>>     if (*fixup_percpu_cgroup_storage) {
>>>>             map_fds[8] = create_cgroup_storage(true);
>>>> +           if (skip_unsupported_map(map_fds[8],
>>>> +                                    BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE))
>>>> +                   return -1;
>>>>             do {
>>>>                     prog[*fixup_percpu_cgroup_storage].imm = map_fds[8];
>>>>                     fixup_percpu_cgroup_storage++;
>>>> @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>>     if (*fixup_map_sockmap) {
>>>>             map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
>>>>                                     sizeof(int), 1);
>>>> +           if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP))
>>>> +                   return -1;
>>>>             do {
>>>>                     prog[*fixup_map_sockmap].imm = map_fds[9];
>>>>                     fixup_map_sockmap++;
>>>> @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>>     if (*fixup_map_sockhash) {
>>>>             map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
>>>>                                     sizeof(int), 1);
>>>> +           if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH))
>>>> +                   return -1;
>>>>             do {
>>>>                     prog[*fixup_map_sockhash].imm = map_fds[10];
>>>>                     fixup_map_sockhash++;
>>>> @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>>     if (*fixup_map_xskmap) {
>>>>             map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
>>>>                                     sizeof(int), 1);
>>>> +           if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP))
>>>> +                   return -1;
>>>>             do {
>>>>                     prog[*fixup_map_xskmap].imm = map_fds[11];
>>>>                     fixup_map_xskmap++;
>>>> @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
>>>>     if (*fixup_map_stacktrace) {
>>>>             map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
>>>>                                      sizeof(u64), 1);
>>>> +           if (skip_unsupported_map(map_fds[12],
>>>> +                                    BPF_MAP_TYPE_STACK_TRACE))
>>>> +                   return -1;
>>>
>>> Nit: Could probably be slightly simplified by moving this into and by reworking
>>> create_{map,cgroup_storage}() a bit.
>> Yeah, I stated that option in the cover letter. I did that initially,
>> but it required some additional argument (skip/supported) to the
>> create_{map,cgroup_storage} and I scrapped this approach due to too
>> much plumbing.
>>
>> But I think since we are not doing any parallel tests in the verifier,
>> we can do something like the following patch below. WDYT?
> Daniel, should this go as is or you'd like me to respin to the version
> from my last reply (or something similar)?

Your diff on top of the set looks good to me. My preference though is that
we get both your work and Quentin's merged in the next cycle (given we're
about to close bpf-next) and have plenty of time to consolidate the two and
get it into good shape in order to then move the logic into libbpf as next
step such that bpftool /and/ kselftests can make use of it.

Thanks a lot,
Daniel

Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types

[Stanislav Fomichev]

On 12/20, Daniel Borkmann wrote:
> On 12/20/2018 09:51 PM, Stanislav Fomichev wrote:
> > On Tue, Dec 18, 2018 at 4:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >> On 12/19, Daniel Borkmann wrote:
> >>> On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
> >>>> Use recently introduced bpf_map_type_supported() to skip tests in the
> >>>> test_verifier if map creation (create_map) fails. It's handled
> >>>> explicitly for each fixup, i.e. if bpf_create_map returns negative fd,
> >>>> we probe the kernel for the appropriate map support and skip the
> >>>> test is map type is not supported.
> >>>>
> >>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >>>> ---
> >>>>  tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++--
> >>>>  1 file changed, 31 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> >>>> index 124d21306c27..d267f5248b5d 100644
> >>>> --- a/tools/testing/selftests/bpf/test_verifier.c
> >>>> +++ b/tools/testing/selftests/bpf/test_verifier.c
> >>>> @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu)
> >>>>     return fd;
> >>>>  }
> >>>>
> >>>> +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type)
> >>>> +{
> >>>> +   if (ret < 0 && !bpf_map_type_supported(map_type)) {
> >>>> +           printf("SKIP (unsupported map type %d)\n", map_type);
> >>>> +           skips++;
> >>>> +           return true;
> >>>> +   }
> >>>> +   return false;
> >>>> +}
> >>>> +
> >>>>  static char bpf_vlog[UINT_MAX >> 8];
> >>>>
> >>>> -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>> -                     struct bpf_insn *prog, int *map_fds)
> >>>> +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>> +                    struct bpf_insn *prog, int *map_fds)
> >>>>  {
> >>>>     int *fixup_map_hash_8b = test->fixup_map_hash_8b;
> >>>>     int *fixup_map_hash_48b = test->fixup_map_hash_48b;
> >>>> @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>>
> >>>>     if (*fixup_cgroup_storage) {
> >>>>             map_fds[7] = create_cgroup_storage(false);
> >>>> +           if (skip_unsupported_map(map_fds[7],
> >>>> +                                    BPF_MAP_TYPE_CGROUP_STORAGE))
> >>>> +                   return -1;
> >>>>             do {
> >>>>                     prog[*fixup_cgroup_storage].imm = map_fds[7];
> >>>>                     fixup_cgroup_storage++;
> >>>> @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>>
> >>>>     if (*fixup_percpu_cgroup_storage) {
> >>>>             map_fds[8] = create_cgroup_storage(true);
> >>>> +           if (skip_unsupported_map(map_fds[8],
> >>>> +                                    BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE))
> >>>> +                   return -1;
> >>>>             do {
> >>>>                     prog[*fixup_percpu_cgroup_storage].imm = map_fds[8];
> >>>>                     fixup_percpu_cgroup_storage++;
> >>>> @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>>     if (*fixup_map_sockmap) {
> >>>>             map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
> >>>>                                     sizeof(int), 1);
> >>>> +           if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP))
> >>>> +                   return -1;
> >>>>             do {
> >>>>                     prog[*fixup_map_sockmap].imm = map_fds[9];
> >>>>                     fixup_map_sockmap++;
> >>>> @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>>     if (*fixup_map_sockhash) {
> >>>>             map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
> >>>>                                     sizeof(int), 1);
> >>>> +           if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH))
> >>>> +                   return -1;
> >>>>             do {
> >>>>                     prog[*fixup_map_sockhash].imm = map_fds[10];
> >>>>                     fixup_map_sockhash++;
> >>>> @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>>     if (*fixup_map_xskmap) {
> >>>>             map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
> >>>>                                     sizeof(int), 1);
> >>>> +           if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP))
> >>>> +                   return -1;
> >>>>             do {
> >>>>                     prog[*fixup_map_xskmap].imm = map_fds[11];
> >>>>                     fixup_map_xskmap++;
> >>>> @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> >>>>     if (*fixup_map_stacktrace) {
> >>>>             map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
> >>>>                                      sizeof(u64), 1);
> >>>> +           if (skip_unsupported_map(map_fds[12],
> >>>> +                                    BPF_MAP_TYPE_STACK_TRACE))
> >>>> +                   return -1;
> >>>
> >>> Nit: Could probably be slightly simplified by moving this into and by reworking
> >>> create_{map,cgroup_storage}() a bit.
> >> Yeah, I stated that option in the cover letter. I did that initially,
> >> but it required some additional argument (skip/supported) to the
> >> create_{map,cgroup_storage} and I scrapped this approach due to too
> >> much plumbing.
> >>
> >> But I think since we are not doing any parallel tests in the verifier,
> >> we can do something like the following patch below. WDYT?
> > Daniel, should this go as is or you'd like me to respin to the version
> > from my last reply (or something similar)?
> 
> Your diff on top of the set looks good to me. My preference though is that
> we get both your work and Quentin's merged in the next cycle (given we're
> about to close bpf-next) and have plenty of time to consolidate the two and
> get it into good shape in order to then move the logic into libbpf as next
> step such that bpftool /and/ kselftests can make use of it.
Ack, I'll use that for a v3 submission when bpf-next opens again.
Maybe I can even drop my custom probes if Quentin's patch set is ready
by that point.
> 
> Thanks a lot,
> Daniel

[PATCH bpf-next v2 5/6] selftests/bpf: mark verifier test that uses bpf_trace_printk as BPF_PROG_TYPE_TRACEPOINT

[Stanislav Fomichev]

We don't have this helper if the kernel was compiled without
CONFIG_BPF_EVENTS. Setting prog_type to BPF_PROG_TYPE_TRACEPOINT
let's verifier correctly skip this test based on the missing
prog_type support in the kernel.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/test_verifier.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index d267f5248b5d..590bf9a63ad9 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -2622,6 +2622,7 @@ static struct bpf_test tests[] = {
 		.errstr_unpriv = "unknown func bpf_trace_printk#6",
 		.result_unpriv = REJECT,
 		.result = ACCEPT,
+		.prog_type = BPF_PROG_TYPE_TRACEPOINT,
 	},
 	{
 		"unpriv: pass pointer to helper function",
-- 
2.20.0.405.gbc1bbc6f85-goog

[PATCH bpf-next v2 6/6] bpf: BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} require cgroups enabled

[Stanislav Fomichev]

There is no way to exercise appropriate attach points without cgroups
enabled. This lets test_verifier correctly skip tests for these
prog_types if kernel was compiled without BPF cgroup support.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 include/linux/bpf_types.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 44d9ab4809bd..08bf2f1fe553 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -6,9 +6,11 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_SOCKET_FILTER, sk_filter)
 BPF_PROG_TYPE(BPF_PROG_TYPE_SCHED_CLS, tc_cls_act)
 BPF_PROG_TYPE(BPF_PROG_TYPE_SCHED_ACT, tc_cls_act)
 BPF_PROG_TYPE(BPF_PROG_TYPE_XDP, xdp)
+#ifdef CONFIG_CGROUP_BPF
 BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SKB, cg_skb)
 BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SOCK, cg_sock)
 BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SOCK_ADDR, cg_sock_addr)
+#endif
 BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_IN, lwt_in)
 BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_OUT, lwt_out)
 BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_XMIT, lwt_xmit)
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH bpf-next v2 0/6] skip verifier/map tests if kernel support is missing

[Alexei Starovoitov]

On Mon, Dec 17, 2018 at 10:25:48AM -0800, Stanislav Fomichev wrote:
> If test_maps/test_verifier is running against the kernel which doesn't
> have _all_ BPF features enabled, it fails with an error. This patch
> series tries to probe kernel support for each failed test and skip
> it instead. This lets users run BPF selftests in the not-all-bpf-yes
> environments and received correct PASS/NON-PASS result.
> 
> See https://www.spinics.net/lists/netdev/msg539331.html for more
> context.
> 
> The series goes like this:
> 
> * patch #1 adds bpf_prog_type_supported() and
>   bpf_map_type_supported() which query the kernel (insert 'return 0'
>   program or try to create empty map with correct key/value sizes) and
>   return supported/unsupported.
>   Note: this functionality can later be reimplemented on top of Quentin's
>   recent 'bpftool feature' patchset if he decides to move the probes
>   into libbpf.
> * patch #2 skips sockmap tests in test_maps.c if BPF_MAP_TYPE_SOCKMAP
>   map is not supported (if bpf_create_map fails, we probe the kernel
>   for support)
> * patch #3 skips verifier tests if test->prog_type is not supported (if
>   bpf_verify_program fails, we probe the kernel for support)
> * patch #4 skips verifier tests if test fixup map is not supported (if
>   create_map fails, we probe the kernel for support)
>   Note: we can probably move this probe into create_map helper and
>   return some argument instead of adding skip_unsupported_map()
>   to each fixup; but I'm not sure it's better.
>   Also note: in current implementation we still print 'Failed to
>   create hash map' from the create_map, but still skip the test.
> * next patches fix various small issues that arise from the first four:
>   * patch #5 sets "unknown func bpf_trace_printk#6" prog_type to
>     BPF_PROG_TYPE_TRACEPOINT so it is correctly skipped in
>     CONFIG_BPF_EVENTS=n case
>   * patch #6 exposes BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} only when
>     CONFIG_CGROUP_BPF=y, this makes verifier correctly skip appropriate
>     tests
> 
> v2 changes:
> 
> * don't sprinkle "ifdef CONFIG_CGROUP_BPF" all around net/core/filter.c,
>   doing it only in the bpf_types.h is enough to disable
>   BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} prog types for non-cgroup
>   enabled kernels

the patches look good to me.
I think it's ok to proceed this way though long term we probably
want to have such bpf_prog_type_supported() to be part of libbpf
and reused in test_verifier.c and in bpftool.
Daniel, thoughts?

Re: [PATCH bpf-next v2 0/6] skip verifier/map tests if kernel support is missing

[Stanislav Fomichev]

On 12/18, Alexei Starovoitov wrote:
> On Mon, Dec 17, 2018 at 10:25:48AM -0800, Stanislav Fomichev wrote:
> > If test_maps/test_verifier is running against the kernel which doesn't
> > have _all_ BPF features enabled, it fails with an error. This patch
> > series tries to probe kernel support for each failed test and skip
> > it instead. This lets users run BPF selftests in the not-all-bpf-yes
> > environments and received correct PASS/NON-PASS result.
> > 
> > See https://www.spinics.net/lists/netdev/msg539331.html for more
> > context.
> > 
> > The series goes like this:
> > 
> > * patch #1 adds bpf_prog_type_supported() and
> >   bpf_map_type_supported() which query the kernel (insert 'return 0'
> >   program or try to create empty map with correct key/value sizes) and
> >   return supported/unsupported.
> >   Note: this functionality can later be reimplemented on top of Quentin's
> >   recent 'bpftool feature' patchset if he decides to move the probes
> >   into libbpf.
> > * patch #2 skips sockmap tests in test_maps.c if BPF_MAP_TYPE_SOCKMAP
> >   map is not supported (if bpf_create_map fails, we probe the kernel
> >   for support)
> > * patch #3 skips verifier tests if test->prog_type is not supported (if
> >   bpf_verify_program fails, we probe the kernel for support)
> > * patch #4 skips verifier tests if test fixup map is not supported (if
> >   create_map fails, we probe the kernel for support)
> >   Note: we can probably move this probe into create_map helper and
> >   return some argument instead of adding skip_unsupported_map()
> >   to each fixup; but I'm not sure it's better.
> >   Also note: in current implementation we still print 'Failed to
> >   create hash map' from the create_map, but still skip the test.
> > * next patches fix various small issues that arise from the first four:
> >   * patch #5 sets "unknown func bpf_trace_printk#6" prog_type to
> >     BPF_PROG_TYPE_TRACEPOINT so it is correctly skipped in
> >     CONFIG_BPF_EVENTS=n case
> >   * patch #6 exposes BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} only when
> >     CONFIG_CGROUP_BPF=y, this makes verifier correctly skip appropriate
> >     tests
> > 
> > v2 changes:
> > 
> > * don't sprinkle "ifdef CONFIG_CGROUP_BPF" all around net/core/filter.c,
> >   doing it only in the bpf_types.h is enough to disable
> >   BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} prog types for non-cgroup
> >   enabled kernels
> 
> the patches look good to me.
> I think it's ok to proceed this way though long term we probably
> want to have such bpf_prog_type_supported() to be part of libbpf
> and reused in test_verifier.c and in bpftool.
Quentin is working on adding more generic bpf_xyz_type_supported() to
libbpf. My plan is to switch to them as soon as they are merged.
> Daniel, thoughts?
> 

Re: [PATCH bpf-next v2 0/6] skip verifier/map tests if kernel support is missing

[Daniel Borkmann]

On 12/18/2018 10:30 PM, Stanislav Fomichev wrote:
> On 12/18, Alexei Starovoitov wrote:
>> On Mon, Dec 17, 2018 at 10:25:48AM -0800, Stanislav Fomichev wrote:
>>> If test_maps/test_verifier is running against the kernel which doesn't
>>> have _all_ BPF features enabled, it fails with an error. This patch
>>> series tries to probe kernel support for each failed test and skip
>>> it instead. This lets users run BPF selftests in the not-all-bpf-yes
>>> environments and received correct PASS/NON-PASS result.
>>>
>>> See https://www.spinics.net/lists/netdev/msg539331.html for more
>>> context.
>>>
>>> The series goes like this:
>>>
>>> * patch #1 adds bpf_prog_type_supported() and
>>>   bpf_map_type_supported() which query the kernel (insert 'return 0'
>>>   program or try to create empty map with correct key/value sizes) and
>>>   return supported/unsupported.
>>>   Note: this functionality can later be reimplemented on top of Quentin's
>>>   recent 'bpftool feature' patchset if he decides to move the probes
>>>   into libbpf.
>>> * patch #2 skips sockmap tests in test_maps.c if BPF_MAP_TYPE_SOCKMAP
>>>   map is not supported (if bpf_create_map fails, we probe the kernel
>>>   for support)
>>> * patch #3 skips verifier tests if test->prog_type is not supported (if
>>>   bpf_verify_program fails, we probe the kernel for support)
>>> * patch #4 skips verifier tests if test fixup map is not supported (if
>>>   create_map fails, we probe the kernel for support)
>>>   Note: we can probably move this probe into create_map helper and
>>>   return some argument instead of adding skip_unsupported_map()
>>>   to each fixup; but I'm not sure it's better.
>>>   Also note: in current implementation we still print 'Failed to
>>>   create hash map' from the create_map, but still skip the test.
>>> * next patches fix various small issues that arise from the first four:
>>>   * patch #5 sets "unknown func bpf_trace_printk#6" prog_type to
>>>     BPF_PROG_TYPE_TRACEPOINT so it is correctly skipped in
>>>     CONFIG_BPF_EVENTS=n case
>>>   * patch #6 exposes BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} only when
>>>     CONFIG_CGROUP_BPF=y, this makes verifier correctly skip appropriate
>>>     tests
>>>
>>> v2 changes:
>>>
>>> * don't sprinkle "ifdef CONFIG_CGROUP_BPF" all around net/core/filter.c,
>>>   doing it only in the bpf_types.h is enough to disable
>>>   BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} prog types for non-cgroup
>>>   enabled kernels
>>
>> the patches look good to me.
>> I think it's ok to proceed this way though long term we probably
>> want to have such bpf_prog_type_supported() to be part of libbpf
>> and reused in test_verifier.c and in bpftool.
> Quentin is working on adding more generic bpf_xyz_type_supported() to
> libbpf. My plan is to switch to them as soon as they are merged.

Yeah, libbpf probes in-tree user for BPF kselftest sounds good to me.

>> Daniel, thoughts?

I just have few minor nits; will reply in a sec to the two patches, but
it's nothing blocking the series here.

Thanks,
Daniel