| Message ID | 20200731061600.18344-1-Jianlin.Lv@arm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpf: fix compilation warning of selftests | expand |
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) { >
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; > + } > [...]
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) {
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(-)