All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/4] selftests/net: fix a few small compiler warnings
@ 2023-11-24 17:15 Willem de Bruijn
  2023-11-24 17:15 ` [PATCH net 1/4] selftests/net: ipsec: fix constant out of range Willem de Bruijn
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Willem de Bruijn @ 2023-11-24 17:15 UTC (permalink / raw)
  To: netdev; +Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn

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

-- 
2.43.0.rc1.413.gea7ed67945-goog


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

* [PATCH net 1/4] selftests/net: ipsec: fix constant out of range
  2023-11-24 17:15 [PATCH net 0/4] selftests/net: fix a few small compiler warnings Willem de Bruijn
@ 2023-11-24 17:15 ` Willem de Bruijn
  2023-11-27 14:37   ` Dmitry Safonov
  2023-11-24 17:15 ` [PATCH net 2/4] selftests/net: fix a char signedness issue Willem de Bruijn
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Willem de Bruijn @ 2023-11-24 17:15 UTC (permalink / raw)
  To: netdev
  Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn,
	Dmitry Safonov

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);
-- 
2.43.0.rc1.413.gea7ed67945-goog


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

* [PATCH net 2/4] selftests/net: fix a char signedness issue
  2023-11-24 17:15 [PATCH net 0/4] selftests/net: fix a few small compiler warnings Willem de Bruijn
  2023-11-24 17:15 ` [PATCH net 1/4] selftests/net: ipsec: fix constant out of range Willem de Bruijn
@ 2023-11-24 17:15 ` Willem de Bruijn
  2023-11-24 17:15 ` [PATCH net 3/4] selftests/net: unix: fix unused variable compiler warning Willem de Bruijn
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2023-11-24 17:15 UTC (permalink / raw)
  To: netdev; +Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn

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;
 
-- 
2.43.0.rc1.413.gea7ed67945-goog


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

* [PATCH net 3/4] selftests/net: unix: fix unused variable compiler warning
  2023-11-24 17:15 [PATCH net 0/4] selftests/net: fix a few small compiler warnings Willem de Bruijn
  2023-11-24 17:15 ` [PATCH net 1/4] selftests/net: ipsec: fix constant out of range Willem de Bruijn
  2023-11-24 17:15 ` [PATCH net 2/4] selftests/net: fix a char signedness issue Willem de Bruijn
@ 2023-11-24 17:15 ` Willem de Bruijn
  2023-11-24 17:15 ` [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings Willem de Bruijn
  2023-11-28  2:20 ` [PATCH net 0/4] selftests/net: fix a few small compiler warnings patchwork-bot+netdevbpf
  4 siblings, 0 replies; 10+ messages in thread
From: Willem de Bruijn @ 2023-11-24 17:15 UTC (permalink / raw)
  To: netdev; +Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn

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;
 
-- 
2.43.0.rc1.413.gea7ed67945-goog


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

* [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings
  2023-11-24 17:15 [PATCH net 0/4] selftests/net: fix a few small compiler warnings Willem de Bruijn
                   ` (2 preceding siblings ...)
  2023-11-24 17:15 ` [PATCH net 3/4] selftests/net: unix: fix unused variable compiler warning Willem de Bruijn
@ 2023-11-24 17:15 ` Willem de Bruijn
  2023-11-27 12:29   ` Matthieu Baerts
  2023-11-28  2:20 ` [PATCH net 0/4] selftests/net: fix a few small compiler warnings patchwork-bot+netdevbpf
  4 siblings, 1 reply; 10+ messages in thread
From: Willem de Bruijn @ 2023-11-24 17:15 UTC (permalink / raw)
  To: netdev
  Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn,
	Florian Westphal

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);
-- 
2.43.0.rc1.413.gea7ed67945-goog


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

* Re: [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings
  2023-11-24 17:15 ` [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings Willem de Bruijn
@ 2023-11-27 12:29   ` Matthieu Baerts
  2023-11-27 15:46     ` Willem de Bruijn
  0 siblings, 1 reply; 10+ messages in thread
From: Matthieu Baerts @ 2023-11-27 12:29 UTC (permalink / raw)
  To: Willem de Bruijn, netdev
  Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn,
	Florian Westphal, MPTCP Upstream

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

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

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

* Re: [PATCH net 1/4] selftests/net: ipsec: fix constant out of range
  2023-11-24 17:15 ` [PATCH net 1/4] selftests/net: ipsec: fix constant out of range Willem de Bruijn
@ 2023-11-27 14:37   ` Dmitry Safonov
  0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Safonov @ 2023-11-27 14:37 UTC (permalink / raw)
  To: Willem de Bruijn, netdev
  Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn

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

-- 
Dmitry


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

* Re: [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings
  2023-11-27 12:29   ` Matthieu Baerts
@ 2023-11-27 15:46     ` Willem de Bruijn
  2023-11-27 16:33       ` Matthieu Baerts
  0 siblings, 1 reply; 10+ messages in thread
From: Willem de Bruijn @ 2023-11-27 15:46 UTC (permalink / raw)
  To: Matthieu Baerts, Willem de Bruijn, netdev
  Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn,
	Florian Westphal, MPTCP Upstream

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

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



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

* Re: [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings
  2023-11-27 15:46     ` Willem de Bruijn
@ 2023-11-27 16:33       ` Matthieu Baerts
  0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2023-11-27 16:33 UTC (permalink / raw)
  To: Willem de Bruijn, netdev
  Cc: davem, kuba, edumazet, pabeni, linux-kselftest, Willem de Bruijn,
	Florian Westphal, MPTCP Upstream

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

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

* Re: [PATCH net 0/4] selftests/net: fix a few small compiler warnings
  2023-11-24 17:15 [PATCH net 0/4] selftests/net: fix a few small compiler warnings Willem de Bruijn
                   ` (3 preceding siblings ...)
  2023-11-24 17:15 ` [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings Willem de Bruijn
@ 2023-11-28  2:20 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-11-28  2:20 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: netdev, davem, kuba, edumazet, pabeni, linux-kselftest, willemb

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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

end of thread, other threads:[~2023-11-28  2:20 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-24 17:15 [PATCH net 0/4] selftests/net: fix a few small compiler warnings Willem de Bruijn
2023-11-24 17:15 ` [PATCH net 1/4] selftests/net: ipsec: fix constant out of range Willem de Bruijn
2023-11-27 14:37   ` Dmitry Safonov
2023-11-24 17:15 ` [PATCH net 2/4] selftests/net: fix a char signedness issue Willem de Bruijn
2023-11-24 17:15 ` [PATCH net 3/4] selftests/net: unix: fix unused variable compiler warning Willem de Bruijn
2023-11-24 17:15 ` [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings Willem de Bruijn
2023-11-27 12:29   ` Matthieu Baerts
2023-11-27 15:46     ` Willem de Bruijn
2023-11-27 16:33       ` Matthieu Baerts
2023-11-28  2:20 ` [PATCH net 0/4] selftests/net: fix a few small compiler warnings patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.