diff mbox series

[bpf-next,v2] selftests/bpf: fix the incorrect verification of port numbers.

Message ID 20230804165831.173627-1-thinker.li@gmail.com (mailing list archive)
State Accepted
Commit 9eab71bd887ae28882c29a82cb0cdb00e1654c54
Delegated to: BPF
Headers show
Series [bpf-next,v2] selftests/bpf: fix the incorrect verification of port numbers. | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers fail 1 blamed authors not CCed: daniel@iogearbox.net; 9 maintainers not CCed: daniel@iogearbox.net kpsingh@kernel.org shuah@kernel.org john.fastabend@gmail.com sdf@google.com mykolal@fb.com linux-kselftest@vger.kernel.org jolsa@kernel.org haoluo@google.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on s390x with gcc

Commit Message

Kui-Feng Lee Aug. 4, 2023, 4:58 p.m. UTC 
From: Kui-Feng Lee <thinker.li@gmail.com>

Check port numbers before calling htons().

According to Dan Carpenter's report, Smatch identified incorrect port
number checks. It is expected that the returned port number is an integer,
with negative numbers indicating errors. However, the value was mistakenly
verified after being translated by htons().

Major changes from v1:

 - Move the variable 'port' to the same line of 'err'.

Fixes: 539c7e67aa4a ("selftests/bpf: Verify that the cgroup_skb filters receive expected packets.")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/bpf/cafd6585-d5a2-4096-b94f-7556f5aa7737@moroto.mountain/
Acked-by: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org Aug. 4, 2023, 9:30 p.m. UTC | #1 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Fri,  4 Aug 2023 09:58:31 -0700 you wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Check port numbers before calling htons().
> 
> According to Dan Carpenter's report, Smatch identified incorrect port
> number checks. It is expected that the returned port number is an integer,
> with negative numbers indicating errors. However, the value was mistakenly
> verified after being translated by htons().
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2] selftests/bpf: fix the incorrect verification of port numbers.
    https://git.kernel.org/bpf/bpf-next/c/9eab71bd887a

You are awesome, thank you!
Martin KaFai Lau Aug. 4, 2023, 9:37 p.m. UTC | #2 
On 8/4/23 9:58 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Check port numbers before calling htons().
> 
> According to Dan Carpenter's report, Smatch identified incorrect port
> number checks. It is expected that the returned port number is an integer,
> with negative numbers indicating errors. However, the value was mistakenly
> verified after being translated by htons().
> 
> Major changes from v1:
> 
>   - Move the variable 'port' to the same line of 'err'.
> 
> Fixes: 539c7e67aa4a ("selftests/bpf: Verify that the cgroup_skb filters receive expected packets.")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/bpf/cafd6585-d5a2-4096-b94f-7556f5aa7737@moroto.mountain/
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
> index 95bab61a1e57..d686ef19f705 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
> @@ -110,11 +110,12 @@ static int connect_client_server_v6(int client_fd, int listen_fd)
>   		.sin6_family = AF_INET6,
>   		.sin6_addr = IN6ADDR_LOOPBACK_INIT,
>   	};
> -	int err;
> +	int err, port;
>   
> -	addr.sin6_port = htons(get_sock_port_v6(listen_fd));
> -	if (addr.sin6_port < 0)
> +	port = get_sock_port_v6(listen_fd);
> +	if (port < 0)

Applied. Some follow up questions:

Does other get_sock_port_v6() usage need to check -1 also?

It is a good idea to see if similar helpers exist in network_helpers.c e.g. 
There is get_socket_local_port() that supports both v4 and v6 in 
network_helpers.c which is equivalent to the get_sock_port_v6() here.
To a larger extent, I believe many codes in this new test can be saved by using 
the helpers in network_helpers.c. For example, the connect_client_server_v6() 
here can be replaced with connect_to_fd() from network_helpers.c to avoid the 
mistake this patch fixing. It also has some timeout limit on the socket such 
that it won't block the test_progs for a long time.
Kui-Feng Lee Aug. 7, 2023, 5:12 p.m. UTC | #3 
On 8/4/23 14:37, Martin KaFai Lau wrote:
> On 8/4/23 9:58 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Check port numbers before calling htons().
>>
>> According to Dan Carpenter's report, Smatch identified incorrect port
>> number checks. It is expected that the returned port number is an 
>> integer,
>> with negative numbers indicating errors. However, the value was 
>> mistakenly
>> verified after being translated by htons().
>>
>> Major changes from v1:
>>
>>   - Move the variable 'port' to the same line of 'err'.
>>
>> Fixes: 539c7e67aa4a ("selftests/bpf: Verify that the cgroup_skb 
>> filters receive expected packets.")
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: 
>> https://lore.kernel.org/bpf/cafd6585-d5a2-4096-b94f-7556f5aa7737@moroto.mountain/
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c 
>> b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
>> index 95bab61a1e57..d686ef19f705 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
>> @@ -110,11 +110,12 @@ static int connect_client_server_v6(int 
>> client_fd, int listen_fd)
>>           .sin6_family = AF_INET6,
>>           .sin6_addr = IN6ADDR_LOOPBACK_INIT,
>>       };
>> -    int err;
>> +    int err, port;
>> -    addr.sin6_port = htons(get_sock_port_v6(listen_fd));
>> -    if (addr.sin6_port < 0)
>> +    port = get_sock_port_v6(listen_fd);
>> +    if (port < 0)
> 
> Applied. Some follow up questions:
> 
> Does other get_sock_port_v6() usage need to check -1 also?
> 
> It is a good idea to see if similar helpers exist in network_helpers.c 
> e.g. There is get_socket_local_port() that supports both v4 and v6 in 
> network_helpers.c which is equivalent to the get_sock_port_v6() here.
> To a larger extent, I believe many codes in this new test can be saved 
> by using the helpers in network_helpers.c. For example, the 
> connect_client_server_v6() here can be replaced with connect_to_fd() 
> from network_helpers.c to avoid the mistake this patch fixing. It also 
> has some timeout limit on the socket such that it won't block the 
> test_progs for a long time.


Got it! I will check the code to send another patch if necessary.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
index 95bab61a1e57..d686ef19f705 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
@@ -110,11 +110,12 @@  static int connect_client_server_v6(int client_fd, int listen_fd)
 		.sin6_family = AF_INET6,
 		.sin6_addr = IN6ADDR_LOOPBACK_INIT,
 	};
-	int err;
+	int err, port;
 
-	addr.sin6_port = htons(get_sock_port_v6(listen_fd));
-	if (addr.sin6_port < 0)
+	port = get_sock_port_v6(listen_fd);
+	if (port < 0)
 		return -1;
+	addr.sin6_port = htons(port);
 
 	err = connect(client_fd, (struct sockaddr *)&addr, sizeof(addr));
 	if (err < 0) {