From: Willem de Bruijn willemb@google.com
Observed a clang warning when backporting cmsg_sender. Ran the same build against all the .c files under selftests/net.
This is clang-14 with -Wall Which is what tools/testing/selftests/net/Makefile also enables.
Willem de Bruijn (4): selftests/net: ipsec: fix constant out of range selftests/net: fix a char signedness issue selftests/net: unix: fix unused variable compiler warning selftests/net: mptcp: fix uninitialized variable warnings
tools/testing/selftests/net/af_unix/diag_uid.c | 1 - tools/testing/selftests/net/cmsg_sender.c | 2 +- tools/testing/selftests/net/ipsec.c | 4 ++-- tools/testing/selftests/net/mptcp/mptcp_connect.c | 11 ++++------- tools/testing/selftests/net/mptcp/mptcp_inq.c | 11 ++++------- 5 files changed, 11 insertions(+), 18 deletions(-)
From: Willem de Bruijn willemb@google.com
Fix a small compiler warning.
nr_process must be a signed long: it is assigned a signed long by strtol() and is compared against LONG_MIN and LONG_MAX.
ipsec.c:2280:65: error: result of comparison of constant -9223372036854775808 with expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if ((errno == ERANGE && (nr_process == LONG_MAX || nr_process == LONG_MIN))
Fixes: bc2652b7ae1e ("selftest/net/xfrm: Add test for ipsec tunnel") Cc: Dmitry Safonov 0x7f454c46@gmail.com Signed-off-by: Willem de Bruijn willemb@google.com --- tools/testing/selftests/net/ipsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/ipsec.c b/tools/testing/selftests/net/ipsec.c index 9a8229abfa026..be4a30a0d02ae 100644 --- a/tools/testing/selftests/net/ipsec.c +++ b/tools/testing/selftests/net/ipsec.c @@ -2263,7 +2263,7 @@ static int check_results(void)
int main(int argc, char **argv) { - unsigned int nr_process = 1; + long nr_process = 1; int route_sock = -1, ret = KSFT_SKIP; int test_desc_fd[2]; uint32_t route_seq; @@ -2284,7 +2284,7 @@ int main(int argc, char **argv) exit_usage(argv); }
- if (nr_process > MAX_PROCESSES || !nr_process) { + if (nr_process > MAX_PROCESSES || nr_process < 1) { printk("nr_process should be between [1; %u]", MAX_PROCESSES); exit_usage(argv);
On 11/24/23 17:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Fix a small compiler warning.
nr_process must be a signed long: it is assigned a signed long by strtol() and is compared against LONG_MIN and LONG_MAX.
ipsec.c:2280:65: error: result of comparison of constant -9223372036854775808 with expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if ((errno == ERANGE && (nr_process == LONG_MAX || nr_process == LONG_MIN))
Fixes: bc2652b7ae1e ("selftest/net/xfrm: Add test for ipsec tunnel") Cc: Dmitry Safonov 0x7f454c46@gmail.com Signed-off-by: Willem de Bruijn willemb@google.com
LGTM, thanks! Reviewed-by: Dmitry Safonov 0x7f454c46@gmail.com
tools/testing/selftests/net/ipsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/ipsec.c b/tools/testing/selftests/net/ipsec.c index 9a8229abfa026..be4a30a0d02ae 100644 --- a/tools/testing/selftests/net/ipsec.c +++ b/tools/testing/selftests/net/ipsec.c @@ -2263,7 +2263,7 @@ static int check_results(void) int main(int argc, char **argv) {
- unsigned int nr_process = 1;
- long nr_process = 1; int route_sock = -1, ret = KSFT_SKIP; int test_desc_fd[2]; uint32_t route_seq;
@@ -2284,7 +2284,7 @@ int main(int argc, char **argv) exit_usage(argv); }
if (nr_process > MAX_PROCESSES || !nr_process) {
if (nr_process > MAX_PROCESSES || nr_process < 1) { printk("nr_process should be between [1; %u]", MAX_PROCESSES); exit_usage(argv);
From: Willem de Bruijn willemb@google.com
Signedness of char is signed on x86_64, but unsigned on arm64.
Fix the warning building cmsg_sender.c on signed platforms or forced with -fsigned-char:
msg_sender.c:455:12: error: implicit conversion from 'int' to 'char' changes value from 128 to -128 [-Werror,-Wconstant-conversion] buf[0] = ICMPV6_ECHO_REQUEST;
constant ICMPV6_ECHO_REQUEST is 128.
Link: https://lwn.net/Articles/911914 Fixes: de17e305a810 ("selftests: net: cmsg_sender: support icmp and raw sockets") Signed-off-by: Willem de Bruijn willemb@google.com --- tools/testing/selftests/net/cmsg_sender.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/cmsg_sender.c b/tools/testing/selftests/net/cmsg_sender.c index 24b21b15ed3fb..6ff3e732f449f 100644 --- a/tools/testing/selftests/net/cmsg_sender.c +++ b/tools/testing/selftests/net/cmsg_sender.c @@ -416,9 +416,9 @@ int main(int argc, char *argv[]) { struct addrinfo hints, *ai; struct iovec iov[1]; + unsigned char *buf; struct msghdr msg; char cbuf[1024]; - char *buf; int err; int fd;
From: Willem de Bruijn willemb@google.com
Remove an unused variable.
diag_uid.c:151:24: error: unused variable 'udr' [-Werror,-Wunused-variable]
Fixes: ac011361bd4f ("af_unix: Add test for sock_diag and UDIAG_SHOW_UID.") Signed-off-by: Willem de Bruijn willemb@google.com --- tools/testing/selftests/net/af_unix/diag_uid.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/net/af_unix/diag_uid.c b/tools/testing/selftests/net/af_unix/diag_uid.c index 5b88f7129fea4..79a3dd75590e8 100644 --- a/tools/testing/selftests/net/af_unix/diag_uid.c +++ b/tools/testing/selftests/net/af_unix/diag_uid.c @@ -148,7 +148,6 @@ void receive_response(struct __test_metadata *_metadata, .msg_iov = &iov, .msg_iovlen = 1 }; - struct unix_diag_req *udr; struct nlmsghdr *nlh; int ret;
From: Willem de Bruijn willemb@google.com
Same init_rng() in both tests. The function reads /dev/urandom to initialize srand(). In case of failure, it falls back onto the entropy in the uninitialized variable. Not sure if this is on purpose. But failure reading urandom should be rare, so just fail hard. While at it, convert to getrandom(). Which man 4 random suggests is simpler and more robust.
mptcp_inq.c:525:6: mptcp_connect.c:1131:6:
error: variable 'foo' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp") Fixes: b51880568f20 ("selftests: mptcp: add inq test case") Cc: Florian Westphal fw@strlen.de Signed-off-by: Willem de Bruijn willemb@google.com
----
When input is randomized because this is expected to meaningfully explore edge cases, should we also add 1. logging the random seed to stdout and 2. adding a command line argument to replay from a specific seed I can do this in net-next, if authors find it useful in this case. --- tools/testing/selftests/net/mptcp/mptcp_connect.c | 11 ++++------- tools/testing/selftests/net/mptcp/mptcp_inq.c | 11 ++++------- 2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c index c7f9ebeebc2c5..d2043ec3bf6d6 100644 --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c @@ -18,6 +18,7 @@
#include <sys/ioctl.h> #include <sys/poll.h> +#include <sys/random.h> #include <sys/sendfile.h> #include <sys/stat.h> #include <sys/socket.h> @@ -1125,15 +1126,11 @@ int main_loop_s(int listensock)
static void init_rng(void) { - int fd = open("/dev/urandom", O_RDONLY); unsigned int foo;
- if (fd > 0) { - int ret = read(fd, &foo, sizeof(foo)); - - if (ret < 0) - srand(fd + foo); - close(fd); + if (getrandom(&foo, sizeof(foo), 0) == -1) { + perror("getrandom"); + exit(1); }
srand(foo); diff --git a/tools/testing/selftests/net/mptcp/mptcp_inq.c b/tools/testing/selftests/net/mptcp/mptcp_inq.c index 8672d898f8cda..218aac4673212 100644 --- a/tools/testing/selftests/net/mptcp/mptcp_inq.c +++ b/tools/testing/selftests/net/mptcp/mptcp_inq.c @@ -18,6 +18,7 @@ #include <time.h>
#include <sys/ioctl.h> +#include <sys/random.h> #include <sys/socket.h> #include <sys/types.h> #include <sys/wait.h> @@ -519,15 +520,11 @@ static int client(int unixfd)
static void init_rng(void) { - int fd = open("/dev/urandom", O_RDONLY); unsigned int foo;
- if (fd > 0) { - int ret = read(fd, &foo, sizeof(foo)); - - if (ret < 0) - srand(fd + foo); - close(fd); + if (getrandom(&foo, sizeof(foo), 0) == -1) { + perror("getrandom"); + exit(1); }
srand(foo);
Hi Willem,
(+ cc MPTCP list)
On 24/11/2023 18:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Same init_rng() in both tests. The function reads /dev/urandom to initialize srand(). In case of failure, it falls back onto the entropy in the uninitialized variable. Not sure if this is on purpose. But failure reading urandom should be rare, so just fail hard. While at it, convert to getrandom(). Which man 4 random suggests is simpler and more robust.
mptcp_inq.c:525:6: mptcp_connect.c:1131:6: error: variable 'foo' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
Thank you for the patch!
It looks good to me:
Reviewed-by: Matthieu Baerts matttbe@kernel.org
Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp") Fixes: b51880568f20 ("selftests: mptcp: add inq test case") Cc: Florian Westphal fw@strlen.de Signed-off-by: Willem de Bruijn willemb@google.com
When input is randomized because this is expected to meaningfully explore edge cases, should we also add
- logging the random seed to stdout and
- adding a command line argument to replay from a specific seed
I can do this in net-next, if authors find it useful in this case.
I think we should have done that from the beginning, otherwise we cannot easily reproduce these edge cases. To be honest, I don't think this technique helped to find bugs, and it was probably used here as a good habit to increase the coverage. But on the other hand, we might not realise some inputs are randomised and can cause instabilities in the tests because we don't print anything about that.
So I would say that the minimal thing to do is to log the random seed. But it might not be that easy to do, for example 'mptcp_connect' is used a lot of time by the .sh scripts: printing this seed number each time 'mptcp_connect' is started will "flood" the logs. Maybe we should only print that at the end, in case of errors: e.g. in xerror() and die_perror() for example, but I see 'exit(1)' is directly used in other places...
That's more code to change, but if it is still OK for you to do that, please also note that you will need to log this to stderr: mptcp_connect prints what has been received from the other peer to stdout.
Because it is more than just adding a 'printf()', I just created a ticket in our bug tracker, so anybody can look at that and check all the details about that:
https://github.com/multipath-tcp/mptcp_net-next/issues/462
tools/testing/selftests/net/mptcp/mptcp_connect.c | 11 ++++------- tools/testing/selftests/net/mptcp/mptcp_inq.c | 11 ++++------- 2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c index c7f9ebeebc2c5..d2043ec3bf6d6 100644 --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
(...)
@@ -1125,15 +1126,11 @@ int main_loop_s(int listensock) static void init_rng(void) {
- int fd = open("/dev/urandom", O_RDONLY); unsigned int foo;
- if (fd > 0) {
I just realised that here, we could have fd == 0 which is a valid value. I don't think we would have that when executing the selftests, but that's another reason to change this :)
int ret = read(fd, &foo, sizeof(foo));if (ret < 0)srand(fd + foo);close(fd);
- if (getrandom(&foo, sizeof(foo), 0) == -1) {
perror("getrandom");exit(1);srand(foo);
Cheers, Matt
Matthieu Baerts wrote:
Hi Willem,
(+ cc MPTCP list)
On 24/11/2023 18:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Same init_rng() in both tests. The function reads /dev/urandom to initialize srand(). In case of failure, it falls back onto the entropy in the uninitialized variable. Not sure if this is on purpose. But failure reading urandom should be rare, so just fail hard. While at it, convert to getrandom(). Which man 4 random suggests is simpler and more robust.
mptcp_inq.c:525:6: mptcp_connect.c:1131:6: error: variable 'foo' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]Thank you for the patch!
It looks good to me:
Reviewed-by: Matthieu Baerts matttbe@kernel.org
Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp") Fixes: b51880568f20 ("selftests: mptcp: add inq test case") Cc: Florian Westphal fw@strlen.de Signed-off-by: Willem de Bruijn willemb@google.com
When input is randomized because this is expected to meaningfully explore edge cases, should we also add
- logging the random seed to stdout and
- adding a command line argument to replay from a specific seed
I can do this in net-next, if authors find it useful in this case.
I think we should have done that from the beginning, otherwise we cannot easily reproduce these edge cases. To be honest, I don't think this technique helped to find bugs, and it was probably used here as a good habit to increase the coverage. But on the other hand, we might not realise some inputs are randomised and can cause instabilities in the tests because we don't print anything about that.
So I would say that the minimal thing to do is to log the random seed. But it might not be that easy to do, for example 'mptcp_connect' is used a lot of time by the .sh scripts: printing this seed number each time 'mptcp_connect' is started will "flood" the logs. Maybe we should only print that at the end, in case of errors: e.g. in xerror() and die_perror() for example, but I see 'exit(1)' is directly used in other places...
That's more code to change, but if it is still OK for you to do that, please also note that you will need to log this to stderr: mptcp_connect prints what has been received from the other peer to stdout.
Because it is more than just adding a 'printf()', I just created a ticket in our bug tracker, so anybody can look at that and check all the details about that:
Thanks for the detailed feedback, Matthieu!
Another option to avoid flooding the logs might be to choose a pseudo random number in the script and pass the explicit value mptcp_connect.
I haven't looked closely, but for transport layer tests it is likely that the payload is entirely ignored. Bar perhaps checksum coverage. If it does not increase code coverage, randomization can also just be turned off.
tools/testing/selftests/net/mptcp/mptcp_connect.c | 11 ++++------- tools/testing/selftests/net/mptcp/mptcp_inq.c | 11 ++++------- 2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c index c7f9ebeebc2c5..d2043ec3bf6d6 100644 --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
(...)
@@ -1125,15 +1126,11 @@ int main_loop_s(int listensock) static void init_rng(void) {
- int fd = open("/dev/urandom", O_RDONLY); unsigned int foo;
- if (fd > 0) {
I just realised that here, we could have fd == 0 which is a valid value. I don't think we would have that when executing the selftests, but that's another reason to change this :)
int ret = read(fd, &foo, sizeof(foo));if (ret < 0)srand(fd + foo);close(fd);
- if (getrandom(&foo, sizeof(foo), 0) == -1) {
perror("getrandom");exit(1);srand(foo);
Cheers, Matt
On 27/11/2023 16:46, Willem de Bruijn wrote:
Matthieu Baerts wrote:
Hi Willem,
(+ cc MPTCP list)
On 24/11/2023 18:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Same init_rng() in both tests. The function reads /dev/urandom to initialize srand(). In case of failure, it falls back onto the entropy in the uninitialized variable. Not sure if this is on purpose. But failure reading urandom should be rare, so just fail hard. While at it, convert to getrandom(). Which man 4 random suggests is simpler and more robust.
mptcp_inq.c:525:6: mptcp_connect.c:1131:6: error: variable 'foo' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]Thank you for the patch!
It looks good to me:
Reviewed-by: Matthieu Baerts matttbe@kernel.org
Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp") Fixes: b51880568f20 ("selftests: mptcp: add inq test case") Cc: Florian Westphal fw@strlen.de Signed-off-by: Willem de Bruijn willemb@google.com
When input is randomized because this is expected to meaningfully explore edge cases, should we also add
- logging the random seed to stdout and
- adding a command line argument to replay from a specific seed
I can do this in net-next, if authors find it useful in this case.
I think we should have done that from the beginning, otherwise we cannot easily reproduce these edge cases. To be honest, I don't think this technique helped to find bugs, and it was probably used here as a good habit to increase the coverage. But on the other hand, we might not realise some inputs are randomised and can cause instabilities in the tests because we don't print anything about that.
So I would say that the minimal thing to do is to log the random seed. But it might not be that easy to do, for example 'mptcp_connect' is used a lot of time by the .sh scripts: printing this seed number each time 'mptcp_connect' is started will "flood" the logs. Maybe we should only print that at the end, in case of errors: e.g. in xerror() and die_perror() for example, but I see 'exit(1)' is directly used in other places...
That's more code to change, but if it is still OK for you to do that, please also note that you will need to log this to stderr: mptcp_connect prints what has been received from the other peer to stdout.
Because it is more than just adding a 'printf()', I just created a ticket in our bug tracker, so anybody can look at that and check all the details about that:
Thanks for the detailed feedback, Matthieu!
Another option to avoid flooding the logs might be to choose a pseudo random number in the script and pass the explicit value mptcp_connect.
Good idea!
If I understood correctly, from the .c file, we can check if an env var is set (e.g. `MPTCP_RND_SEED`) and use it. If not, we generate a random one like before. The .sh scripts should generate a random number if the env var is not already set. In any case, this seed should be printed by the scripts.
I haven't looked closely, but for transport layer tests it is likely that the payload is entirely ignored. Bar perhaps checksum coverage. If it does not increase code coverage, randomization can also just be turned off.
Here the randomisation is used to change the length of the data that are exchanged or to do some actions in different orders. I think it still makes sense to have randomisation. But in case of issues around that, it might not be clear what the userspace was exactly doing. That's what can be improved later in net-next.
Cheers, Matt
Hello:
This series was applied to netdev/net.git (main) by Jakub Kicinski kuba@kernel.org:
On Fri, 24 Nov 2023 12:15:18 -0500 you wrote:
From: Willem de Bruijn willemb@google.com
Observed a clang warning when backporting cmsg_sender. Ran the same build against all the .c files under selftests/net.
This is clang-14 with -Wall Which is what tools/testing/selftests/net/Makefile also enables.
[...]
Here is the summary with links: - [net,1/4] selftests/net: ipsec: fix constant out of range https://git.kernel.org/netdev/net/c/088559815477 - [net,2/4] selftests/net: fix a char signedness issue https://git.kernel.org/netdev/net/c/7b29828c5af6 - [net,3/4] selftests/net: unix: fix unused variable compiler warning https://git.kernel.org/netdev/net/c/59fef379d453 - [net,4/4] selftests/net: mptcp: fix uninitialized variable warnings https://git.kernel.org/netdev/net/c/00a4f8fd9c75
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Dmitry Safonov -
Matthieu Baerts -
patchwork-bot+netdevbpf＠kernel.org -
Willem de Bruijn