On 12/14, Quentin Monnet wrote:

Hi Stanislav,

2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev [off-list ref]

quoted

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'.

Why would you fall back on “supported” if it does not know about them?
Would that be worth having an enum as a return type (..._SUPPORTED,
..._UNSUPPORTED, ..._UNKNOWN) maybe? Or default to not supported?

I thought that it's safer for verifier to FAIL in case we forgot to add
a specific map support to bpf_map_type_supported(). This is not the case
if we were to use your version where you try to support every map type.

Not related - We would need to put a warning somewhere, maybe a comment
in the header, that using probes repeatedly in a short amount of time
needs to update resources limits (setrlimit()), otherwise probes won't
work correctly.

If we were to move this to libbpf, yes. For tests, I think we include
bpr_rlimit.h everywhere and things just work :-)

quoted

Signed-off-by: Stanislav Fomichev <redacted>
---
 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)

Can we please make it possible to add an ifindex for testing offload
support?

Can we extend it later as we go? This is just a test helper with a
limited support.
If you want to start with putting this to libbpf, then yes, we need
to add ifindex and properly support all map types.

quoted

+{
+	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)

Could we take an ifindex here as well?

ditto, see above

quoted

+{
+	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:

Why not probing the other types of maps and blindly assume everything
else is supported?

Again, for a verifier, I'd rather fail for a case where we didn't
explicitly allow it to skip.

quoted

+		return true;
+	}

For the record if you were to probe all existing map types at this date
you have would have issues here for LPM_TRIE (key_size, value_size,
map_flags), QUEUE and STACK (key_size). Also, maps in maps.

Ack, again, this just for a limited set of maps where we do fixups in
verifier.

quoted

+
+	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);

Should these get a visibility attribute with "LIBBPF_API" in front of
the declarations?

If we were to move them to the libbpf, yes. So far, it's only a test
helper.

quoted

+
+#endif