bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] bpf: fix compilation warning of selftests
@ 2020-07-31  6:16 Jianlin Lv
  2020-07-31 15:00 ` Daniel Borkmann
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Jianlin Lv @ 2020-07-31  6:16 UTC (permalink / raw)
  To: bpf
  Cc: davem, kuba, ast, daniel, yhs, Song.Zhu, Jianlin.Lv,
	linux-kernel, netdev

Clang compiler version: 12.0.0
The following warning appears during the selftests/bpf compilation:

prog_tests/send_signal.c:51:3: warning: ignoring return value of ‘write’,
declared with attribute warn_unused_result [-Wunused-result]
   51 |   write(pipe_c2p[1], buf, 1);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
prog_tests/send_signal.c:54:3: warning: ignoring return value of ‘read’,
declared with attribute warn_unused_result [-Wunused-result]
   54 |   read(pipe_p2c[0], buf, 1);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~
......

prog_tests/stacktrace_build_id_nmi.c:13:2: warning: ignoring return value
of ‘fscanf’,declared with attribute warn_unused_result [-Wunused-resul]
   13 |  fscanf(f, "%llu", &sample_freq);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test_tcpnotify_user.c:133:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  133 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~
test_tcpnotify_user.c:138:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  138 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~
test_tcpnotify_user.c:143:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  143 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~

Add code that fix compilation warning about ignoring return value and
handles any errors; Check return value of library`s API make the code
more secure.

Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
---
 .../selftests/bpf/prog_tests/send_signal.c    | 37 ++++++++++++++-----
 .../bpf/prog_tests/stacktrace_build_id_nmi.c  |  3 +-
 .../selftests/bpf/test_tcpnotify_user.c       | 15 ++++++--
 3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 504abb7bfb95..7a5272e4e810 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -48,22 +48,31 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 		close(pipe_p2c[1]); /* close write */
 
 		/* notify parent signal handler is installed */
-		write(pipe_c2p[1], buf, 1);
+		if (CHECK_FAIL(write(pipe_c2p[1], buf, 1) != 1)) {
+			perror("Child: write pipe error");
+			goto close_out;
+		}
 
 		/* make sure parent enabled bpf program to send_signal */
-		read(pipe_p2c[0], buf, 1);
+		if (CHECK_FAIL(read(pipe_p2c[0], buf, 1) != 1)) {
+			perror("Child: read pipe error");
+			goto close_out;
+		}
 
 		/* wait a little for signal handler */
 		sleep(1);
 
-		if (sigusr1_received)
-			write(pipe_c2p[1], "2", 1);
-		else
-			write(pipe_c2p[1], "0", 1);
+		buf[0] = sigusr1_received ? '2' : '0';
+		if (CHECK_FAIL(write(pipe_c2p[1], buf, 1) != 1)) {
+			perror("Child: write pipe error");
+			goto close_out;
+		}
 
 		/* wait for parent notification and exit */
-		read(pipe_p2c[0], buf, 1);
+		if (CHECK_FAIL(read(pipe_p2c[0], buf, 1) != 1))
+			perror("Child: read pipe error");
 
+close_out:
 		close(pipe_c2p[1]);
 		close(pipe_p2c[0]);
 		exit(0);
@@ -99,7 +108,11 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	}
 
 	/* wait until child signal handler installed */
-	read(pipe_c2p[0], buf, 1);
+	if (CHECK_FAIL(read(pipe_c2p[0], buf, 1) != 1)) {
+		perror("Parent: read pipe error");
+		goto disable_pmu;
+	}
+
 
 	/* trigger the bpf send_signal */
 	skel->bss->pid = pid;
@@ -107,7 +120,10 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	skel->bss->signal_thread = signal_thread;
 
 	/* notify child that bpf program can send_signal now */
-	write(pipe_p2c[1], buf, 1);
+	if (CHECK_FAIL(write(pipe_p2c[1], buf, 1) != 1)) {
+		perror("Parent: write pipe error");
+		goto disable_pmu;
+	}
 
 	/* wait for result */
 	err = read(pipe_c2p[0], buf, 1);
@@ -121,7 +137,8 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	CHECK(buf[0] != '2', test_name, "incorrect result\n");
 
 	/* notify child safe to exit */
-	write(pipe_p2c[1], buf, 1);
+	if (CHECK_FAIL(write(pipe_p2c[1], buf, 1) != 1))
+		perror("Parent: write pipe error");
 
 disable_pmu:
 	close(pmu_fd);
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index f002e3090d92..a27de3d46e58 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -10,7 +10,8 @@ static __u64 read_perf_max_sample_freq(void)
 	f = fopen("/proc/sys/kernel/perf_event_max_sample_rate", "r");
 	if (f == NULL)
 		return sample_freq;
-	fscanf(f, "%llu", &sample_freq);
+	if (CHECK_FAIL(fscanf(f, "%llu", &sample_freq) != 1))
+		perror("Get max sample rate fail, return default value: 5000\n");
 	fclose(f);
 	return sample_freq;
 }
diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
index f9765ddf0761..869e28c92d73 100644
--- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
+++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
@@ -130,17 +130,26 @@ int main(int argc, char **argv)
 	sprintf(test_script,
 		"iptables -A INPUT -p tcp --dport %d -j DROP",
 		TESTPORT);
-	system(test_script);
+	if (system(test_script)) {
+		printf("FAILED: execute command: %s\n", test_script);
+		goto err;
+	}
 
 	sprintf(test_script,
 		"nc 127.0.0.1 %d < /etc/passwd > /dev/null 2>&1 ",
 		TESTPORT);
-	system(test_script);
+	if (system(test_script)) {
+		printf("FAILED: execute command: %s\n", test_script);
+		goto err;
+	}
 
 	sprintf(test_script,
 		"iptables -D INPUT -p tcp --dport %d -j DROP",
 		TESTPORT);
-	system(test_script);
+	if (system(test_script)) {
+		printf("FAILED: execute command: %s\n", test_script);
+		goto err;
+	}
 
 	rv = bpf_map_lookup_elem(bpf_map__fd(global_map), &key, &g);
 	if (rv != 0) {
-- 
2.17.1


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

* Re: [PATCH bpf-next] bpf: fix compilation warning of selftests
  2020-07-31  6:16 [PATCH bpf-next] bpf: fix compilation warning of selftests Jianlin Lv
@ 2020-07-31 15:00 ` Daniel Borkmann
  2020-07-31 17:39 ` Andrii Nakryiko
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2020-07-31 15:00 UTC (permalink / raw)
  To: Jianlin Lv, bpf; +Cc: davem, kuba, ast, yhs, Song.Zhu, linux-kernel, netdev

On 7/31/20 8:16 AM, Jianlin Lv wrote:
> Clang compiler version: 12.0.0
> The following warning appears during the selftests/bpf compilation:
> 
> prog_tests/send_signal.c:51:3: warning: ignoring return value of ‘write’,
> declared with attribute warn_unused_result [-Wunused-result]
>     51 |   write(pipe_c2p[1], buf, 1);
>        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
> prog_tests/send_signal.c:54:3: warning: ignoring return value of ‘read’,
> declared with attribute warn_unused_result [-Wunused-result]
>     54 |   read(pipe_p2c[0], buf, 1);
>        |   ^~~~~~~~~~~~~~~~~~~~~~~~~
> ......
> 
> prog_tests/stacktrace_build_id_nmi.c:13:2: warning: ignoring return value
> of ‘fscanf’,declared with attribute warn_unused_result [-Wunused-resul]
>     13 |  fscanf(f, "%llu", &sample_freq);
>        |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> test_tcpnotify_user.c:133:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>    133 |  system(test_script);
>        |  ^~~~~~~~~~~~~~~~~~~
> test_tcpnotify_user.c:138:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>    138 |  system(test_script);
>        |  ^~~~~~~~~~~~~~~~~~~
> test_tcpnotify_user.c:143:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>    143 |  system(test_script);
>        |  ^~~~~~~~~~~~~~~~~~~
> 
> Add code that fix compilation warning about ignoring return value and
> handles any errors; Check return value of library`s API make the code
> more secure.
> 
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>

Looks good overall, there is one small bug that slipped in, see below:

[...]
> diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> index f9765ddf0761..869e28c92d73 100644
> --- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
> +++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> @@ -130,17 +130,26 @@ int main(int argc, char **argv)
>   	sprintf(test_script,
>   		"iptables -A INPUT -p tcp --dport %d -j DROP",
>   		TESTPORT);
> -	system(test_script);
> +	if (system(test_script)) {
> +		printf("FAILED: execute command: %s\n", test_script);
> +		goto err;
> +	}
>   
>   	sprintf(test_script,
>   		"nc 127.0.0.1 %d < /etc/passwd > /dev/null 2>&1 ",
>   		TESTPORT);
> -	system(test_script);
> +	if (system(test_script)) {
> +		printf("FAILED: execute command: %s\n", test_script);
> +		goto err;
> +	}

Did you try to run this test case? With the patch here it will fail:

   # ./test_tcpnotify_user
   FAILED: execute command: nc 127.0.0.1 12877 < /etc/passwd > /dev/null 2>&1

This is because nc returns 1 as exit code and for the test it is actually expected
to fail given the iptables rule we installed for TESTPORT right above and remove
again below.

Please adapt this and send a v2, thanks!

>   	sprintf(test_script,
>   		"iptables -D INPUT -p tcp --dport %d -j DROP",
>   		TESTPORT);
> -	system(test_script);
> +	if (system(test_script)) {
> +		printf("FAILED: execute command: %s\n", test_script);
> +		goto err;
> +	}
>   
>   	rv = bpf_map_lookup_elem(bpf_map__fd(global_map), &key, &g);
>   	if (rv != 0) {
> 


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

* Re: [PATCH bpf-next] bpf: fix compilation warning of selftests
  2020-07-31  6:16 [PATCH bpf-next] bpf: fix compilation warning of selftests Jianlin Lv
  2020-07-31 15:00 ` Daniel Borkmann
@ 2020-07-31 17:39 ` Andrii Nakryiko
  2020-08-06 10:42 ` [PATCH bpf-next v2] " Jianlin Lv
  2020-08-07 17:20 ` [PATCH bpf-next] bpf: fix segmentation fault of test_progs Jianlin Lv
  3 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2020-07-31 17:39 UTC (permalink / raw)
  To: Jianlin Lv
  Cc: bpf, David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Yonghong Song, Song.Zhu, open list, Networking

On Thu, Jul 30, 2020 at 11:18 PM Jianlin Lv <Jianlin.Lv@arm.com> wrote:
>
> Clang compiler version: 12.0.0
> The following warning appears during the selftests/bpf compilation:
>
> prog_tests/send_signal.c:51:3: warning: ignoring return value of ‘write’,
> declared with attribute warn_unused_result [-Wunused-result]
>    51 |   write(pipe_c2p[1], buf, 1);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
> prog_tests/send_signal.c:54:3: warning: ignoring return value of ‘read’,
> declared with attribute warn_unused_result [-Wunused-result]
>    54 |   read(pipe_p2c[0], buf, 1);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~
> ......
>
> prog_tests/stacktrace_build_id_nmi.c:13:2: warning: ignoring return value
> of ‘fscanf’,declared with attribute warn_unused_result [-Wunused-resul]
>    13 |  fscanf(f, "%llu", &sample_freq);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> test_tcpnotify_user.c:133:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>   133 |  system(test_script);
>       |  ^~~~~~~~~~~~~~~~~~~
> test_tcpnotify_user.c:138:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>   138 |  system(test_script);
>       |  ^~~~~~~~~~~~~~~~~~~
> test_tcpnotify_user.c:143:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>   143 |  system(test_script);
>       |  ^~~~~~~~~~~~~~~~~~~
>
> Add code that fix compilation warning about ignoring return value and
> handles any errors; Check return value of library`s API make the code
> more secure.
>
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
> ---
>  .../selftests/bpf/prog_tests/send_signal.c    | 37 ++++++++++++++-----
>  .../bpf/prog_tests/stacktrace_build_id_nmi.c  |  3 +-
>  .../selftests/bpf/test_tcpnotify_user.c       | 15 ++++++--
>  3 files changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 504abb7bfb95..7a5272e4e810 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -48,22 +48,31 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>                 close(pipe_p2c[1]); /* close write */
>
>                 /* notify parent signal handler is installed */
> -               write(pipe_c2p[1], buf, 1);
> +               if (CHECK_FAIL(write(pipe_c2p[1], buf, 1) != 1)) {
> +                       perror("Child: write pipe error");
> +                       goto close_out;
> +               }

Please don't use CHECK_FAIL. Using CHECK is better for many reasons,
but it will also be shorter here (while still recording failure):


CHECK(write(pipe_c2p[1], buf, 1) != 1, "pipe_write", "err %d\n", -errno);


>
>                 /* make sure parent enabled bpf program to send_signal */
> -               read(pipe_p2c[0], buf, 1);
> +               if (CHECK_FAIL(read(pipe_p2c[0], buf, 1) != 1)) {
> +                       perror("Child: read pipe error");
> +                       goto close_out;
> +               }
>

[...]

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

* [PATCH bpf-next v2] bpf: fix compilation warning of selftests
  2020-07-31  6:16 [PATCH bpf-next] bpf: fix compilation warning of selftests Jianlin Lv
  2020-07-31 15:00 ` Daniel Borkmann
  2020-07-31 17:39 ` Andrii Nakryiko
@ 2020-08-06 10:42 ` Jianlin Lv
  2020-08-07  0:05   ` Alexei Starovoitov
  2020-08-07 17:20 ` [PATCH bpf-next] bpf: fix segmentation fault of test_progs Jianlin Lv
  3 siblings, 1 reply; 10+ messages in thread
From: Jianlin Lv @ 2020-08-06 10:42 UTC (permalink / raw)
  To: bpf
  Cc: davem, kuba, ast, daniel, yhs, Song.Zhu, Jianlin.Lv,
	linux-kernel, netdev

Clang compiler version: 12.0.0
The following warning appears during the selftests/bpf compilation:

prog_tests/send_signal.c:51:3: warning: ignoring return value of ‘write’,
declared with attribute warn_unused_result [-Wunused-result]
   51 |   write(pipe_c2p[1], buf, 1);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
prog_tests/send_signal.c:54:3: warning: ignoring return value of ‘read’,
declared with attribute warn_unused_result [-Wunused-result]
   54 |   read(pipe_p2c[0], buf, 1);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~
......

prog_tests/stacktrace_build_id_nmi.c:13:2: warning: ignoring return value
of ‘fscanf’,declared with attribute warn_unused_result [-Wunused-resul]
   13 |  fscanf(f, "%llu", &sample_freq);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test_tcpnotify_user.c:133:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  133 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~
test_tcpnotify_user.c:138:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  138 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~
test_tcpnotify_user.c:143:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  143 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~

Add code that fix compilation warning about ignoring return value and
handles any errors; Check return value of library`s API make the code
more secure.

Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
---
v2:
- replease CHECK_FAIL by CHECK
- fix test_tcpnotify_user failed issue
---
 .../selftests/bpf/prog_tests/send_signal.c     | 18 ++++++++----------
 .../bpf/prog_tests/stacktrace_build_id_nmi.c   |  4 +++-
 .../selftests/bpf/test_tcpnotify_user.c        | 13 ++++++++++---
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 504abb7bfb95..7043e6ded0e6 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -48,21 +48,19 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 		close(pipe_p2c[1]); /* close write */
 
 		/* notify parent signal handler is installed */
-		write(pipe_c2p[1], buf, 1);
+		CHECK(write(pipe_c2p[1], buf, 1) != 1, "pipe_write", "err %d\n", -errno);
 
 		/* make sure parent enabled bpf program to send_signal */
-		read(pipe_p2c[0], buf, 1);
+		CHECK(read(pipe_p2c[0], buf, 1) != 1, "pipe_read", "err %d\n", -errno);
 
 		/* wait a little for signal handler */
 		sleep(1);
 
-		if (sigusr1_received)
-			write(pipe_c2p[1], "2", 1);
-		else
-			write(pipe_c2p[1], "0", 1);
+		buf[0] = sigusr1_received ? '2' : '0';
+		CHECK(write(pipe_c2p[1], buf, 1) != 1, "pipe_write", "err %d\n", -errno);
 
 		/* wait for parent notification and exit */
-		read(pipe_p2c[0], buf, 1);
+		CHECK(read(pipe_p2c[0], buf, 1) != 1, "pipe_read", "err %d\n", -errno);
 
 		close(pipe_c2p[1]);
 		close(pipe_p2c[0]);
@@ -99,7 +97,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	}
 
 	/* wait until child signal handler installed */
-	read(pipe_c2p[0], buf, 1);
+	CHECK(read(pipe_c2p[0], buf, 1) != 1, "pipe_read", "err %d\n", -errno);
 
 	/* trigger the bpf send_signal */
 	skel->bss->pid = pid;
@@ -107,7 +105,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	skel->bss->signal_thread = signal_thread;
 
 	/* notify child that bpf program can send_signal now */
-	write(pipe_p2c[1], buf, 1);
+	CHECK(write(pipe_p2c[1], buf, 1) != 1, "pipe_write", "err %d\n", -errno);
 
 	/* wait for result */
 	err = read(pipe_c2p[0], buf, 1);
@@ -121,7 +119,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	CHECK(buf[0] != '2', test_name, "incorrect result\n");
 
 	/* notify child safe to exit */
-	write(pipe_p2c[1], buf, 1);
+	CHECK(write(pipe_p2c[1], buf, 1) != 1, "pipe_write", "err %d\n", -errno);
 
 disable_pmu:
 	close(pmu_fd);
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index f002e3090d92..11a769e18f5d 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -6,11 +6,13 @@ static __u64 read_perf_max_sample_freq(void)
 {
 	__u64 sample_freq = 5000; /* fallback to 5000 on error */
 	FILE *f;
+	__u32 duration = 0;
 
 	f = fopen("/proc/sys/kernel/perf_event_max_sample_rate", "r");
 	if (f == NULL)
 		return sample_freq;
-	fscanf(f, "%llu", &sample_freq);
+	CHECK(fscanf(f, "%llu", &sample_freq) != 1, "Get max sample rate",
+		  "return default value: 5000,err %d\n", -errno);
 	fclose(f);
 	return sample_freq;
 }
diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
index 8549b31716ab..73da7fe8c152 100644
--- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
+++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
@@ -124,17 +124,24 @@ int main(int argc, char **argv)
 	sprintf(test_script,
 		"iptables -A INPUT -p tcp --dport %d -j DROP",
 		TESTPORT);
-	system(test_script);
+	if (system(test_script)) {
+		printf("FAILED: execute command: %s, err %d\n", test_script, -errno);
+		goto err;
+	}
 
 	sprintf(test_script,
 		"nc 127.0.0.1 %d < /etc/passwd > /dev/null 2>&1 ",
 		TESTPORT);
-	system(test_script);
+	if (system(test_script))
+		printf("execute command: %s, err %d\n", test_script, -errno);
 
 	sprintf(test_script,
 		"iptables -D INPUT -p tcp --dport %d -j DROP",
 		TESTPORT);
-	system(test_script);
+	if (system(test_script)) {
+		printf("FAILED: execute command: %s, err %d\n", test_script, -errno);
+		goto err;
+	}
 
 	rv = bpf_map_lookup_elem(bpf_map__fd(global_map), &key, &g);
 	if (rv != 0) {
-- 
2.17.1


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

* Re: [PATCH bpf-next v2] bpf: fix compilation warning of selftests
  2020-08-06 10:42 ` [PATCH bpf-next v2] " Jianlin Lv
@ 2020-08-07  0:05   ` Alexei Starovoitov
  0 siblings, 0 replies; 10+ messages in thread
From: Alexei Starovoitov @ 2020-08-07  0:05 UTC (permalink / raw)
  To: Jianlin Lv
  Cc: bpf, David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Yonghong Song, Song.Zhu, LKML,
	Network Development

On Thu, Aug 6, 2020 at 3:46 AM Jianlin Lv <Jianlin.Lv@arm.com> wrote:
>
> Clang compiler version: 12.0.0
> The following warning appears during the selftests/bpf compilation:
>
> prog_tests/send_signal.c:51:3: warning: ignoring return value of ‘write’,
> declared with attribute warn_unused_result [-Wunused-result]
>    51 |   write(pipe_c2p[1], buf, 1);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
> prog_tests/send_signal.c:54:3: warning: ignoring return value of ‘read’,
> declared with attribute warn_unused_result [-Wunused-result]
>    54 |   read(pipe_p2c[0], buf, 1);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~
> ......
>
> prog_tests/stacktrace_build_id_nmi.c:13:2: warning: ignoring return value
> of ‘fscanf’,declared with attribute warn_unused_result [-Wunused-resul]
>    13 |  fscanf(f, "%llu", &sample_freq);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> test_tcpnotify_user.c:133:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>   133 |  system(test_script);
>       |  ^~~~~~~~~~~~~~~~~~~
> test_tcpnotify_user.c:138:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>   138 |  system(test_script);
>       |  ^~~~~~~~~~~~~~~~~~~
> test_tcpnotify_user.c:143:2: warning:ignoring return value of ‘system’,
> declared with attribute warn_unused_result [-Wunused-result]
>   143 |  system(test_script);
>       |  ^~~~~~~~~~~~~~~~~~~
>
> Add code that fix compilation warning about ignoring return value and
> handles any errors; Check return value of library`s API make the code
> more secure.
>
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
> ---
> v2:
> - replease CHECK_FAIL by CHECK
> - fix test_tcpnotify_user failed issue

Applied. Thanks

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

* [PATCH bpf-next] bpf: fix segmentation fault of test_progs
  2020-07-31  6:16 [PATCH bpf-next] bpf: fix compilation warning of selftests Jianlin Lv
                   ` (2 preceding siblings ...)
  2020-08-06 10:42 ` [PATCH bpf-next v2] " Jianlin Lv
@ 2020-08-07 17:20 ` Jianlin Lv
  2020-08-07 20:13   ` Andrii Nakryiko
  2020-08-10 15:39   ` [PATCH bpf-next v2] " Jianlin Lv
  3 siblings, 2 replies; 10+ messages in thread
From: Jianlin Lv @ 2020-08-07 17:20 UTC (permalink / raw)
  To: bpf; +Cc: davem, kuba, ast, daniel, yhs, Jianlin.Lv, linux-kernel, netdev

test_progs reports the segmentation fault as below

$ sudo ./test_progs -t mmap --verbose
test_mmap:PASS:skel_open_and_load 0 nsec
......
test_mmap:PASS:adv_mmap1 0 nsec
test_mmap:PASS:adv_mmap2 0 nsec
test_mmap:PASS:adv_mmap3 0 nsec
test_mmap:PASS:adv_mmap4 0 nsec
Segmentation fault

This issue was triggered because mmap() and munmap() used inconsistent
length parameters; mmap() creates a new mapping of 3*page_size, but the
length parameter set in the subsequent re-map and munmap() functions is
4*page_size; this leads to the destruction of the process space.

Another issue is that when unmap the second page fails, the length
parameter to delete tmp1 mappings should be 3*page_size.

Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
---
 tools/testing/selftests/bpf/prog_tests/mmap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mmap.c b/tools/testing/selftests/bpf/prog_tests/mmap.c
index 43d0b5578f46..2070cfe19cac 100644
--- a/tools/testing/selftests/bpf/prog_tests/mmap.c
+++ b/tools/testing/selftests/bpf/prog_tests/mmap.c
@@ -192,7 +192,7 @@ void test_mmap(void)
 	/* unmap second page: pages 1, 3 mapped */
 	err = munmap(tmp1 + page_size, page_size);
 	if (CHECK(err, "adv_mmap2", "errno %d\n", errno)) {
-		munmap(tmp1, map_sz);
+		munmap(tmp1, 3 * page_size);
 		goto cleanup;
 	}
 
@@ -207,8 +207,8 @@ void test_mmap(void)
 	CHECK(tmp1 + page_size != tmp2, "adv_mmap4",
 	      "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
 
-	/* re-map all 4 pages */
-	tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
+	/* re-map all 3 pages */
+	tmp2 = mmap(tmp1, 3 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
 		    data_map_fd, 0);
 	if (CHECK(tmp2 == MAP_FAILED, "adv_mmap5", "errno %d\n", errno)) {
 		munmap(tmp1, 3 * page_size); /* unmap page 1 */
@@ -226,7 +226,7 @@ void test_mmap(void)
 	CHECK_FAIL(map_data->val[2] != 321);
 	CHECK_FAIL(map_data->val[far] != 3 * 321);
 
-	munmap(tmp2, 4 * page_size);
+	munmap(tmp2, 3 * page_size);
 
 	/* map all 4 pages, but with pg_off=1 page, should fail */
 	tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
-- 
2.17.1


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

* Re: [PATCH bpf-next] bpf: fix segmentation fault of test_progs
  2020-08-07 17:20 ` [PATCH bpf-next] bpf: fix segmentation fault of test_progs Jianlin Lv
@ 2020-08-07 20:13   ` Andrii Nakryiko
  2020-08-10 15:39   ` [PATCH bpf-next v2] " Jianlin Lv
  1 sibling, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2020-08-07 20:13 UTC (permalink / raw)
  To: Jianlin Lv
  Cc: bpf, David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Yonghong Song, open list, Networking

On Fri, Aug 7, 2020 at 10:21 AM Jianlin Lv <Jianlin.Lv@arm.com> wrote:
>
> test_progs reports the segmentation fault as below
>
> $ sudo ./test_progs -t mmap --verbose
> test_mmap:PASS:skel_open_and_load 0 nsec
> ......
> test_mmap:PASS:adv_mmap1 0 nsec
> test_mmap:PASS:adv_mmap2 0 nsec
> test_mmap:PASS:adv_mmap3 0 nsec
> test_mmap:PASS:adv_mmap4 0 nsec
> Segmentation fault
>
> This issue was triggered because mmap() and munmap() used inconsistent
> length parameters; mmap() creates a new mapping of 3*page_size, but the
> length parameter set in the subsequent re-map and munmap() functions is
> 4*page_size; this leads to the destruction of the process space.
>
> Another issue is that when unmap the second page fails, the length
> parameter to delete tmp1 mappings should be 3*page_size.
>
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/mmap.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/mmap.c b/tools/testing/selftests/bpf/prog_tests/mmap.c
> index 43d0b5578f46..2070cfe19cac 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mmap.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mmap.c
> @@ -192,7 +192,7 @@ void test_mmap(void)
>         /* unmap second page: pages 1, 3 mapped */
>         err = munmap(tmp1 + page_size, page_size);
>         if (CHECK(err, "adv_mmap2", "errno %d\n", errno)) {
> -               munmap(tmp1, map_sz);
> +               munmap(tmp1, 3 * page_size);

this is a good catch, thank you!

>                 goto cleanup;
>         }
>
> @@ -207,8 +207,8 @@ void test_mmap(void)
>         CHECK(tmp1 + page_size != tmp2, "adv_mmap4",
>               "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
>
> -       /* re-map all 4 pages */
> -       tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
> +       /* re-map all 3 pages */
> +       tmp2 = mmap(tmp1, 3 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
>                     data_map_fd, 0);

"all 3 pages" is a lie, there are 4. I'd still want to work with all 4
pages. How about we mmap() 4 pages of anonymous memory first, then do
all the mmap() with MAP_FIXED, re-using that memory range. That will
ensure that we are not stepping on any other allocated memory, right?


>         if (CHECK(tmp2 == MAP_FAILED, "adv_mmap5", "errno %d\n", errno)) {
>                 munmap(tmp1, 3 * page_size); /* unmap page 1 */
> @@ -226,7 +226,7 @@ void test_mmap(void)
>         CHECK_FAIL(map_data->val[2] != 321);
>         CHECK_FAIL(map_data->val[far] != 3 * 321);
>
> -       munmap(tmp2, 4 * page_size);
> +       munmap(tmp2, 3 * page_size);
>
>         /* map all 4 pages, but with pg_off=1 page, should fail */
>         tmp1 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
> --
> 2.17.1
>

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

* [PATCH bpf-next v2] bpf: fix segmentation fault of test_progs
  2020-08-07 17:20 ` [PATCH bpf-next] bpf: fix segmentation fault of test_progs Jianlin Lv
  2020-08-07 20:13   ` Andrii Nakryiko
@ 2020-08-10 15:39   ` Jianlin Lv
  2020-08-11  0:23     ` Andrii Nakryiko
  2020-08-11 13:19     ` Daniel Borkmann
  1 sibling, 2 replies; 10+ messages in thread
From: Jianlin Lv @ 2020-08-10 15:39 UTC (permalink / raw)
  To: bpf; +Cc: davem, kuba, ast, daniel, yhs, Jianlin.Lv, linux-kernel, netdev

test_progs reports the segmentation fault as below

$ sudo ./test_progs -t mmap --verbose
test_mmap:PASS:skel_open_and_load 0 nsec
......
test_mmap:PASS:adv_mmap1 0 nsec
test_mmap:PASS:adv_mmap2 0 nsec
test_mmap:PASS:adv_mmap3 0 nsec
test_mmap:PASS:adv_mmap4 0 nsec
Segmentation fault

This issue was triggered because mmap() and munmap() used inconsistent
length parameters; mmap() creates a new mapping of 3*page_size, but the
length parameter set in the subsequent re-map and munmap() functions is
4*page_size; this leads to the destruction of the process space.

To fix this issue, first create 4 pages of anonymous mapping,then do all
the mmap() with MAP_FIXED.

Another issue is that when unmap the second page fails, the length
parameter to delete tmp1 mappings should be 4*page_size.

Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
---
v2:
- Update commit messages
- Create 4 pages of anonymous mapping that serve the subsequent mmap()
---
 tools/testing/selftests/bpf/prog_tests/mmap.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mmap.c b/tools/testing/selftests/bpf/prog_tests/mmap.c
index 43d0b5578f46..9c3c5c0f068f 100644
--- a/tools/testing/selftests/bpf/prog_tests/mmap.c
+++ b/tools/testing/selftests/bpf/prog_tests/mmap.c
@@ -21,7 +21,7 @@ void test_mmap(void)
 	const long page_size = sysconf(_SC_PAGE_SIZE);
 	int err, duration = 0, i, data_map_fd, data_map_id, tmp_fd, rdmap_fd;
 	struct bpf_map *data_map, *bss_map;
-	void *bss_mmaped = NULL, *map_mmaped = NULL, *tmp1, *tmp2;
+	void *bss_mmaped = NULL, *map_mmaped = NULL, *tmp0, *tmp1, *tmp2;
 	struct test_mmap__bss *bss_data;
 	struct bpf_map_info map_info;
 	__u32 map_info_sz = sizeof(map_info);
@@ -183,16 +183,23 @@ void test_mmap(void)
 
 	/* check some more advanced mmap() manipulations */
 
+	tmp0 = mmap(NULL, 4 * page_size, PROT_READ, MAP_SHARED | MAP_ANONYMOUS,
+			  -1, 0);
+	if (CHECK(tmp0 == MAP_FAILED, "adv_mmap0", "errno %d\n", errno))
+		goto cleanup;
+
 	/* map all but last page: pages 1-3 mapped */
-	tmp1 = mmap(NULL, 3 * page_size, PROT_READ, MAP_SHARED,
+	tmp1 = mmap(tmp0, 3 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
 			  data_map_fd, 0);
-	if (CHECK(tmp1 == MAP_FAILED, "adv_mmap1", "errno %d\n", errno))
+	if (CHECK(tmp0 != tmp1, "adv_mmap1", "tmp0: %p, tmp1: %p\n", tmp0, tmp1)) {
+		munmap(tmp0, 4 * page_size);
 		goto cleanup;
+	}
 
 	/* unmap second page: pages 1, 3 mapped */
 	err = munmap(tmp1 + page_size, page_size);
 	if (CHECK(err, "adv_mmap2", "errno %d\n", errno)) {
-		munmap(tmp1, map_sz);
+		munmap(tmp1, 4 * page_size);
 		goto cleanup;
 	}
 
@@ -201,7 +208,7 @@ void test_mmap(void)
 		    MAP_SHARED | MAP_FIXED, data_map_fd, 0);
 	if (CHECK(tmp2 == MAP_FAILED, "adv_mmap3", "errno %d\n", errno)) {
 		munmap(tmp1, page_size);
-		munmap(tmp1 + 2*page_size, page_size);
+		munmap(tmp1 + 2*page_size, 2 * page_size);
 		goto cleanup;
 	}
 	CHECK(tmp1 + page_size != tmp2, "adv_mmap4",
@@ -211,7 +218,7 @@ void test_mmap(void)
 	tmp2 = mmap(tmp1, 4 * page_size, PROT_READ, MAP_SHARED | MAP_FIXED,
 		    data_map_fd, 0);
 	if (CHECK(tmp2 == MAP_FAILED, "adv_mmap5", "errno %d\n", errno)) {
-		munmap(tmp1, 3 * page_size); /* unmap page 1 */
+		munmap(tmp1, 4 * page_size); /* unmap page 1 */
 		goto cleanup;
 	}
 	CHECK(tmp1 != tmp2, "adv_mmap6", "tmp1: %p, tmp2: %p\n", tmp1, tmp2);
-- 
2.17.1


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

* Re: [PATCH bpf-next v2] bpf: fix segmentation fault of test_progs
  2020-08-10 15:39   ` [PATCH bpf-next v2] " Jianlin Lv
@ 2020-08-11  0:23     ` Andrii Nakryiko
  2020-08-11 13:19     ` Daniel Borkmann
  1 sibling, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2020-08-11  0:23 UTC (permalink / raw)
  To: Jianlin Lv
  Cc: bpf, David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Yonghong Song, open list, Networking

On Mon, Aug 10, 2020 at 8:40 AM Jianlin Lv <Jianlin.Lv@arm.com> wrote:
>
> test_progs reports the segmentation fault as below
>
> $ sudo ./test_progs -t mmap --verbose
> test_mmap:PASS:skel_open_and_load 0 nsec
> ......
> test_mmap:PASS:adv_mmap1 0 nsec
> test_mmap:PASS:adv_mmap2 0 nsec
> test_mmap:PASS:adv_mmap3 0 nsec
> test_mmap:PASS:adv_mmap4 0 nsec
> Segmentation fault
>
> This issue was triggered because mmap() and munmap() used inconsistent
> length parameters; mmap() creates a new mapping of 3*page_size, but the
> length parameter set in the subsequent re-map and munmap() functions is
> 4*page_size; this leads to the destruction of the process space.
>
> To fix this issue, first create 4 pages of anonymous mapping,then do all
> the mmap() with MAP_FIXED.
>
> Another issue is that when unmap the second page fails, the length
> parameter to delete tmp1 mappings should be 4*page_size.
>
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
> ---

LGTM, thanks for the fix!

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

> v2:
> - Update commit messages
> - Create 4 pages of anonymous mapping that serve the subsequent mmap()
> ---
>  tools/testing/selftests/bpf/prog_tests/mmap.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>

[...]

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

* Re: [PATCH bpf-next v2] bpf: fix segmentation fault of test_progs
  2020-08-10 15:39   ` [PATCH bpf-next v2] " Jianlin Lv
  2020-08-11  0:23     ` Andrii Nakryiko
@ 2020-08-11 13:19     ` Daniel Borkmann
  1 sibling, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2020-08-11 13:19 UTC (permalink / raw)
  To: Jianlin Lv, bpf; +Cc: davem, kuba, ast, yhs, linux-kernel, netdev

On 8/10/20 5:39 PM, Jianlin Lv wrote:
> test_progs reports the segmentation fault as below
> 
> $ sudo ./test_progs -t mmap --verbose
> test_mmap:PASS:skel_open_and_load 0 nsec
> ......
> test_mmap:PASS:adv_mmap1 0 nsec
> test_mmap:PASS:adv_mmap2 0 nsec
> test_mmap:PASS:adv_mmap3 0 nsec
> test_mmap:PASS:adv_mmap4 0 nsec
> Segmentation fault
> 
> This issue was triggered because mmap() and munmap() used inconsistent
> length parameters; mmap() creates a new mapping of 3*page_size, but the
> length parameter set in the subsequent re-map and munmap() functions is
> 4*page_size; this leads to the destruction of the process space.
> 
> To fix this issue, first create 4 pages of anonymous mapping,then do all
> the mmap() with MAP_FIXED.
> 
> Another issue is that when unmap the second page fails, the length
> parameter to delete tmp1 mappings should be 4*page_size.
> 
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>

Applied, thanks!

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

end of thread, other threads:[~2020-08-11 13:20 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-31  6:16 [PATCH bpf-next] bpf: fix compilation warning of selftests Jianlin Lv
2020-07-31 15:00 ` Daniel Borkmann
2020-07-31 17:39 ` Andrii Nakryiko
2020-08-06 10:42 ` [PATCH bpf-next v2] " Jianlin Lv
2020-08-07  0:05   ` Alexei Starovoitov
2020-08-07 17:20 ` [PATCH bpf-next] bpf: fix segmentation fault of test_progs Jianlin Lv
2020-08-07 20:13   ` Andrii Nakryiko
2020-08-10 15:39   ` [PATCH bpf-next v2] " Jianlin Lv
2020-08-11  0:23     ` Andrii Nakryiko
2020-08-11 13:19     ` Daniel Borkmann

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