netdev - Re: [PATCH bpf-next] bpf: fix compilation warning of selftests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6f852dcb-abfc-78f8-69b8-3a5b83606793@iogearbox.net>
Date:   Fri, 31 Jul 2020 17:00:05 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jianlin Lv <Jianlin.Lv@....com>, bpf@...r.kernel.org
Cc:     davem@...emloft.net, kuba@...nel.org, ast@...nel.org, yhs@...com,
        Song.Zhu@....com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: fix compilation warning of selftests

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