[bpf-next] selftests/bpf: fix race in test_tcp_rtt test
diff mbox series

Message ID 20190816160339.249832-1-ppenkov.kernel@gmail.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series
  • [bpf-next] selftests/bpf: fix race in test_tcp_rtt test
Related show

Commit Message

Petar Penkov Aug. 16, 2019, 4:03 p.m. UTC
From: Petar Penkov <ppenkov@google.com>

There is a race in this test between receiving the ACK for the
single-byte packet sent in the test, and reading the values from the
map.

This patch fixes this by having the client wait until there are no more
unacknowledged packets.

Before:
for i in {1..1000}; do ../net/in_netns.sh ./test_tcp_rtt; \
done | grep -c PASSED
< trimmed error messages >
993

After:
for i in {1..10000}; do ../net/in_netns.sh ./test_tcp_rtt; \
done | grep -c PASSED
10000

Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB")
Signed-off-by: Petar Penkov <ppenkov@google.com>
---
 tools/testing/selftests/bpf/test_tcp_rtt.c | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Stanislav Fomichev Aug. 16, 2019, 4:13 p.m. UTC | #1
On 08/16, Petar Penkov wrote:
> From: Petar Penkov <ppenkov@google.com>
> 
> There is a race in this test between receiving the ACK for the
> single-byte packet sent in the test, and reading the values from the
> map.
> 
> This patch fixes this by having the client wait until there are no more
> unacknowledged packets.
> 
> Before:
> for i in {1..1000}; do ../net/in_netns.sh ./test_tcp_rtt; \
> done | grep -c PASSED
> < trimmed error messages >
> 993
> 
> After:
> for i in {1..10000}; do ../net/in_netns.sh ./test_tcp_rtt; \
> done | grep -c PASSED
> 10000
> 
> Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB")
> Signed-off-by: Petar Penkov <ppenkov@google.com>
> ---
>  tools/testing/selftests/bpf/test_tcp_rtt.c | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
> index 90c3862f74a8..2b4754473956 100644
> --- a/tools/testing/selftests/bpf/test_tcp_rtt.c
> +++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
> @@ -6,6 +6,7 @@
>  #include <sys/types.h>
>  #include <sys/socket.h>
>  #include <netinet/in.h>
> +#include <netinet/tcp.h>
>  #include <pthread.h>
>  
>  #include <linux/filter.h>
> @@ -34,6 +35,30 @@ static void send_byte(int fd)
>  		error(1, errno, "Failed to send single byte");
>  }
>  
> +static int wait_for_ack(int fd, int retries)
> +{
> +	struct tcp_info info;
> +	socklen_t optlen;
> +	int i, err;
> +
> +	for (i = 0; i < retries; i++) {
> +		optlen = sizeof(info);
> +		err = getsockopt(fd, SOL_TCP, TCP_INFO, &info, &optlen);
> +		if (err < 0) {
> +			log_err("Failed to lookup TCP stats");
> +			return err;
> +		}
> +
> +		if (info.tcpi_unacked == 0)
> +			return 0;
> +
> +		sleep(1);
Isn't it too big of a hammer? Maybe usleep(10) here and do x100 retries
instead?

> +	}
> +
> +	log_err("Did not receive ACK");
> +	return -1;
> +}
> +
>  static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
>  		     __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
>  		     __u32 icsk_retransmits)
> @@ -149,6 +174,11 @@ static int run_test(int cgroup_fd, int server_fd)
>  			 /*icsk_retransmits=*/0);
>  
>  	send_byte(client_fd);
> +	if (wait_for_ack(client_fd, 5) < 0) {
> +		err = -1;
> +		goto close_client_fd;
> +	}
> +
>  
>  	err += verify_sk(map_fd, client_fd, "first payload byte",
>  			 /*invoked=*/2,
> @@ -157,6 +187,7 @@ static int run_test(int cgroup_fd, int server_fd)
>  			 /*delivered_ce=*/0,
>  			 /*icsk_retransmits=*/0);
>  
> +close_client_fd:
>  	close(client_fd);
>  
>  close_bpf_object:
> -- 
> 2.23.0.rc1.153.gdeed80330f-goog
>
Petar Penkov Aug. 16, 2019, 4:37 p.m. UTC | #2
On Fri, Aug 16, 2019 at 9:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 08/16, Petar Penkov wrote:
> > From: Petar Penkov <ppenkov@google.com>
> >
> > There is a race in this test between receiving the ACK for the
> > single-byte packet sent in the test, and reading the values from the
> > map.
> >
> > This patch fixes this by having the client wait until there are no more
> > unacknowledged packets.
> >
> > Before:
> > for i in {1..1000}; do ../net/in_netns.sh ./test_tcp_rtt; \
> > done | grep -c PASSED
> > < trimmed error messages >
> > 993
> >
> > After:
> > for i in {1..10000}; do ../net/in_netns.sh ./test_tcp_rtt; \
> > done | grep -c PASSED
> > 10000
> >
> > Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB")
> > Signed-off-by: Petar Penkov <ppenkov@google.com>
> > ---
> >  tools/testing/selftests/bpf/test_tcp_rtt.c | 31 ++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
> > index 90c3862f74a8..2b4754473956 100644
> > --- a/tools/testing/selftests/bpf/test_tcp_rtt.c
> > +++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
> > @@ -6,6 +6,7 @@
> >  #include <sys/types.h>
> >  #include <sys/socket.h>
> >  #include <netinet/in.h>
> > +#include <netinet/tcp.h>
> >  #include <pthread.h>
> >
> >  #include <linux/filter.h>
> > @@ -34,6 +35,30 @@ static void send_byte(int fd)
> >               error(1, errno, "Failed to send single byte");
> >  }
> >
> > +static int wait_for_ack(int fd, int retries)
> > +{
> > +     struct tcp_info info;
> > +     socklen_t optlen;
> > +     int i, err;
> > +
> > +     for (i = 0; i < retries; i++) {
> > +             optlen = sizeof(info);
> > +             err = getsockopt(fd, SOL_TCP, TCP_INFO, &info, &optlen);
> > +             if (err < 0) {
> > +                     log_err("Failed to lookup TCP stats");
> > +                     return err;
> > +             }
> > +
> > +             if (info.tcpi_unacked == 0)
> > +                     return 0;
> > +
> > +             sleep(1);
> Isn't it too big of a hammer? Maybe usleep(10) here and do x100 retries
> instead?
>
I guess this is more consistent with the time we expect to wait for an
ACK in the test, will change and send out a v2 shortly. Thank you for
your quick review!
> > +     }
> > +
> > +     log_err("Did not receive ACK");
> > +     return -1;
> > +}
> > +
> >  static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
> >                    __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
> >                    __u32 icsk_retransmits)
> > @@ -149,6 +174,11 @@ static int run_test(int cgroup_fd, int server_fd)
> >                        /*icsk_retransmits=*/0);
> >
> >       send_byte(client_fd);
> > +     if (wait_for_ack(client_fd, 5) < 0) {
> > +             err = -1;
> > +             goto close_client_fd;
> > +     }
> > +
> >
> >       err += verify_sk(map_fd, client_fd, "first payload byte",
> >                        /*invoked=*/2,
> > @@ -157,6 +187,7 @@ static int run_test(int cgroup_fd, int server_fd)
> >                        /*delivered_ce=*/0,
> >                        /*icsk_retransmits=*/0);
> >
> > +close_client_fd:
> >       close(client_fd);
> >
> >  close_bpf_object:
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >

Patch
diff mbox series

diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
index 90c3862f74a8..2b4754473956 100644
--- a/tools/testing/selftests/bpf/test_tcp_rtt.c
+++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
@@ -6,6 +6,7 @@ 
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <netinet/tcp.h>
 #include <pthread.h>
 
 #include <linux/filter.h>
@@ -34,6 +35,30 @@  static void send_byte(int fd)
 		error(1, errno, "Failed to send single byte");
 }
 
+static int wait_for_ack(int fd, int retries)
+{
+	struct tcp_info info;
+	socklen_t optlen;
+	int i, err;
+
+	for (i = 0; i < retries; i++) {
+		optlen = sizeof(info);
+		err = getsockopt(fd, SOL_TCP, TCP_INFO, &info, &optlen);
+		if (err < 0) {
+			log_err("Failed to lookup TCP stats");
+			return err;
+		}
+
+		if (info.tcpi_unacked == 0)
+			return 0;
+
+		sleep(1);
+	}
+
+	log_err("Did not receive ACK");
+	return -1;
+}
+
 static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
 		     __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
 		     __u32 icsk_retransmits)
@@ -149,6 +174,11 @@  static int run_test(int cgroup_fd, int server_fd)
 			 /*icsk_retransmits=*/0);
 
 	send_byte(client_fd);
+	if (wait_for_ack(client_fd, 5) < 0) {
+		err = -1;
+		goto close_client_fd;
+	}
+
 
 	err += verify_sk(map_fd, client_fd, "first payload byte",
 			 /*invoked=*/2,
@@ -157,6 +187,7 @@  static int run_test(int cgroup_fd, int server_fd)
 			 /*delivered_ce=*/0,
 			 /*icsk_retransmits=*/0);
 
+close_client_fd:
 	close(client_fd);
 
 close_bpf_object: