linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/5] bpftool: Make probes which emit dmesg warnings optional
@ 2020-02-21  3:16 Michal Rostecki
  2020-02-21  3:16 ` [PATCH bpf-next v2 1/5] bpftool: Move out sections to separate functions Michal Rostecki
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Michal Rostecki @ 2020-02-21  3:16 UTC (permalink / raw)
  To: bpf
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Quentin Monnet, Jakub Kicinski, netdev, linux-kernel, Shuah Khan,
	open list:KERNEL SELFTEST FRAMEWORK

Feature probes in bpftool related to bpf_probe_write_user and
bpf_trace_printk helpers emit dmesg warnings which might be confusing
for people running bpftool on production environments. This patch series
addresses that by filtering them out by default and introducing the new
positional argument "full" which enables all available probes.

The main motivation behind those changes is ability the fact that some
probes (for example those related to "trace" or "write_user" helpers)
emit dmesg messages which might be confusing for people who are running
on production environments. For details see the Cilium issue[0].

v1: https://lore.kernel.org/bpf/20200218190224.22508-1-mrostecki@opensuse.org/T/

v1 -> v2:
- Do not expose regex filters to users, keep filtering logic internal,
expose only the "full" option for including probes which emit dmesg
warnings.

[0] https://github.com/cilium/cilium/issues/10048

Michal Rostecki (5):
  bpftool: Move out sections to separate functions
  bpftool: Make probes which emit dmesg warnings optional
  bpftool: Update documentation of "bpftool feature" command
  bpftool: Update bash completion for "bpftool feature" command
  selftests/bpf: Add test for "bpftool feature" command

 .../bpftool/Documentation/bpftool-feature.rst |  15 +-
 tools/bpf/bpftool/bash-completion/bpftool     |  27 +-
 tools/bpf/bpftool/feature.c                   | 291 ++++++++++++------
 tools/testing/selftests/.gitignore            |   5 +-
 tools/testing/selftests/bpf/Makefile          |   3 +-
 tools/testing/selftests/bpf/test_bpftool.py   | 228 ++++++++++++++
 tools/testing/selftests/bpf/test_bpftool.sh   |   5 +
 7 files changed, 463 insertions(+), 111 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/test_bpftool.py
 create mode 100755 tools/testing/selftests/bpf/test_bpftool.sh

-- 
2.25.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH bpf-next v2 1/5] bpftool: Move out sections to separate functions
  2020-02-21  3:16 [PATCH bpf-next v2 0/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
@ 2020-02-21  3:16 ` Michal Rostecki
  2020-02-21 11:27   ` Quentin Monnet
  2020-02-21  3:16 ` [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Michal Rostecki @ 2020-02-21  3:16 UTC (permalink / raw)
  To: bpf
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Quentin Monnet, Jakub Kicinski, netdev, linux-kernel, Shuah Khan,
	David S. Miller, Jesper Dangaard Brouer, John Fastabend,
	open list:KERNEL SELFTEST FRAMEWORK

Remove all calls of print_end_then_start_section function and for loops
out from the do_probe function. Instead, provide separate functions for
each section (like i.e. section_helpers) which are called in do_probe.
This change is motivated by better readability.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
---
 tools/bpf/bpftool/feature.c | 219 +++++++++++++++++++++---------------
 1 file changed, 126 insertions(+), 93 deletions(-)

diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
index 941873d778d8..345e4a2b4f53 100644
--- a/tools/bpf/bpftool/feature.c
+++ b/tools/bpf/bpftool/feature.c
@@ -112,18 +112,12 @@ print_start_section(const char *json_title, const char *plain_title,
 	}
 }
 
-static void
-print_end_then_start_section(const char *json_title, const char *plain_title,
-			     const char *define_comment,
-			     const char *define_prefix)
+static void print_end_section(void)
 {
 	if (json_output)
 		jsonw_end_object(json_wtr);
 	else
 		printf("\n");
-
-	print_start_section(json_title, plain_title, define_comment,
-			    define_prefix);
 }
 
 /* Probing functions */
@@ -584,13 +578,130 @@ probe_large_insn_limit(const char *define_prefix, __u32 ifindex)
 			   res, define_prefix);
 }
 
+static void
+section_system_config(enum probe_component target, const char *define_prefix)
+{
+	switch (target) {
+	case COMPONENT_KERNEL:
+	case COMPONENT_UNSPEC:
+		if (define_prefix)
+			break;
+
+		print_start_section("system_config",
+				    "Scanning system configuration...",
+				    NULL, /* define_comment never used here */
+				    NULL); /* define_prefix always NULL here */
+		if (check_procfs()) {
+			probe_unprivileged_disabled();
+			probe_jit_enable();
+			probe_jit_harden();
+			probe_jit_kallsyms();
+			probe_jit_limit();
+		} else {
+			p_info("/* procfs not mounted, skipping related probes */");
+		}
+		probe_kernel_image_config();
+		print_end_section();
+		break;
+	default:
+		break;
+	}
+}
+
+static bool section_syscall_config(const char *define_prefix)
+{
+	bool res;
+
+	print_start_section("syscall_config",
+			    "Scanning system call availability...",
+			    "/*** System call availability ***/",
+			    define_prefix);
+	res = probe_bpf_syscall(define_prefix);
+	print_end_section();
+
+	return res;
+}
+
+static void
+section_program_types(bool *supported_types, const char *define_prefix,
+		      __u32 ifindex)
+{
+	unsigned int i;
+
+	print_start_section("program_types",
+			    "Scanning eBPF program types...",
+			    "/*** eBPF program types ***/",
+			    define_prefix);
+
+	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
+		probe_prog_type(i, supported_types, define_prefix, ifindex);
+
+	print_end_section();
+}
+
+static void section_map_types(const char *define_prefix, __u32 ifindex)
+{
+	unsigned int i;
+
+	print_start_section("map_types",
+			    "Scanning eBPF map types...",
+			    "/*** eBPF map types ***/",
+			    define_prefix);
+
+	for (i = BPF_MAP_TYPE_UNSPEC + 1; i < map_type_name_size; i++)
+		probe_map_type(i, define_prefix, ifindex);
+
+	print_end_section();
+}
+
+static void
+section_helpers(bool *supported_types, const char *define_prefix, __u32 ifindex)
+{
+	unsigned int i;
+
+	print_start_section("helpers",
+			    "Scanning eBPF helper functions...",
+			    "/*** eBPF helper functions ***/",
+			    define_prefix);
+
+	if (define_prefix)
+		printf("/*\n"
+		       " * Use %sHAVE_PROG_TYPE_HELPER(prog_type_name, helper_name)\n"
+		       " * to determine if <helper_name> is available for <prog_type_name>,\n"
+		       " * e.g.\n"
+		       " *	#if %sHAVE_PROG_TYPE_HELPER(xdp, bpf_redirect)\n"
+		       " *		// do stuff with this helper\n"
+		       " *	#elif\n"
+		       " *		// use a workaround\n"
+		       " *	#endif\n"
+		       " */\n"
+		       "#define %sHAVE_PROG_TYPE_HELPER(prog_type, helper)	\\\n"
+		       "	%sBPF__PROG_TYPE_ ## prog_type ## __HELPER_ ## helper\n",
+		       define_prefix, define_prefix, define_prefix,
+		       define_prefix);
+	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
+		probe_helpers_for_progtype(i, supported_types[i],
+					   define_prefix, ifindex);
+
+	print_end_section();
+}
+
+static void section_misc(const char *define_prefix, __u32 ifindex)
+{
+	print_start_section("misc",
+			    "Scanning miscellaneous eBPF features...",
+			    "/*** eBPF misc features ***/",
+			    define_prefix);
+	probe_large_insn_limit(define_prefix, ifindex);
+	print_end_section();
+}
+
 static int do_probe(int argc, char **argv)
 {
 	enum probe_component target = COMPONENT_UNSPEC;
 	const char *define_prefix = NULL;
 	bool supported_types[128] = {};
 	__u32 ifindex = 0;
-	unsigned int i;
 	char *ifname;
 
 	/* Detection assumes user has sufficient privileges (CAP_SYS_ADMIN).
@@ -658,97 +769,19 @@ static int do_probe(int argc, char **argv)
 		jsonw_start_object(json_wtr);
 	}
 
-	switch (target) {
-	case COMPONENT_KERNEL:
-	case COMPONENT_UNSPEC:
-		if (define_prefix)
-			break;
-
-		print_start_section("system_config",
-				    "Scanning system configuration...",
-				    NULL, /* define_comment never used here */
-				    NULL); /* define_prefix always NULL here */
-		if (check_procfs()) {
-			probe_unprivileged_disabled();
-			probe_jit_enable();
-			probe_jit_harden();
-			probe_jit_kallsyms();
-			probe_jit_limit();
-		} else {
-			p_info("/* procfs not mounted, skipping related probes */");
-		}
-		probe_kernel_image_config();
-		if (json_output)
-			jsonw_end_object(json_wtr);
-		else
-			printf("\n");
-		break;
-	default:
-		break;
-	}
-
-	print_start_section("syscall_config",
-			    "Scanning system call availability...",
-			    "/*** System call availability ***/",
-			    define_prefix);
-
-	if (!probe_bpf_syscall(define_prefix))
+	section_system_config(target, define_prefix);
+	if (!section_syscall_config(define_prefix))
 		/* bpf() syscall unavailable, don't probe other BPF features */
 		goto exit_close_json;
-
-	print_end_then_start_section("program_types",
-				     "Scanning eBPF program types...",
-				     "/*** eBPF program types ***/",
-				     define_prefix);
-
-	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
-		probe_prog_type(i, supported_types, define_prefix, ifindex);
-
-	print_end_then_start_section("map_types",
-				     "Scanning eBPF map types...",
-				     "/*** eBPF map types ***/",
-				     define_prefix);
-
-	for (i = BPF_MAP_TYPE_UNSPEC + 1; i < map_type_name_size; i++)
-		probe_map_type(i, define_prefix, ifindex);
-
-	print_end_then_start_section("helpers",
-				     "Scanning eBPF helper functions...",
-				     "/*** eBPF helper functions ***/",
-				     define_prefix);
-
-	if (define_prefix)
-		printf("/*\n"
-		       " * Use %sHAVE_PROG_TYPE_HELPER(prog_type_name, helper_name)\n"
-		       " * to determine if <helper_name> is available for <prog_type_name>,\n"
-		       " * e.g.\n"
-		       " *	#if %sHAVE_PROG_TYPE_HELPER(xdp, bpf_redirect)\n"
-		       " *		// do stuff with this helper\n"
-		       " *	#elif\n"
-		       " *		// use a workaround\n"
-		       " *	#endif\n"
-		       " */\n"
-		       "#define %sHAVE_PROG_TYPE_HELPER(prog_type, helper)	\\\n"
-		       "	%sBPF__PROG_TYPE_ ## prog_type ## __HELPER_ ## helper\n",
-		       define_prefix, define_prefix, define_prefix,
-		       define_prefix);
-	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
-		probe_helpers_for_progtype(i, supported_types[i],
-					   define_prefix, ifindex);
-
-	print_end_then_start_section("misc",
-				     "Scanning miscellaneous eBPF features...",
-				     "/*** eBPF misc features ***/",
-				     define_prefix);
-	probe_large_insn_limit(define_prefix, ifindex);
+	section_program_types(supported_types, define_prefix, ifindex);
+	section_map_types(define_prefix, ifindex);
+	section_helpers(supported_types, define_prefix, ifindex);
+	section_misc(define_prefix, ifindex);
 
 exit_close_json:
-	if (json_output) {
-		/* End current "section" of probes */
-		jsonw_end_object(json_wtr);
+	if (json_output)
 		/* End root object */
 		jsonw_end_object(json_wtr);
-	}
 
 	return 0;
 }
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional
  2020-02-21  3:16 [PATCH bpf-next v2 0/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
  2020-02-21  3:16 ` [PATCH bpf-next v2 1/5] bpftool: Move out sections to separate functions Michal Rostecki
@ 2020-02-21  3:16 ` Michal Rostecki
  2020-02-21 11:28   ` Quentin Monnet
  2020-02-21  3:16 ` [PATCH bpf-next v2 3/5] bpftool: Update documentation of "bpftool feature" command Michal Rostecki
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Michal Rostecki @ 2020-02-21  3:16 UTC (permalink / raw)
  To: bpf
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Quentin Monnet, Jakub Kicinski, netdev, linux-kernel, Shuah Khan,
	open list:KERNEL SELFTEST FRAMEWORK

Probes related to bpf_probe_write_user and bpf_trace_printk helpers emit
dmesg warnings which might be confusing for people running bpftool on
production environments. This change filters them out by default and
introduces the new positional argument "full" which enables all
available probes.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
---
 tools/bpf/bpftool/feature.c | 80 +++++++++++++++++++++++++++++++++----
 1 file changed, 73 insertions(+), 7 deletions(-)

diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
index 345e4a2b4f53..0731804b8160 100644
--- a/tools/bpf/bpftool/feature.c
+++ b/tools/bpf/bpftool/feature.c
@@ -3,6 +3,7 @@
 
 #include <ctype.h>
 #include <errno.h>
+#include <regex.h>
 #include <string.h>
 #include <unistd.h>
 #include <net/if.h>
@@ -22,6 +23,9 @@
 # define PROC_SUPER_MAGIC	0x9fa0
 #endif
 
+/* Regex pattern for filtering out probes which emit dmesg warnings */
+#define FILTER_OUT_PATTERN "(trace|write_user)"
+
 enum probe_component {
 	COMPONENT_UNSPEC,
 	COMPONENT_KERNEL,
@@ -57,6 +61,35 @@ static void uppercase(char *str, size_t len)
 		str[i] = toupper(str[i]);
 }
 
+/* Filtering utility functions */
+
+static bool
+check_filters(const char *name, regex_t *filter_out)
+{
+	char err_buf[100];
+	int ret;
+
+	/* Do not probe if filter_out was defined and string matches against the
+	 * pattern.
+	 */
+	if (filter_out) {
+		ret = regexec(filter_out, name, 0, NULL, 0);
+		switch (ret) {
+		case 0:
+			return false;
+		case REG_NOMATCH:
+			break;
+		default:
+			regerror(ret, filter_out, err_buf, ARRAY_SIZE(err_buf));
+			p_err("could not match regex: %s", err_buf);
+			free(filter_out);
+			exit(1);
+		}
+	}
+
+	return true;
+}
+
 /* Printing utility functions */
 
 static void
@@ -515,7 +548,8 @@ probe_map_type(enum bpf_map_type map_type, const char *define_prefix,
 
 static void
 probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
-			   const char *define_prefix, __u32 ifindex)
+			   const char *define_prefix, regex_t *filter_out,
+			   __u32 ifindex)
 {
 	const char *ptype_name = prog_type_name[prog_type];
 	char feat_name[128];
@@ -542,6 +576,9 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
 	}
 
 	for (id = 1; id < ARRAY_SIZE(helper_name); id++) {
+		if (!check_filters(helper_name[id], filter_out))
+			continue;
+
 		if (!supported_type)
 			res = false;
 		else
@@ -634,7 +671,8 @@ section_program_types(bool *supported_types, const char *define_prefix,
 			    define_prefix);
 
 	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
-		probe_prog_type(i, supported_types, define_prefix, ifindex);
+		probe_prog_type(i, supported_types, define_prefix,
+				ifindex);
 
 	print_end_section();
 }
@@ -655,7 +693,8 @@ static void section_map_types(const char *define_prefix, __u32 ifindex)
 }
 
 static void
-section_helpers(bool *supported_types, const char *define_prefix, __u32 ifindex)
+section_helpers(bool *supported_types, const char *define_prefix,
+		regex_t *filter_out, __u32 ifindex)
 {
 	unsigned int i;
 
@@ -681,7 +720,7 @@ section_helpers(bool *supported_types, const char *define_prefix, __u32 ifindex)
 		       define_prefix);
 	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
 		probe_helpers_for_progtype(i, supported_types[i],
-					   define_prefix, ifindex);
+					   define_prefix, filter_out, ifindex);
 
 	print_end_section();
 }
@@ -701,8 +740,13 @@ static int do_probe(int argc, char **argv)
 	enum probe_component target = COMPONENT_UNSPEC;
 	const char *define_prefix = NULL;
 	bool supported_types[128] = {};
+	regex_t *filter_out = NULL;
+	bool full_mode = false;
+	char regerror_buf[100];
 	__u32 ifindex = 0;
 	char *ifname;
+	int reg_ret;
+	int ret = 0;
 
 	/* Detection assumes user has sufficient privileges (CAP_SYS_ADMIN).
 	 * Let's approximate, and restrict usage to root user only.
@@ -740,6 +784,9 @@ static int do_probe(int argc, char **argv)
 				      strerror(errno));
 				return -1;
 			}
+		} else if (is_prefix(*argv, "full")) {
+			full_mode = true;
+			NEXT_ARG();
 		} else if (is_prefix(*argv, "macros") && !define_prefix) {
 			define_prefix = "";
 			NEXT_ARG();
@@ -764,6 +811,22 @@ static int do_probe(int argc, char **argv)
 		}
 	}
 
+	/* If full mode is not acivated, filter out probes which emit dmesg
+	 * messages.
+	 */
+	if (!full_mode) {
+		filter_out = malloc(sizeof(regex_t));
+		reg_ret = regcomp(filter_out, FILTER_OUT_PATTERN, REG_EXTENDED);
+		if (reg_ret) {
+			regerror(reg_ret, filter_out, regerror_buf,
+				 ARRAY_SIZE(regerror_buf));
+			p_err("could not compile regex: %s",
+			      regerror_buf);
+			ret = -1;
+			goto cleanup;
+		}
+	}
+
 	if (json_output) {
 		define_prefix = NULL;
 		jsonw_start_object(json_wtr);
@@ -775,7 +838,7 @@ static int do_probe(int argc, char **argv)
 		goto exit_close_json;
 	section_program_types(supported_types, define_prefix, ifindex);
 	section_map_types(define_prefix, ifindex);
-	section_helpers(supported_types, define_prefix, ifindex);
+	section_helpers(supported_types, define_prefix, filter_out, ifindex);
 	section_misc(define_prefix, ifindex);
 
 exit_close_json:
@@ -783,7 +846,10 @@ static int do_probe(int argc, char **argv)
 		/* End root object */
 		jsonw_end_object(json_wtr);
 
-	return 0;
+cleanup:
+	free(filter_out);
+
+	return ret;
 }
 
 static int do_help(int argc, char **argv)
@@ -794,7 +860,7 @@ static int do_help(int argc, char **argv)
 	}
 
 	fprintf(stderr,
-		"Usage: %s %s probe [COMPONENT] [macros [prefix PREFIX]]\n"
+		"Usage: %s %s probe [COMPONENT] [full] [macros [prefix PREFIX]]\n"
 		"       %s %s help\n"
 		"\n"
 		"       COMPONENT := { kernel | dev NAME }\n"
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH bpf-next v2 3/5] bpftool: Update documentation of "bpftool feature" command
  2020-02-21  3:16 [PATCH bpf-next v2 0/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
  2020-02-21  3:16 ` [PATCH bpf-next v2 1/5] bpftool: Move out sections to separate functions Michal Rostecki
  2020-02-21  3:16 ` [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
@ 2020-02-21  3:16 ` Michal Rostecki
  2020-02-21 11:28   ` Quentin Monnet
  2020-02-21  3:16 ` [PATCH bpf-next v2 4/5] bpftool: Update bash completion for " Michal Rostecki
  2020-02-21  3:17 ` [PATCH bpf-next v2 5/5] selftests/bpf: Add test " Michal Rostecki
  4 siblings, 1 reply; 16+ messages in thread
From: Michal Rostecki @ 2020-02-21  3:16 UTC (permalink / raw)
  To: bpf
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Quentin Monnet, Jakub Kicinski, netdev, linux-kernel, Shuah Khan,
	open list:KERNEL SELFTEST FRAMEWORK

Update documentation of "bpftool feature" command with information about
new arguments: "full".

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
---
 .../bpf/bpftool/Documentation/bpftool-feature.rst | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-feature.rst b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
index 4d08f35034a2..2e8f66ee1e77 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
@@ -19,19 +19,24 @@ SYNOPSIS
 FEATURE COMMANDS
 ================
 
-|	**bpftool** **feature probe** [*COMPONENT*] [**macros** [**prefix** *PREFIX*]]
+|	**bpftool** **feature probe** [*COMPONENT*] [**full**] [**macros** [**prefix** *PREFIX*]]
 |	**bpftool** **feature help**
 |
 |	*COMPONENT* := { **kernel** | **dev** *NAME* }
 
 DESCRIPTION
 ===========
-	**bpftool feature probe** [**kernel**] [**macros** [**prefix** *PREFIX*]]
+	**bpftool feature probe** [**kernel**] [**full**] [**macros** [**prefix** *PREFIX*]]
 		  Probe the running kernel and dump a number of eBPF-related
 		  parameters, such as availability of the **bpf()** system call,
 		  JIT status, eBPF program types availability, eBPF helper
 		  functions availability, and more.
 
+		  By default, bpftool does not run probes for
+		  bpf_probe_write_user and bpf_trace_printk helpers which emit
+		  dmesg warnings. To enable them and run all probes, the
+		  **full** keyword should be used.
+
 		  If the **macros** keyword (but not the **-j** option) is
 		  passed, a subset of the output is dumped as a list of
 		  **#define** macros that are ready to be included in a C
@@ -48,12 +53,12 @@ DESCRIPTION
 		  **bpf_trace_printk**\ () or **bpf_probe_write_user**\ ()) may
 		  print warnings to kernel logs.
 
-	**bpftool feature probe dev** *NAME* [**macros** [**prefix** *PREFIX*]]
+	**bpftool feature probe dev** *NAME* [**full**] [**macros** [**prefix** *PREFIX*]]
 		  Probe network device for supported eBPF features and dump
 		  results to the console.
 
-		  The two keywords **macros** and **prefix** have the same
-		  role as when probing the kernel.
+		  The keywords **full**, **macros** and **prefix** have the
+		  same role as when probing the kernel.
 
 	**bpftool feature help**
 		  Print short help message.
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH bpf-next v2 4/5] bpftool: Update bash completion for "bpftool feature" command
  2020-02-21  3:16 [PATCH bpf-next v2 0/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
                   ` (2 preceding siblings ...)
  2020-02-21  3:16 ` [PATCH bpf-next v2 3/5] bpftool: Update documentation of "bpftool feature" command Michal Rostecki
@ 2020-02-21  3:16 ` Michal Rostecki
  2020-02-21 11:29   ` Quentin Monnet
  2020-02-21  3:17 ` [PATCH bpf-next v2 5/5] selftests/bpf: Add test " Michal Rostecki
  4 siblings, 1 reply; 16+ messages in thread
From: Michal Rostecki @ 2020-02-21  3:16 UTC (permalink / raw)
  To: bpf
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Quentin Monnet, Jakub Kicinski, netdev, linux-kernel, Shuah Khan,
	open list:KERNEL SELFTEST FRAMEWORK

Update bash completion for "bpftool feature" command with the new
argument: "full".

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
---
 tools/bpf/bpftool/bash-completion/bpftool | 27 ++++++++++++++++-------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 754d8395e451..f2bcc4bacee2 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -981,14 +981,25 @@ _bpftool()
         feature)
             case $command in
                 probe)
-                    [[ $prev == "prefix" ]] && return 0
-                    if _bpftool_search_list 'macros'; then
-                        COMPREPLY+=( $( compgen -W 'prefix' -- "$cur" ) )
-                    else
-                        COMPREPLY+=( $( compgen -W 'macros' -- "$cur" ) )
-                    fi
-                    _bpftool_one_of_list 'kernel dev'
-                    return 0
+                    case $prev in
+                        $command)
+                            COMPREPLY+=( $( compgen -W 'kernel dev full macros' -- \
+                                "$cur" ) )
+                            return 0
+                            ;;
+                        prefix)
+                            return 0
+                            ;;
+                        macros)
+                            COMPREPLY+=( $( compgen -W 'prefix' -- "$cur" ) )
+                            return 0
+                            ;;
+                        *)
+                            _bpftool_one_of_list 'kernel dev'
+                            _bpftool_once_attr 'full macros'
+                            return 0
+                            ;;
+                    esac
                     ;;
                 *)
                     [[ $prev == $object ]] && \
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH bpf-next v2 5/5] selftests/bpf: Add test for "bpftool feature" command
  2020-02-21  3:16 [PATCH bpf-next v2 0/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
                   ` (3 preceding siblings ...)
  2020-02-21  3:16 ` [PATCH bpf-next v2 4/5] bpftool: Update bash completion for " Michal Rostecki
@ 2020-02-21  3:17 ` Michal Rostecki
  2020-02-21 11:28   ` Quentin Monnet
  4 siblings, 1 reply; 16+ messages in thread
From: Michal Rostecki @ 2020-02-21  3:17 UTC (permalink / raw)
  To: bpf
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Quentin Monnet, Jakub Kicinski, netdev, linux-kernel, Shuah Khan,
	David S. Miller, Jesper Dangaard Brouer, John Fastabend,
	open list:KERNEL SELFTEST FRAMEWORK

Add Python module with tests for "bpftool feature" command, which mainly
wheck whether the "full" option is working properly.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
---
 tools/testing/selftests/.gitignore          |   5 +-
 tools/testing/selftests/bpf/Makefile        |   3 +-
 tools/testing/selftests/bpf/test_bpftool.py | 228 ++++++++++++++++++++
 tools/testing/selftests/bpf/test_bpftool.sh |   5 +
 4 files changed, 239 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/test_bpftool.py
 create mode 100755 tools/testing/selftests/bpf/test_bpftool.sh

diff --git a/tools/testing/selftests/.gitignore b/tools/testing/selftests/.gitignore
index 61df01cdf0b2..304fdf1a21dc 100644
--- a/tools/testing/selftests/.gitignore
+++ b/tools/testing/selftests/.gitignore
@@ -3,4 +3,7 @@ gpiogpio-hammer
 gpioinclude/
 gpiolsgpio
 tpm2/SpaceTest.log
-tpm2/*.pyc
+
+# Python bytecode and cache
+__pycache__/
+*.py[cod]
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 257a1aaaa37d..e7d822259c50 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -62,7 +62,8 @@ TEST_PROGS := test_kmod.sh \
 	test_tc_tunnel.sh \
 	test_tc_edt.sh \
 	test_xdping.sh \
-	test_bpftool_build.sh
+	test_bpftool_build.sh \
+	test_bpftool.sh
 
 TEST_PROGS_EXTENDED := with_addr.sh \
 	with_tunnels.sh \
diff --git a/tools/testing/selftests/bpf/test_bpftool.py b/tools/testing/selftests/bpf/test_bpftool.py
new file mode 100644
index 000000000000..7f545feaec98
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_bpftool.py
@@ -0,0 +1,228 @@
+# Copyright (c) 2020 SUSE LLC.
+#
+# This software is licensed under the GNU General License Version 2,
+# June 1991 as shown in the file COPYING in the top-level directory of this
+# source tree.
+#
+# THE COPYRIGHT HOLDERS AND/OR OTHER PARTIES PROVIDE THE PROGRAM "AS IS"
+# WITHOUT WARRANTY OF ANY KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING,
+# BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+# FOR A PARTICULAR PURPOSE. THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE
+# OF THE PROGRAM IS WITH YOU. SHOULD THE PROGRAM PROVE DEFECTIVE, YOU ASSUME
+# THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.
+
+import collections
+import functools
+import json
+import os
+import socket
+import subprocess
+import unittest
+
+
+# Add the source tree of bpftool and /usr/local/sbin to PATH
+cur_dir = os.path.dirname(os.path.realpath(__file__))
+bpftool_dir = os.path.abspath(os.path.join(cur_dir, "..", "..", "..", "..",
+                                           "tools", "bpf", "bpftool"))
+os.environ["PATH"] = bpftool_dir + ":/usr/local/sbin:" + os.environ["PATH"]
+
+# Probe sections
+SECTION_SYSTEM_CONFIG_PATTERN = b"Scanning system configuration..."
+SECTION_SYSCALL_CONFIG_PATTERN = b"Scanning system call availability..."
+SECTION_PROGRAM_TYPES_PATTERN = b"Scanning eBPF program types..."
+SECTION_MAP_TYPES_PATTERN = b"Scanning eBPF map types..."
+SECTION_HELPERS_PATTERN = b"Scanning eBPF helper functions..."
+SECTION_MISC_PATTERN = b"Scanning miscellaneous eBPF features..."
+
+
+class IfaceNotFoundError(Exception):
+    pass
+
+
+class UnprivilegedUserError(Exception):
+    pass
+
+
+def _bpftool(args, json=True):
+    _args = ["bpftool"]
+    if json:
+        _args.append("-j")
+    _args.extend(args)
+
+    res = subprocess.run(_args, capture_output=True)
+    return res.stdout
+
+
+def bpftool(args):
+    return _bpftool(args, json=False)
+
+
+def bpftool_json(args):
+    res = _bpftool(args)
+    return json.loads(res)
+
+
+def get_default_iface():
+    for iface in socket.if_nameindex():
+        if iface[1] != "lo":
+            return iface[1]
+    raise IfaceNotFoundError("Could not find any network interface to probe")
+
+
+def default_iface(f):
+    @functools.wraps(f)
+    def wrapper(*args, **kwargs):
+        iface = get_default_iface()
+        return f(*args, iface, **kwargs)
+    return wrapper
+
+
+class TestBpftool(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        if os.getuid() != 0:
+            raise UnprivilegedUserError("This test suite eeeds root privileges")
+
+    def _assert_pattern_not_in_dict(self, dct, pattern, check_keys=False):
+        """Check if all string values inside dictionary do not containe the
+        given pattern.
+        """
+        for key, value in dct.items():
+            if check_keys:
+                self.assertNotIn(pattern, key)
+            if isinstance(value, dict):
+                self._assert_pattern_not_in_dict(value, pattern,
+                                                 check_keys=True)
+            elif isinstance(value, str):
+                self.assertNotIn(pattern, value)
+
+    @default_iface
+    def test_feature_dev(self, iface):
+        expected_patterns = [
+            SECTION_SYSCALL_CONFIG_PATTERN,
+            SECTION_PROGRAM_TYPES_PATTERN,
+            SECTION_MAP_TYPES_PATTERN,
+            SECTION_HELPERS_PATTERN,
+            SECTION_MISC_PATTERN,
+        ]
+        unexpected_patterns = [
+            b"bpf_trace_printk",
+            b"bpf_probe_write_user",
+        ]
+
+        res = bpftool(["feature", "probe", "dev", iface])
+        for pattern in expected_patterns:
+            self.assertIn(pattern, res)
+        for pattern in unexpected_patterns:
+            self.assertNotIn(pattern, res)
+
+    @default_iface
+    def test_feature_dev_json(self, iface):
+        expected_keys = [
+            "syscall_config",
+            "program_types",
+            "map_types",
+            "helpers",
+            "misc",
+        ]
+        unexpected_values = [
+            "bpf_trace_printk",
+            "bpf_probe_write_user",
+        ]
+
+        res = bpftool_json(["feature", "probe", "dev", iface])
+        self.assertCountEqual(res.keys(), expected_keys)
+        for value in unexpected_values:
+            self._assert_pattern_not_in_dict(res, value)
+
+    def test_feature_kernel(self):
+        expected_patterns = [
+            SECTION_SYSTEM_CONFIG_PATTERN,
+            SECTION_SYSCALL_CONFIG_PATTERN,
+            SECTION_PROGRAM_TYPES_PATTERN,
+            SECTION_MAP_TYPES_PATTERN,
+            SECTION_HELPERS_PATTERN,
+            SECTION_MISC_PATTERN,
+        ]
+        unexpected_patterns = [
+            b"bpf_trace_printk",
+            b"bpf_probe_write_user",
+        ]
+
+        res_default1 = bpftool(["feature"])
+        res_default2 = bpftool(["feature", "probe"])
+        res = bpftool(["feature", "probe", "kernel"])
+
+        for pattern in expected_patterns:
+            self.assertIn(pattern, res_default1)
+            self.assertIn(pattern, res_default2)
+            self.assertIn(pattern, res)
+        for pattern in unexpected_patterns:
+            self.assertNotIn(pattern, res_default1)
+            self.assertNotIn(pattern, res_default2)
+            self.assertNotIn(pattern, res)
+
+    def test_feature_kernel_full(self):
+        expected_patterns = [
+            SECTION_SYSTEM_CONFIG_PATTERN,
+            SECTION_SYSCALL_CONFIG_PATTERN,
+            SECTION_PROGRAM_TYPES_PATTERN,
+            SECTION_MAP_TYPES_PATTERN,
+            SECTION_HELPERS_PATTERN,
+            SECTION_MISC_PATTERN,
+            b"bpf_trace_printk",
+            b"bpf_probe_write_user",
+        ]
+
+        res_default = bpftool(["feature", "probe", "full"])
+        res = bpftool(["feature", "probe", "kernel", "full"])
+
+        for pattern in expected_patterns:
+            self.assertIn(pattern, res_default)
+            self.assertIn(pattern, res)
+
+    def test_feature_kernel_json(self):
+        expected_keys = [
+            "system_config",
+            "syscall_config",
+            "program_types",
+            "map_types",
+            "helpers",
+            "misc",
+        ]
+        unexpected_values = [
+            "bpf_trace_printk",
+            "bpf_probe_write_user",
+        ]
+
+        res_default1 = bpftool_json(["feature"])
+        self.assertCountEqual(res_default1.keys(), expected_keys)
+        for value in unexpected_values:
+            self._assert_pattern_not_in_dict(res_default1, value)
+
+        res_default2 = bpftool_json(["feature", "probe"])
+        self.assertCountEqual(res_default2.keys(), expected_keys)
+        for value in unexpected_values:
+            self._assert_pattern_not_in_dict(res_default2, value)
+
+        res = bpftool_json(["feature", "probe", "kernel"])
+        self.assertCountEqual(res.keys(), expected_keys)
+        for value in unexpected_values:
+            self._assert_pattern_not_in_dict(res, value)
+
+    def test_feature_macros(self):
+        expected_patterns = [
+            b"/\*\*\* System call availability \*\*\*/",
+            b"#define HAVE_BPF_SYSCALL",
+            b"/\*\*\* eBPF program types \*\*\*/",
+            b"#define HAVE.*PROG_TYPE",
+            b"/\*\*\* eBPF map types \*\*\*/",
+            b"#define HAVE.*MAP_TYPE",
+            b"/\*\*\* eBPF helper functions \*\*\*/",
+            b"#define HAVE.*HELPER",
+            b"/\*\*\* eBPF misc features \*\*\*/",
+        ]
+
+        res = bpftool(["feature", "probe", "macros"])
+        for pattern in expected_patterns:
+            self.assertRegex(res, pattern)
diff --git a/tools/testing/selftests/bpf/test_bpftool.sh b/tools/testing/selftests/bpf/test_bpftool.sh
new file mode 100755
index 000000000000..66690778e36d
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_bpftool.sh
@@ -0,0 +1,5 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020 SUSE LLC.
+
+python3 -m unittest -v test_bpftool.TestBpftool
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 1/5] bpftool: Move out sections to separate functions
  2020-02-21  3:16 ` [PATCH bpf-next v2 1/5] bpftool: Move out sections to separate functions Michal Rostecki
@ 2020-02-21 11:27   ` Quentin Monnet
  0 siblings, 0 replies; 16+ messages in thread
From: Quentin Monnet @ 2020-02-21 11:27 UTC (permalink / raw)
  To: Michal Rostecki, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Quentin Monnet, Jakub Kicinski,
	netdev, linux-kernel, Shuah Khan, David S. Miller,
	Jesper Dangaard Brouer, John Fastabend,
	open list:KERNEL SELFTEST FRAMEWORK

2020-02-21 04:16 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org>
> Remove all calls of print_end_then_start_section function and for loops
> out from the do_probe function. Instead, provide separate functions for
> each section (like i.e. section_helpers) which are called in do_probe.
> This change is motivated by better readability.
> 
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Thanks!

Reviewed-by: Quentin Monnet <quentin@isovalent.com>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional
  2020-02-21  3:16 ` [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
@ 2020-02-21 11:28   ` Quentin Monnet
  2020-02-21 22:44     ` Alexei Starovoitov
  0 siblings, 1 reply; 16+ messages in thread
From: Quentin Monnet @ 2020-02-21 11:28 UTC (permalink / raw)
  To: Michal Rostecki, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Jakub Kicinski, netdev,
	linux-kernel, Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK

2020-02-21 04:16 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org>
> Probes related to bpf_probe_write_user and bpf_trace_printk helpers emit
> dmesg warnings which might be confusing for people running bpftool on
> production environments. This change filters them out by default and
> introduces the new positional argument "full" which enables all
> available probes.
> 
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> ---
>   tools/bpf/bpftool/feature.c | 80 +++++++++++++++++++++++++++++++++----
>   1 file changed, 73 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index 345e4a2b4f53..0731804b8160 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -3,6 +3,7 @@
>   
>   #include <ctype.h>
>   #include <errno.h>
> +#include <regex.h>
>   #include <string.h>
>   #include <unistd.h>
>   #include <net/if.h>
> @@ -22,6 +23,9 @@
>   # define PROC_SUPER_MAGIC	0x9fa0
>   #endif
>   
> +/* Regex pattern for filtering out probes which emit dmesg warnings */
> +#define FILTER_OUT_PATTERN "(trace|write_user)"

"trace" sounds too generic. If filters are applied again to prog and map 
types in the future (as you had in v1), this would catch tracepoint and 
raw_tracepoint program types and stack_trace map type. Or if new helpers 
with "trace" in their name are added, we skip them too. Can we use 
something more specific, probably "trace_printk"?

> +
>   enum probe_component {
>   	COMPONENT_UNSPEC,
>   	COMPONENT_KERNEL,
> @@ -57,6 +61,35 @@ static void uppercase(char *str, size_t len)
>   		str[i] = toupper(str[i]);
>   }
>   
> +/* Filtering utility functions */
> +
> +static bool
> +check_filters(const char *name, regex_t *filter_out)
> +{
> +	char err_buf[100];
> +	int ret;
> +
> +	/* Do not probe if filter_out was defined and string matches against the
> +	 * pattern.
> +	 */
> +	if (filter_out) {
> +		ret = regexec(filter_out, name, 0, NULL, 0);
> +		switch (ret) {
> +		case 0:
> +			return false;
> +		case REG_NOMATCH:
> +			break;
> +		default:
> +			regerror(ret, filter_out, err_buf, ARRAY_SIZE(err_buf));
> +			p_err("could not match regex: %s", err_buf);
> +			free(filter_out);
> +			exit(1);
> +		}
> +	}
> +
> +	return true;
> +}
> +
>   /* Printing utility functions */
>   
>   static void
> @@ -515,7 +548,8 @@ probe_map_type(enum bpf_map_type map_type, const char *define_prefix,
>   
>   static void
>   probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
> -			   const char *define_prefix, __u32 ifindex)
> +			   const char *define_prefix, regex_t *filter_out,
> +			   __u32 ifindex)
>   {
>   	const char *ptype_name = prog_type_name[prog_type];
>   	char feat_name[128];
> @@ -542,6 +576,9 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
>   	}
>   
>   	for (id = 1; id < ARRAY_SIZE(helper_name); id++) {
> +		if (!check_filters(helper_name[id], filter_out))
> +			continue;
> +
>   		if (!supported_type)
>   			res = false;
>   		else
> @@ -634,7 +671,8 @@ section_program_types(bool *supported_types, const char *define_prefix,
>   			    define_prefix);
>   
>   	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
> -		probe_prog_type(i, supported_types, define_prefix, ifindex);
> +		probe_prog_type(i, supported_types, define_prefix,
> +				ifindex);

Splitting the line here is not desirable, probably some leftover after 
rolling back on changes?

>   
>   	print_end_section();
>   }
> @@ -655,7 +693,8 @@ static void section_map_types(const char *define_prefix, __u32 ifindex)
>   }
>   
>   static void
> -section_helpers(bool *supported_types, const char *define_prefix, __u32 ifindex)
> +section_helpers(bool *supported_types, const char *define_prefix,
> +		regex_t *filter_out, __u32 ifindex)
>   {
>   	unsigned int i;
>   
> @@ -681,7 +720,7 @@ section_helpers(bool *supported_types, const char *define_prefix, __u32 ifindex)
>   		       define_prefix);
>   	for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
>   		probe_helpers_for_progtype(i, supported_types[i],
> -					   define_prefix, ifindex);
> +					   define_prefix, filter_out, ifindex);
>   
>   	print_end_section();
>   }
> @@ -701,8 +740,13 @@ static int do_probe(int argc, char **argv)
>   	enum probe_component target = COMPONENT_UNSPEC;
>   	const char *define_prefix = NULL;
>   	bool supported_types[128] = {};
> +	regex_t *filter_out = NULL;
> +	bool full_mode = false;
> +	char regerror_buf[100];
>   	__u32 ifindex = 0;
>   	char *ifname;
> +	int reg_ret;
> +	int ret = 0;
>   
>   	/* Detection assumes user has sufficient privileges (CAP_SYS_ADMIN).
>   	 * Let's approximate, and restrict usage to root user only.
> @@ -740,6 +784,9 @@ static int do_probe(int argc, char **argv)
>   				      strerror(errno));
>   				return -1;
>   			}
> +		} else if (is_prefix(*argv, "full")) {
> +			full_mode = true;
> +			NEXT_ARG();
>   		} else if (is_prefix(*argv, "macros") && !define_prefix) {
>   			define_prefix = "";
>   			NEXT_ARG();
> @@ -764,6 +811,22 @@ static int do_probe(int argc, char **argv)
>   		}
>   	}
>   
> +	/* If full mode is not acivated, filter out probes which emit dmesg

Typo: acivated

> +	 * messages.
> +	 */
> +	if (!full_mode) {
> +		filter_out = malloc(sizeof(regex_t));

filter_out is not free()-d on the different error paths in the function. 
You would probably have to `goto cleanup` from several other locations.

> +		reg_ret = regcomp(filter_out, FILTER_OUT_PATTERN, REG_EXTENDED);
> +		if (reg_ret) {
> +			regerror(reg_ret, filter_out, regerror_buf,
> +				 ARRAY_SIZE(regerror_buf));
> +			p_err("could not compile regex: %s",
> +			      regerror_buf);
> +			ret = -1;
> +			goto cleanup;
> +		}
> +	}
> +
>   	if (json_output) {
>   		define_prefix = NULL;
>   		jsonw_start_object(json_wtr);
> @@ -775,7 +838,7 @@ static int do_probe(int argc, char **argv)
>   		goto exit_close_json;
>   	section_program_types(supported_types, define_prefix, ifindex);
>   	section_map_types(define_prefix, ifindex);
> -	section_helpers(supported_types, define_prefix, ifindex);
> +	section_helpers(supported_types, define_prefix, filter_out, ifindex);
>   	section_misc(define_prefix, ifindex);
>   
>   exit_close_json:
> @@ -783,7 +846,10 @@ static int do_probe(int argc, char **argv)
>   		/* End root object */
>   		jsonw_end_object(json_wtr);
>   
> -	return 0;
> +cleanup:
> +	free(filter_out);
> +
> +	return ret;
>   }
>   
>   static int do_help(int argc, char **argv)
> @@ -794,7 +860,7 @@ static int do_help(int argc, char **argv)
>   	}
>   
>   	fprintf(stderr,
> -		"Usage: %s %s probe [COMPONENT] [macros [prefix PREFIX]]\n"
> +		"Usage: %s %s probe [COMPONENT] [full] [macros [prefix PREFIX]]\n"
>   		"       %s %s help\n"
>   		"\n"
>   		"       COMPONENT := { kernel | dev NAME }\n"
> 

Thanks for the patch! While I understand you want to keep the changes 
you have done to use regex, I do not really think they bring much in 
this version of the patch. As we only want to filter out two specific 
helpers, it seems to me that it would be much simpler to just compare 
helper names instead of introducing regular expressions that are not 
used otherwise. What do you think?

Quentin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 3/5] bpftool: Update documentation of "bpftool feature" command
  2020-02-21  3:16 ` [PATCH bpf-next v2 3/5] bpftool: Update documentation of "bpftool feature" command Michal Rostecki
@ 2020-02-21 11:28   ` Quentin Monnet
  0 siblings, 0 replies; 16+ messages in thread
From: Quentin Monnet @ 2020-02-21 11:28 UTC (permalink / raw)
  To: Michal Rostecki, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Jakub Kicinski, netdev,
	linux-kernel, Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK

2020-02-21 04:16 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org>
> Update documentation of "bpftool feature" command with information about
> new arguments: "full".
> 
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> ---
>   .../bpf/bpftool/Documentation/bpftool-feature.rst | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-feature.rst b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
> index 4d08f35034a2..2e8f66ee1e77 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
> @@ -19,19 +19,24 @@ SYNOPSIS
>   FEATURE COMMANDS
>   ================
>   
> -|	**bpftool** **feature probe** [*COMPONENT*] [**macros** [**prefix** *PREFIX*]]
> +|	**bpftool** **feature probe** [*COMPONENT*] [**full**] [**macros** [**prefix** *PREFIX*]]
>   |	**bpftool** **feature help**
>   |
>   |	*COMPONENT* := { **kernel** | **dev** *NAME* }
>   
>   DESCRIPTION
>   ===========
> -	**bpftool feature probe** [**kernel**] [**macros** [**prefix** *PREFIX*]]
> +	**bpftool feature probe** [**kernel**] [**full**] [**macros** [**prefix** *PREFIX*]]
>   		  Probe the running kernel and dump a number of eBPF-related
>   		  parameters, such as availability of the **bpf()** system call,
>   		  JIT status, eBPF program types availability, eBPF helper
>   		  functions availability, and more.
>   
> +		  By default, bpftool does not run probes for
> +		  bpf_probe_write_user and bpf_trace_printk helpers which emit

Please use formatting on helper function names, for readability and 
consistency.

I would even be tempted to highlight part or all of the sentence, with 
caps or bold, as some users may be surprised not to see those helpers in 
the list of available helpers on their system.

> +		  dmesg warnings. To enable them and run all probes, the
> +		  **full** keyword should be used.
> +
>   		  If the **macros** keyword (but not the **-j** option) is
>   		  passed, a subset of the output is dumped as a list of
>   		  **#define** macros that are ready to be included in a C
> @@ -48,12 +53,12 @@ DESCRIPTION
>   		  **bpf_trace_printk**\ () or **bpf_probe_write_user**\ ()) may
>   		  print warnings to kernel logs.

This should maybe be moved upwards and combined with your new paragraph?

>   
> -	**bpftool feature probe dev** *NAME* [**macros** [**prefix** *PREFIX*]]
> +	**bpftool feature probe dev** *NAME* [**full**] [**macros** [**prefix** *PREFIX*]]
>   		  Probe network device for supported eBPF features and dump
>   		  results to the console.
>   
> -		  The two keywords **macros** and **prefix** have the same
> -		  role as when probing the kernel.
> +		  The keywords **full**, **macros** and **prefix** have the
> +		  same role as when probing the kernel.
>   
>   	**bpftool feature help**
>   		  Print short help message.
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add test for "bpftool feature" command
  2020-02-21  3:17 ` [PATCH bpf-next v2 5/5] selftests/bpf: Add test " Michal Rostecki
@ 2020-02-21 11:28   ` Quentin Monnet
  2020-02-25 13:25     ` Michal Rostecki
  2020-02-25 13:55     ` Michal Rostecki
  0 siblings, 2 replies; 16+ messages in thread
From: Quentin Monnet @ 2020-02-21 11:28 UTC (permalink / raw)
  To: Michal Rostecki, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Quentin Monnet, Jakub Kicinski,
	netdev, linux-kernel, Shuah Khan, David S. Miller,
	Jesper Dangaard Brouer, John Fastabend,
	open list:KERNEL SELFTEST FRAMEWORK

2020-02-21 04:17 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org>
> Add Python module with tests for "bpftool feature" command, which mainly
> wheck whether the "full" option is working properly.
> 
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> ---
>   tools/testing/selftests/.gitignore          |   5 +-
>   tools/testing/selftests/bpf/Makefile        |   3 +-
>   tools/testing/selftests/bpf/test_bpftool.py | 228 ++++++++++++++++++++
>   tools/testing/selftests/bpf/test_bpftool.sh |   5 +
>   4 files changed, 239 insertions(+), 2 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/test_bpftool.py
>   create mode 100755 tools/testing/selftests/bpf/test_bpftool.sh
> 
> diff --git a/tools/testing/selftests/.gitignore b/tools/testing/selftests/.gitignore
> index 61df01cdf0b2..304fdf1a21dc 100644
> --- a/tools/testing/selftests/.gitignore
> +++ b/tools/testing/selftests/.gitignore
> @@ -3,4 +3,7 @@ gpiogpio-hammer
>   gpioinclude/
>   gpiolsgpio
>   tpm2/SpaceTest.log
> -tpm2/*.pyc
> +
> +# Python bytecode and cache
> +__pycache__/
> +*.py[cod]
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 257a1aaaa37d..e7d822259c50 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -62,7 +62,8 @@ TEST_PROGS := test_kmod.sh \
>   	test_tc_tunnel.sh \
>   	test_tc_edt.sh \
>   	test_xdping.sh \
> -	test_bpftool_build.sh
> +	test_bpftool_build.sh \
> +	test_bpftool.sh
>   
>   TEST_PROGS_EXTENDED := with_addr.sh \
>   	with_tunnels.sh \
> diff --git a/tools/testing/selftests/bpf/test_bpftool.py b/tools/testing/selftests/bpf/test_bpftool.py
> new file mode 100644
> index 000000000000..7f545feaec98
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_bpftool.py
> @@ -0,0 +1,228 @@
> +# Copyright (c) 2020 SUSE LLC.
> +#
> +# This software is licensed under the GNU General License Version 2,
> +# June 1991 as shown in the file COPYING in the top-level directory of this
> +# source tree.
> +#
> +# THE COPYRIGHT HOLDERS AND/OR OTHER PARTIES PROVIDE THE PROGRAM "AS IS"
> +# WITHOUT WARRANTY OF ANY KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING,
> +# BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> +# FOR A PARTICULAR PURPOSE. THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE
> +# OF THE PROGRAM IS WITH YOU. SHOULD THE PROGRAM PROVE DEFECTIVE, YOU ASSUME
> +# THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.

SPDX tag instead of boilerplate?

> +
> +import collections
> +import functools
> +import json
> +import os
> +import socket
> +import subprocess
> +import unittest
> +
> +
> +# Add the source tree of bpftool and /usr/local/sbin to PATH
> +cur_dir = os.path.dirname(os.path.realpath(__file__))
> +bpftool_dir = os.path.abspath(os.path.join(cur_dir, "..", "..", "..", "..",
> +                                           "tools", "bpf", "bpftool"))
> +os.environ["PATH"] = bpftool_dir + ":/usr/local/sbin:" + os.environ["PATH"]
> +
> +# Probe sections
> +SECTION_SYSTEM_CONFIG_PATTERN = b"Scanning system configuration..."
> +SECTION_SYSCALL_CONFIG_PATTERN = b"Scanning system call availability..."
> +SECTION_PROGRAM_TYPES_PATTERN = b"Scanning eBPF program types..."
> +SECTION_MAP_TYPES_PATTERN = b"Scanning eBPF map types..."
> +SECTION_HELPERS_PATTERN = b"Scanning eBPF helper functions..."
> +SECTION_MISC_PATTERN = b"Scanning miscellaneous eBPF features..."
> +
> +
> +class IfaceNotFoundError(Exception):
> +    pass
> +
> +
> +class UnprivilegedUserError(Exception):
> +    pass
> +
> +
> +def _bpftool(args, json=True):
> +    _args = ["bpftool"]
> +    if json:
> +        _args.append("-j")
> +    _args.extend(args)
> +
> +    res = subprocess.run(_args, capture_output=True)
> +    return res.stdout
> +
> +
> +def bpftool(args):
> +    return _bpftool(args, json=False)
> +
> +
> +def bpftool_json(args):
> +    res = _bpftool(args)
> +    return json.loads(res)
> +
> +
> +def get_default_iface():
> +    for iface in socket.if_nameindex():
> +        if iface[1] != "lo":
> +            return iface[1]
> +    raise IfaceNotFoundError("Could not find any network interface to probe")
> +
> +
> +def default_iface(f):
> +    @functools.wraps(f)
> +    def wrapper(*args, **kwargs):
> +        iface = get_default_iface()
> +        return f(*args, iface, **kwargs)
> +    return wrapper
> +
> +
> +class TestBpftool(unittest.TestCase):
> +    @classmethod
> +    def setUpClass(cls):
> +        if os.getuid() != 0:
> +            raise UnprivilegedUserError("This test suite eeeds root privileges")

Typo: eeeds

> +
> +    def _assert_pattern_not_in_dict(self, dct, pattern, check_keys=False):
> +        """Check if all string values inside dictionary do not containe the

Typo: containe

> +        given pattern.
> +        """
> +        for key, value in dct.items():
> +            if check_keys:
> +                self.assertNotIn(pattern, key)
> +            if isinstance(value, dict):
> +                self._assert_pattern_not_in_dict(value, pattern,
> +                                                 check_keys=True)
> +            elif isinstance(value, str):
> +                self.assertNotIn(pattern, value)
> +
> +    @default_iface
> +    def test_feature_dev(self, iface):
> +        expected_patterns = [
> +            SECTION_SYSCALL_CONFIG_PATTERN,
> +            SECTION_PROGRAM_TYPES_PATTERN,
> +            SECTION_MAP_TYPES_PATTERN,
> +            SECTION_HELPERS_PATTERN,
> +            SECTION_MISC_PATTERN,
> +        ]

Mixed feeling on the tests with plain output, as we keep telling people 
that plain output should not be parsed (not reliable, may change). But 
if you want to run one or two tests with it, why not, I guess.

> +        unexpected_patterns = [
> +            b"bpf_trace_printk",
> +            b"bpf_probe_write_user",
> +        ]
> +
> +        res = bpftool(["feature", "probe", "dev", iface])
> +        for pattern in expected_patterns:
> +            self.assertIn(pattern, res)
> +        for pattern in unexpected_patterns:
> +            self.assertNotIn(pattern, res)
> +
> +    @default_iface
> +    def test_feature_dev_json(self, iface):
> +        expected_keys = [
> +            "syscall_config",
> +            "program_types",
> +            "map_types",
> +            "helpers",
> +            "misc",
> +        ]
> +        unexpected_values = [
> +            "bpf_trace_printk",
> +            "bpf_probe_write_user",
> +        ]
> +
> +        res = bpftool_json(["feature", "probe", "dev", iface])
> +        self.assertCountEqual(res.keys(), expected_keys)
> +        for value in unexpected_values:
> +            self._assert_pattern_not_in_dict(res, value)
> +
> +    def test_feature_kernel(self):
> +        expected_patterns = [
> +            SECTION_SYSTEM_CONFIG_PATTERN,
> +            SECTION_SYSCALL_CONFIG_PATTERN,
> +            SECTION_PROGRAM_TYPES_PATTERN,
> +            SECTION_MAP_TYPES_PATTERN,
> +            SECTION_HELPERS_PATTERN,
> +            SECTION_MISC_PATTERN,
> +        ]
> +        unexpected_patterns = [
> +            b"bpf_trace_printk",
> +            b"bpf_probe_write_user",
> +        ]
> +
> +        res_default1 = bpftool(["feature"])
> +        res_default2 = bpftool(["feature", "probe"])
> +        res = bpftool(["feature", "probe", "kernel"])
> +
> +        for pattern in expected_patterns:
> +            self.assertIn(pattern, res_default1)
> +            self.assertIn(pattern, res_default2)
> +            self.assertIn(pattern, res)
> +        for pattern in unexpected_patterns:
> +            self.assertNotIn(pattern, res_default1)
> +            self.assertNotIn(pattern, res_default2)
> +            self.assertNotIn(pattern, res)
> +
> +    def test_feature_kernel_full(self):
> +        expected_patterns = [
> +            SECTION_SYSTEM_CONFIG_PATTERN,
> +            SECTION_SYSCALL_CONFIG_PATTERN,
> +            SECTION_PROGRAM_TYPES_PATTERN,
> +            SECTION_MAP_TYPES_PATTERN,
> +            SECTION_HELPERS_PATTERN,
> +            SECTION_MISC_PATTERN,
> +            b"bpf_trace_printk",
> +            b"bpf_probe_write_user",
> +        ]

However, if you do just one test for "kernel full", please favour JSON 
over plain output.

> +
> +        res_default = bpftool(["feature", "probe", "full"])
> +        res = bpftool(["feature", "probe", "kernel", "full"])
> +
> +        for pattern in expected_patterns:
> +            self.assertIn(pattern, res_default)
> +            self.assertIn(pattern, res)
> +
> +    def test_feature_kernel_json(self):
> +        expected_keys = [
> +            "system_config",
> +            "syscall_config",
> +            "program_types",
> +            "map_types",
> +            "helpers",
> +            "misc",
> +        ]
> +        unexpected_values = [
> +            "bpf_trace_printk",
> +            "bpf_probe_write_user",
> +        ]
> +
> +        res_default1 = bpftool_json(["feature"])
> +        self.assertCountEqual(res_default1.keys(), expected_keys)
> +        for value in unexpected_values:
> +            self._assert_pattern_not_in_dict(res_default1, value)
> +
> +        res_default2 = bpftool_json(["feature", "probe"])
> +        self.assertCountEqual(res_default2.keys(), expected_keys)
> +        for value in unexpected_values:
> +            self._assert_pattern_not_in_dict(res_default2, value)
> +
> +        res = bpftool_json(["feature", "probe", "kernel"])
> +        self.assertCountEqual(res.keys(), expected_keys)
> +        for value in unexpected_values:
> +            self._assert_pattern_not_in_dict(res, value)
> +
> +    def test_feature_macros(self):
> +        expected_patterns = [
> +            b"/\*\*\* System call availability \*\*\*/",
> +            b"#define HAVE_BPF_SYSCALL",
> +            b"/\*\*\* eBPF program types \*\*\*/",
> +            b"#define HAVE.*PROG_TYPE",
> +            b"/\*\*\* eBPF map types \*\*\*/",
> +            b"#define HAVE.*MAP_TYPE",
> +            b"/\*\*\* eBPF helper functions \*\*\*/",
> +            b"#define HAVE.*HELPER",
> +            b"/\*\*\* eBPF misc features \*\*\*/",
> +        ]
> +
> +        res = bpftool(["feature", "probe", "macros"])
> +        for pattern in expected_patterns:
> +            self.assertRegex(res, pattern)

Could we have (or did I miss it?) a test that compares the output of 
probes _with_ "full" and _without_ it, to make sure that the only lines 
that differ are about "bpf_trace_prink" or "bpf_probe_write_user"? Could 
help determine if we filter out too many elements by mistake.

Thanks,
Quentin

> diff --git a/tools/testing/selftests/bpf/test_bpftool.sh b/tools/testing/selftests/bpf/test_bpftool.sh
> new file mode 100755
> index 000000000000..66690778e36d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_bpftool.sh
> @@ -0,0 +1,5 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 SUSE LLC.
> +
> +python3 -m unittest -v test_bpftool.TestBpftool
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 4/5] bpftool: Update bash completion for "bpftool feature" command
  2020-02-21  3:16 ` [PATCH bpf-next v2 4/5] bpftool: Update bash completion for " Michal Rostecki
@ 2020-02-21 11:29   ` Quentin Monnet
  0 siblings, 0 replies; 16+ messages in thread
From: Quentin Monnet @ 2020-02-21 11:29 UTC (permalink / raw)
  To: Michal Rostecki, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Jakub Kicinski, netdev,
	linux-kernel, Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK

2020-02-21 04:16 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org>
> Update bash completion for "bpftool feature" command with the new
> argument: "full".
> 
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> ---
>   tools/bpf/bpftool/bash-completion/bpftool | 27 ++++++++++++++++-------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 754d8395e451..f2bcc4bacee2 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -981,14 +981,25 @@ _bpftool()
>           feature)
>               case $command in
>                   probe)
> -                    [[ $prev == "prefix" ]] && return 0
> -                    if _bpftool_search_list 'macros'; then
> -                        COMPREPLY+=( $( compgen -W 'prefix' -- "$cur" ) )
> -                    else
> -                        COMPREPLY+=( $( compgen -W 'macros' -- "$cur" ) )
> -                    fi
> -                    _bpftool_one_of_list 'kernel dev'
> -                    return 0
> +                    case $prev in
> +                        $command)
> +                            COMPREPLY+=( $( compgen -W 'kernel dev full macros' -- \
> +                                "$cur" ) )
> +                            return 0
> +                            ;;
> +                        prefix)
> +                            return 0
> +                            ;;
> +                        macros)
> +                            COMPREPLY+=( $( compgen -W 'prefix' -- "$cur" ) )
> +                            return 0

I have not tested, but I think because of the "return 0" this will 
propose only "prefix" after "macros". But "kernel" or "dev" should also 
be in the list.

Maybe just add "_bpftool_once_attr 'full'" under the 
"_bpftool_one_of_list 'kernel dev'" instead of changing to the "case 
$prev in" structure?

> +                            ;;
> +                        *)
> +                            _bpftool_one_of_list 'kernel dev'
> +                            _bpftool_once_attr 'full macros'
> +                            return 0
> +                            ;;
> +                    esac
>                       ;;
>                   *)
>                       [[ $prev == $object ]] && \
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional
  2020-02-21 11:28   ` Quentin Monnet
@ 2020-02-21 22:44     ` Alexei Starovoitov
  2020-02-25 12:18       ` Michal Rostecki
  0 siblings, 1 reply; 16+ messages in thread
From: Alexei Starovoitov @ 2020-02-21 22:44 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Michal Rostecki, bpf, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Jakub Kicinski, netdev, linux-kernel, Shuah Khan,
	open list:KERNEL SELFTEST FRAMEWORK

On Fri, Feb 21, 2020 at 11:28:05AM +0000, Quentin Monnet wrote:
> 
> "trace" sounds too generic. If filters are applied again to prog and map
> types in the future (as you had in v1), this would catch tracepoint and
> raw_tracepoint program types and stack_trace map type. Or if new helpers
> with "trace" in their name are added, we skip them too. Can we use something
> more specific, probably "trace_printk"?

+1

> Thanks for the patch! While I understand you want to keep the changes you
> have done to use regex, I do not really think they bring much in this
> version of the patch. As we only want to filter out two specific helpers, it
> seems to me that it would be much simpler to just compare helper names
> instead of introducing regular expressions that are not used otherwise. What
> do you think?

+1
I was thinking the same.
Or filter by specific integer id of the helper.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional
  2020-02-21 22:44     ` Alexei Starovoitov
@ 2020-02-25 12:18       ` Michal Rostecki
  0 siblings, 0 replies; 16+ messages in thread
From: Michal Rostecki @ 2020-02-25 12:18 UTC (permalink / raw)
  To: Alexei Starovoitov, Quentin Monnet
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrii Nakryiko, Jakub Kicinski, netdev,
	linux-kernel, Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK

On 2/21/20 11:44 PM, Alexei Starovoitov wrote:
> On Fri, Feb 21, 2020 at 11:28:05AM +0000, Quentin Monnet wrote:
>>
>> "trace" sounds too generic. If filters are applied again to prog and map
>> types in the future (as you had in v1), this would catch tracepoint and
>> raw_tracepoint program types and stack_trace map type. Or if new helpers
>> with "trace" in their name are added, we skip them too. Can we use something
>> more specific, probably "trace_printk"?
> 
> +1
> 
>> Thanks for the patch! While I understand you want to keep the changes you
>> have done to use regex, I do not really think they bring much in this
>> version of the patch. As we only want to filter out two specific helpers, it
>> seems to me that it would be much simpler to just compare helper names
>> instead of introducing regular expressions that are not used otherwise. What
>> do you think?
> 
> +1
> I was thinking the same.
> Or filter by specific integer id of the helper.
> 

I like the idea of filtering by id. I will do that in v3. Thanks for review!

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add test for "bpftool feature" command
  2020-02-21 11:28   ` Quentin Monnet
@ 2020-02-25 13:25     ` Michal Rostecki
  2020-02-25 13:55     ` Michal Rostecki
  1 sibling, 0 replies; 16+ messages in thread
From: Michal Rostecki @ 2020-02-25 13:25 UTC (permalink / raw)
  To: Quentin Monnet, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Quentin Monnet, Jakub Kicinski,
	netdev, linux-kernel, Shuah Khan, David S. Miller,
	Jesper Dangaard Brouer, John Fastabend,
	open list:KERNEL SELFTEST FRAMEWORK

On 2/21/20 12:28 PM, Quentin Monnet wrote:>> +    def
test_feature_macros(self):
>> +        expected_patterns = [
>> +            b"/\*\*\* System call availability \*\*\*/",
>> +            b"#define HAVE_BPF_SYSCALL",
>> +            b"/\*\*\* eBPF program types \*\*\*/",
>> +            b"#define HAVE.*PROG_TYPE",
>> +            b"/\*\*\* eBPF map types \*\*\*/",
>> +            b"#define HAVE.*MAP_TYPE",
>> +            b"/\*\*\* eBPF helper functions \*\*\*/",
>> +            b"#define HAVE.*HELPER",
>> +            b"/\*\*\* eBPF misc features \*\*\*/",
>> +        ]
>> +
>> +        res = bpftool(["feature", "probe", "macros"])
>> +        for pattern in expected_patterns:
>> +            self.assertRegex(res, pattern)
> 
> Could we have (or did I miss it?) a test that compares the output of
> probes _with_ "full" and _without_ it, to make sure that the only lines
> that differ are about "bpf_trace_prink" or "bpf_probe_write_user"? Could
> help determine if we filter out too many elements by mistake.
> 
> Thanks,
> Quentin

Good idea, I will add that test in v3.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add test for "bpftool feature" command
  2020-02-21 11:28   ` Quentin Monnet
  2020-02-25 13:25     ` Michal Rostecki
@ 2020-02-25 13:55     ` Michal Rostecki
  2020-02-25 14:54       ` Quentin Monnet
  1 sibling, 1 reply; 16+ messages in thread
From: Michal Rostecki @ 2020-02-25 13:55 UTC (permalink / raw)
  To: Quentin Monnet, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Quentin Monnet, Jakub Kicinski,
	netdev, linux-kernel, Shuah Khan, David S. Miller,
	Jesper Dangaard Brouer, John Fastabend,
	open list:KERNEL SELFTEST FRAMEWORK

On 2/21/20 12:28 PM, Quentin Monnet wrote:
>> +    @default_iface
>> +    def test_feature_dev(self, iface):
>> +        expected_patterns = [
>> +            SECTION_SYSCALL_CONFIG_PATTERN,
>> +            SECTION_PROGRAM_TYPES_PATTERN,
>> +            SECTION_MAP_TYPES_PATTERN,
>> +            SECTION_HELPERS_PATTERN,
>> +            SECTION_MISC_PATTERN,
>> +        ]
> 
> Mixed feeling on the tests with plain output, as we keep telling people
> that plain output should not be parsed (not reliable, may change). But
> if you want to run one or two tests with it, why not, I guess.

I thought about that and yes, testing the plain output is probably
redundant and makes those tests less readable. However, the only plain
output test which I would like to keep there is test_feature_macros -
because I guess that we are not planning to change names or patterns of
generated macros (or if so, we should test that change).

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add test for "bpftool feature" command
  2020-02-25 13:55     ` Michal Rostecki
@ 2020-02-25 14:54       ` Quentin Monnet
  0 siblings, 0 replies; 16+ messages in thread
From: Quentin Monnet @ 2020-02-25 14:54 UTC (permalink / raw)
  To: Michal Rostecki, bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, Jakub Kicinski, netdev,
	linux-kernel, Shuah Khan, David S. Miller,
	Jesper Dangaard Brouer, John Fastabend,
	open list:KERNEL SELFTEST FRAMEWORK

2020-02-25 14:55 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org>
> On 2/21/20 12:28 PM, Quentin Monnet wrote:
>>> +    @default_iface
>>> +    def test_feature_dev(self, iface):
>>> +        expected_patterns = [
>>> +            SECTION_SYSCALL_CONFIG_PATTERN,
>>> +            SECTION_PROGRAM_TYPES_PATTERN,
>>> +            SECTION_MAP_TYPES_PATTERN,
>>> +            SECTION_HELPERS_PATTERN,
>>> +            SECTION_MISC_PATTERN,
>>> +        ]
>>
>> Mixed feeling on the tests with plain output, as we keep telling people
>> that plain output should not be parsed (not reliable, may change). But
>> if you want to run one or two tests with it, why not, I guess.
> 
> I thought about that and yes, testing the plain output is probably
> redundant and makes those tests less readable. However, the only plain
> output test which I would like to keep there is test_feature_macros -
> because I guess that we are not planning to change names or patterns of
> generated macros (or if so, we should test that change).
> 

I did not mentally include the header/macros output in “plain output”, 
but yeah I guess I was not explicit on this one. So: Agreed, with 
“macros” it should not change and it is welcome in the tests, feel free 
to keep it :)

Quentin

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-02-25 14:54 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-21  3:16 [PATCH bpf-next v2 0/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
2020-02-21  3:16 ` [PATCH bpf-next v2 1/5] bpftool: Move out sections to separate functions Michal Rostecki
2020-02-21 11:27   ` Quentin Monnet
2020-02-21  3:16 ` [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg warnings optional Michal Rostecki
2020-02-21 11:28   ` Quentin Monnet
2020-02-21 22:44     ` Alexei Starovoitov
2020-02-25 12:18       ` Michal Rostecki
2020-02-21  3:16 ` [PATCH bpf-next v2 3/5] bpftool: Update documentation of "bpftool feature" command Michal Rostecki
2020-02-21 11:28   ` Quentin Monnet
2020-02-21  3:16 ` [PATCH bpf-next v2 4/5] bpftool: Update bash completion for " Michal Rostecki
2020-02-21 11:29   ` Quentin Monnet
2020-02-21  3:17 ` [PATCH bpf-next v2 5/5] selftests/bpf: Add test " Michal Rostecki
2020-02-21 11:28   ` Quentin Monnet
2020-02-25 13:25     ` Michal Rostecki
2020-02-25 13:55     ` Michal Rostecki
2020-02-25 14:54       ` Quentin Monnet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).