[PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Kui-Feng Lee]

Run tcpdump in the background for flaky test cases related to network
features.

We have some flaky test cases that are difficult to debug without
knowing what the traffic looks like. With the log printed by tcpdump,
the CI log may help developers to fix these flaky test cases.

This patch set monitors a few test cases. Recently, they have been
showing flaky behavior. If these test cases fail, they will report a
traffic log.

At the beginning and the end of a traffic log, there are additional
traffic packets used for synchronization between the test cases and
the tcpdump process. These packets consist of UDP packets sent to
127.0.0.241:4321 and ICMP unreachable messages for this
destination. For instance, the first two and the last two packets
serve as synchronization packets in the following log.

    15:04:08.586368 lo    In  IP 127.0.0.1.58904 > 127.0.0.241.4321: UDP, length 5
    15:04:08.586435 lo    In  IP 127.0.0.241 > 127.0.0.1: ICMP 127.0.0.241 udp port 4321 unreachable, length 41
    15:04:08.704526 lo    In  IP6 ::1.52053 > ::1.45070: UDP, length 8
    15:04:08.722785 lo    In  IP 127.0.0.1.51863 > 127.0.0.241.4321: UDP, length 15
    15:04:08.722856 lo    In  IP 127.0.0.241 > 127.0.0.1: ICMP 127.0.0.241 udp port 4321 unreachable, length 51 

The IP address 127.0.0.241 is used for synchronization, so the
loopback interface "lo" should be up in the network namespace where
the test is being conducted. While not ideal, this should suffice for
testing purposes.

The following block is an example that monitors the network traffic of
a test case. This test is running in the network namespace
"testns". You can pass NULL to traffic_monitor_start() if the entire
test, from traffic_monitor_start() to traffic_monitor_stop(), is
running in the same namespace.

    struct tmonitor_ctx *tmon;
    
    ...
    tmon = traffic_monitor_start("testns");
    ASSERT_TRUE(tmon, "traffic_monitor_start");
    
    ... test ...
    
    /* Report the traffic log only if there is one or more errors. */
    if (env.subtest_state->error_cnt)
        traffic_monitor_report(tmon);
    traffic_monitor_stop(tmon);

traffic_monitor_start() may fail, but we just ignore it since the
failure doesn't affect the following test.  This tracking feature
takes another 60ms for each test with qemu on my test environment.

Kui-Feng Lee (4):
  selftests/bpf: Add traffic monitor functions.
  selftests/bpf: Monitor traffic for tc_redirect/tc_redirect_dtime.
  selftests/bpf: Monitor traffic for sockmap_listen.
  selftests/bpf: Monitor traffic for select_reuseport.

 tools/testing/selftests/bpf/network_helpers.c | 244 ++++++++++++++++++
 tools/testing/selftests/bpf/network_helpers.h |   5 +
 .../bpf/prog_tests/select_reuseport.c         |   9 +
 .../selftests/bpf/prog_tests/sockmap_listen.c |  10 +
 .../selftests/bpf/prog_tests/tc_redirect.c    |   7 +
 5 files changed, 275 insertions(+)

-- 
2.34.1

[PATCH bpf-next 1/4] selftests/bpf: Add traffic monitor functions.

[Kui-Feng Lee]

Add functions that run tcpdump in the background, report the traffic log
captured by tcpdump, and stop tcpdump. They are supposed to be used for
debugging flaky network test cases. A monitored test case should call
traffic_monitor_start() to start a tcpdump process in the background for a
given namespace, call traffic_monitor_report() to print the log from
tcpdump, and call traffic_monitor_stop() to shutdown the tcpdump process.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/bpf/network_helpers.c | 244 ++++++++++++++++++
 tools/testing/selftests/bpf/network_helpers.h |   5 +
 2 files changed, 249 insertions(+)

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index 44c2c8fa542a..cf0e03f3b95c 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -12,6 +12,8 @@
 #include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/un.h>
+#include <sys/types.h>
+#include <sys/stat.h>
 
 #include <linux/err.h>
 #include <linux/in.h>
@@ -575,6 +577,248 @@ int set_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param)
 	return 0;
 }
 
+struct tmonitor_ctx {
+	pid_t pid;
+	const char *netns;
+	char log_name[PATH_MAX];
+};
+
+/* Make sure that tcpdump has handled all previous packets.
+ *
+ * Send one or more UDP packets to the loopback interface. The packet
+ * contains a mark string. The mark is used to check if tcpdump has handled
+ * the packet. The function waits for tcpdump to print a message for the
+ * packet containing the mark (by checking the payload length and the
+ * destination). This is not a perfect solution, but it should be enough
+ * for testing purposes.
+ *
+ * log_name is the file name where tcpdump writes its output.
+ * mark is the string that is sent in the UDP packet.
+ * repeat specifies if the function should send multiple packets.
+ *
+ * Device "lo" should be up in the namespace for this to work.  This
+ * function should be called in the same network namespace as a
+ * tmonitor_ctx created for in order to create a socket for sending mark
+ * packets.
+ */
+static int traffic_monitor_sync(const char *log_name, const char *mark,
+				bool repeat)
+{
+	const int max_loop = 1000; /* 10s */
+	char mark_pkt_pattern[64];
+	struct sockaddr_in addr;
+	int sock, log_fd, rd_pos = 0;
+	int pattern_size;
+	struct stat st;
+	char buf[4096];
+	int send_cnt = repeat ? max_loop : 1;
+	bool found;
+	int i, n;
+
+	sock = socket(AF_INET, SOCK_DGRAM, 0);
+	if (sock < 0) {
+		log_err("Failed to create socket");
+		return -1;
+	}
+
+	/* Check only the destination and the payload length */
+	pattern_size = snprintf(mark_pkt_pattern, sizeof(mark_pkt_pattern),
+				" > 127.0.0.241.4321: UDP, length %ld",
+				strlen(mark));
+
+	addr.sin_family = AF_INET;
+	addr.sin_addr.s_addr = inet_addr("127.0.0.241");
+	addr.sin_port = htons(4321);
+
+	/* Wait for the log file to be created */
+	for (i = 0; i < max_loop; i++) {
+		log_fd = open(log_name, O_RDONLY);
+		if (log_fd >= 0) {
+			fstat(log_fd, &st);
+			rd_pos = st.st_size;
+			break;
+		}
+		usleep(10000);
+	}
+	/* Wait for the mark packet */
+	for (found = false; i < max_loop && !found; i++) {
+		if (send_cnt-- > 0) {
+			/* Send an UDP packet */
+			if (sendto(sock, mark, strlen(mark), 0,
+				   (struct sockaddr *)&addr,
+				   sizeof(addr)) != strlen(mark))
+				log_err("Failed to sendto");
+		}
+
+		usleep(10000);
+		fstat(log_fd, &st);
+		/* Check the content of the log file */
+		while (rd_pos + pattern_size <= st.st_size) {
+			lseek(log_fd, rd_pos, SEEK_SET);
+			n = read(log_fd, buf, sizeof(buf) - 1);
+			if (n < pattern_size)
+				break;
+			buf[n] = 0;
+			if (strstr(buf, mark_pkt_pattern)) {
+				found = true;
+				break;
+			}
+			rd_pos += n - pattern_size + 1;
+		}
+	}
+
+	close(log_fd);
+	close(sock);
+
+	if (!found) {
+		log_err("Waited too long for synchronizing traffic monitor");
+		return -1;
+	}
+
+	return 0;
+}
+
+/* Start a tcpdump process to monitor traffic.
+ *
+ * netns specifies what network namespace you want to monitor. It will
+ * monitor the current namespace if netns is NULL.
+ */
+struct tmonitor_ctx *traffic_monitor_start(const char *netns)
+{
+	struct tmonitor_ctx *ctx = NULL;
+	struct nstoken *nstoken = NULL;
+	char log_name[PATH_MAX];
+	int status, log_fd;
+	pid_t pid;
+
+	if (netns) {
+		nstoken = open_netns(netns);
+		if (!nstoken)
+			return NULL;
+	}
+
+	pid = fork();
+	if (pid < 0) {
+		log_err("Failed to fork");
+		goto error;
+	}
+
+	if (pid == 0) {
+		/* Child */
+		pid = getpid();
+		snprintf(log_name, sizeof(log_name), "/tmp/tmon_tcpdump_%d.log", pid);
+		log_fd = open(log_name, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+		dup2(log_fd, STDOUT_FILENO);
+		dup2(log_fd, STDERR_FILENO);
+		if (log_fd != STDOUT_FILENO && log_fd != STDERR_FILENO)
+			close(log_fd);
+
+		/* -n don't convert addresses to hostnames.
+		 *
+		 * --immediate-mode handle captured packets immediately.
+		 *
+		 * -l print messages with line buffer. With this option,
+		 * the output will be written at the end of each line
+		 * rather than when the output buffer is full. This is
+		 * needed to sync with tcpdump efficiently.
+		 */
+		execlp("tcpdump", "tcpdump", "-i", "any", "-n", "--immediate-mode", "-l", NULL);
+		log_err("Failed to exec tcpdump");
+		exit(1);
+	}
+
+	ctx = malloc(sizeof(*ctx));
+	if (!ctx) {
+		log_err("Failed to malloc ctx");
+		goto error;
+	}
+
+	ctx->pid = pid;
+	ctx->netns = netns;
+	snprintf(ctx->log_name, sizeof(ctx->log_name), "/tmp/tmon_tcpdump_%d.log", pid);
+
+	/* Wait for tcpdump to be ready */
+	if (traffic_monitor_sync(ctx->log_name, "hello", true)) {
+		status = 0;
+		if (waitpid(pid, &status, WNOHANG) >= 0 &&
+		    !WIFEXITED(status) && !WIFSIGNALED(status))
+			log_err("Wait too long for tcpdump");
+		else
+			log_err("Fail to start tcpdump");
+		goto error;
+	}
+
+	close_netns(nstoken);
+
+	return ctx;
+
+error:
+	close_netns(nstoken);
+	if (pid > 0) {
+		kill(pid, SIGTERM);
+		waitpid(pid, NULL, 0);
+		snprintf(log_name, sizeof(log_name), "/tmp/tmon_tcpdump_%d.log", pid);
+		unlink(log_name);
+	}
+	free(ctx);
+
+	return NULL;
+}
+
+void traffic_monitor_stop(struct tmonitor_ctx *ctx)
+{
+	if (!ctx)
+		return;
+	kill(ctx->pid, SIGTERM);
+	/* Wait the tcpdump process in case that the log file is created
+	 * after this line.
+	 */
+	waitpid(ctx->pid, NULL, 0);
+	unlink(ctx->log_name);
+	free(ctx);
+}
+
+/* Report the traffic monitored by tcpdump.
+ *
+ * The function reads the log file created by tcpdump and writes the
+ * content to stderr.
+ */
+void traffic_monitor_report(struct tmonitor_ctx *ctx)
+{
+	struct nstoken *nstoken = NULL;
+	char buf[4096];
+	int log_fd, n;
+
+	if (!ctx)
+		return;
+
+	/* Make sure all previous packets have been handled by
+	 * tcpdump.
+	 */
+	if (ctx->netns) {
+		nstoken = open_netns(ctx->netns);
+		if (!nstoken) {
+			log_err("Failed to open netns: %s", ctx->netns);
+			goto out;
+		}
+	}
+	traffic_monitor_sync(ctx->log_name, "sync for report", false);
+	close_netns(nstoken);
+
+	/* Read the log file and write to stderr */
+	log_fd = open(ctx->log_name, O_RDONLY);
+	if (log_fd < 0) {
+		log_err("Failed to open log file");
+		return;
+	}
+
+	while ((n = read(log_fd, buf, sizeof(buf))) > 0)
+		fwrite(buf, n, 1, stderr);
+
+out:
+	close(log_fd);
+}
+
 struct send_recv_arg {
 	int		fd;
 	uint32_t	bytes;
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 9ea36524b9db..d757e495fb39 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -72,6 +72,11 @@ int get_socket_local_port(int sock_fd);
 int get_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param);
 int set_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param);
 
+struct tmonitor_ctx;
+struct tmonitor_ctx *traffic_monitor_start(const char *netns);
+void traffic_monitor_stop(struct tmonitor_ctx *ctx);
+void traffic_monitor_report(struct tmonitor_ctx *ctx);
+
 struct nstoken;
 /**
  * open_netns() - Switch to specified network namespace by name.
-- 
2.34.1

[PATCH bpf-next 2/4] selftests/bpf: Monitor traffic for tc_redirect/tc_redirect_dtime.

[Kui-Feng Lee]

Enable traffic monitoring for the test case tc_redirect/tc_redirect_dtime.
The traffic log is only printed when the subtest fails.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
index 327d51f59142..345f8ce93b29 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
@@ -900,6 +900,7 @@ static void test_udp_dtime(struct test_tc_dtime *skel, int family, bool bpf_fwd)
 static void test_tc_redirect_dtime(struct netns_setup_result *setup_result)
 {
 	struct test_tc_dtime *skel;
+	struct tmonitor_ctx *tmon = NULL;
 	struct nstoken *nstoken;
 	int hold_tstamp_fd, err;
 
@@ -934,6 +935,9 @@ static void test_tc_redirect_dtime(struct netns_setup_result *setup_result)
 	if (!ASSERT_OK(err, "disable forwarding"))
 		goto done;
 
+	tmon = traffic_monitor_start(NS_DST);
+	ASSERT_NEQ(tmon, NULL, "traffic_monitor_start");
+
 	test_tcp_clear_dtime(skel);
 
 	test_tcp_dtime(skel, AF_INET, true);
@@ -958,6 +962,9 @@ static void test_tc_redirect_dtime(struct netns_setup_result *setup_result)
 	test_udp_dtime(skel, AF_INET6, false);
 
 done:
+	if (env.subtest_state->error_cnt)
+		traffic_monitor_report(tmon);
+	traffic_monitor_stop(tmon);
 	test_tc_dtime__destroy(skel);
 	close(hold_tstamp_fd);
 }
-- 
2.34.1

[PATCH bpf-next 3/4] selftests/bpf: Monitor traffic for sockmap_listen.

[Kui-Feng Lee]

Enable traffic monitor for each subtest of sockmap_listen.
A subtest prints the traffic log only if it fails.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 .../testing/selftests/bpf/prog_tests/sockmap_listen.c  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index e91b59366030..617d73671a90 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -28,6 +28,7 @@
 #include "test_sockmap_listen.skel.h"
 
 #include "sockmap_helpers.h"
+#include "network_helpers.h"
 
 static void test_insert_invalid(struct test_sockmap_listen *skel __always_unused,
 				int family, int sotype, int mapfd)
@@ -1893,14 +1894,23 @@ static void test_udp_unix_redir(struct test_sockmap_listen *skel, struct bpf_map
 {
 	const char *family_name, *map_name;
 	char s[MAX_TEST_NAME];
+	struct tmonitor_ctx *tmon;
 
 	family_name = family_str(family);
 	map_name = map_type_str(map);
 	snprintf(s, sizeof(s), "%s %s %s", map_name, family_name, __func__);
 	if (!test__start_subtest(s))
 		return;
+
+	tmon = traffic_monitor_start(NULL);
+	ASSERT_TRUE(tmon, "traffic_monitor_start");
+
 	inet_unix_skb_redir_to_connected(skel, map, family);
 	unix_inet_skb_redir_to_connected(skel, map, family);
+
+	if (env.subtest_state->error_cnt)
+		traffic_monitor_report(tmon);
+	traffic_monitor_stop(tmon);
 }
 
 static void run_tests(struct test_sockmap_listen *skel, struct bpf_map *map,
-- 
2.34.1

[PATCH bpf-next 4/4] selftests/bpf: Monitor traffic for select_reuseport.

[Kui-Feng Lee]

Enable traffic monitoring for the subtests of select_reuseport.
The subtest prints the traffic log only ifit fails.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 .../testing/selftests/bpf/prog_tests/select_reuseport.c  | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
index 64c5f5eb2994..567e8083e7cf 100644
--- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
+++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
@@ -22,6 +22,7 @@
 
 #include "test_progs.h"
 #include "test_select_reuseport_common.h"
+#include "network_helpers.h"
 
 #define MAX_TEST_NAME 80
 #define MIN_TCPHDR_LEN 20
@@ -795,6 +796,7 @@ static void test_config(int sotype, sa_family_t family, bool inany)
 	};
 	char s[MAX_TEST_NAME];
 	const struct test *t;
+	struct tmonitor_ctx *tmon;
 
 	for (t = tests; t < tests + ARRAY_SIZE(tests); t++) {
 		if (t->need_sotype && t->need_sotype != sotype)
@@ -808,9 +810,16 @@ static void test_config(int sotype, sa_family_t family, bool inany)
 		if (!test__start_subtest(s))
 			continue;
 
+		tmon = traffic_monitor_start(NULL);
+		ASSERT_TRUE(tmon, "traffic_monitor_start");
+
 		setup_per_test(sotype, family, inany, t->no_inner_map);
 		t->fn(sotype, family);
 		cleanup_per_test(t->no_inner_map);
+
+		if (env.subtest_state->error_cnt)
+			traffic_monitor_report(tmon);
+		traffic_monitor_stop(tmon);
 	}
 }
 
-- 
2.34.1

Re: [PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Kui-Feng Lee]

It fails on CI. Looks like we don't have tcpdump installed on CI!


On 7/12/24 22:55, Kui-Feng Lee wrote:
> Run tcpdump in the background for flaky test cases related to network
> features.
> 
> We have some flaky test cases that are difficult to debug without
> knowing what the traffic looks like. With the log printed by tcpdump,
> the CI log may help developers to fix these flaky test cases.
> 
> This patch set monitors a few test cases. Recently, they have been
> showing flaky behavior. If these test cases fail, they will report a
> traffic log.
> 
> At the beginning and the end of a traffic log, there are additional
> traffic packets used for synchronization between the test cases and
> the tcpdump process. These packets consist of UDP packets sent to
> 127.0.0.241:4321 and ICMP unreachable messages for this
> destination. For instance, the first two and the last two packets
> serve as synchronization packets in the following log.
> 
>      15:04:08.586368 lo    In  IP 127.0.0.1.58904 > 127.0.0.241.4321: UDP, length 5
>      15:04:08.586435 lo    In  IP 127.0.0.241 > 127.0.0.1: ICMP 127.0.0.241 udp port 4321 unreachable, length 41
>      15:04:08.704526 lo    In  IP6 ::1.52053 > ::1.45070: UDP, length 8
>      15:04:08.722785 lo    In  IP 127.0.0.1.51863 > 127.0.0.241.4321: UDP, length 15
>      15:04:08.722856 lo    In  IP 127.0.0.241 > 127.0.0.1: ICMP 127.0.0.241 udp port 4321 unreachable, length 51
> 
> The IP address 127.0.0.241 is used for synchronization, so the
> loopback interface "lo" should be up in the network namespace where
> the test is being conducted. While not ideal, this should suffice for
> testing purposes.
> 
> The following block is an example that monitors the network traffic of
> a test case. This test is running in the network namespace
> "testns". You can pass NULL to traffic_monitor_start() if the entire
> test, from traffic_monitor_start() to traffic_monitor_stop(), is
> running in the same namespace.
> 
>      struct tmonitor_ctx *tmon;
>      
>      ...
>      tmon = traffic_monitor_start("testns");
>      ASSERT_TRUE(tmon, "traffic_monitor_start");
>      
>      ... test ...
>      
>      /* Report the traffic log only if there is one or more errors. */
>      if (env.subtest_state->error_cnt)
>          traffic_monitor_report(tmon);
>      traffic_monitor_stop(tmon);
> 
> traffic_monitor_start() may fail, but we just ignore it since the
> failure doesn't affect the following test.  This tracking feature
> takes another 60ms for each test with qemu on my test environment.
> 
> Kui-Feng Lee (4):
>    selftests/bpf: Add traffic monitor functions.
>    selftests/bpf: Monitor traffic for tc_redirect/tc_redirect_dtime.
>    selftests/bpf: Monitor traffic for sockmap_listen.
>    selftests/bpf: Monitor traffic for select_reuseport.
> 
>   tools/testing/selftests/bpf/network_helpers.c | 244 ++++++++++++++++++
>   tools/testing/selftests/bpf/network_helpers.h |   5 +
>   .../bpf/prog_tests/select_reuseport.c         |   9 +
>   .../selftests/bpf/prog_tests/sockmap_listen.c |  10 +
>   .../selftests/bpf/prog_tests/tc_redirect.c    |   7 +
>   5 files changed, 275 insertions(+)
> 

Re: [PATCH bpf-next 1/4] selftests/bpf: Add traffic monitor functions.

[Kui-Feng Lee]

On 7/12/24 22:55, Kui-Feng Lee wrote:
> Add functions that run tcpdump in the background, report the traffic log
> captured by tcpdump, and stop tcpdump. They are supposed to be used for
> debugging flaky network test cases. A monitored test case should call
> traffic_monitor_start() to start a tcpdump process in the background for a
> given namespace, call traffic_monitor_report() to print the log from
> tcpdump, and call traffic_monitor_stop() to shutdown the tcpdump process.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   tools/testing/selftests/bpf/network_helpers.c | 244 ++++++++++++++++++
>   tools/testing/selftests/bpf/network_helpers.h |   5 +
>   2 files changed, 249 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index 44c2c8fa542a..cf0e03f3b95c 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -12,6 +12,8 @@
>   #include <sys/mount.h>
>   #include <sys/stat.h>
>   #include <sys/un.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
>   
>   #include <linux/err.h>
>   #include <linux/in.h>
> @@ -575,6 +577,248 @@ int set_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param)
>   	return 0;
>   }
>   
> +struct tmonitor_ctx {
> +	pid_t pid;
> +	const char *netns;
> +	char log_name[PATH_MAX];
> +};
> +
> +/* Make sure that tcpdump has handled all previous packets.
> + *
> + * Send one or more UDP packets to the loopback interface. The packet
> + * contains a mark string. The mark is used to check if tcpdump has handled
> + * the packet. The function waits for tcpdump to print a message for the
> + * packet containing the mark (by checking the payload length and the
> + * destination). This is not a perfect solution, but it should be enough
> + * for testing purposes.
> + *
> + * log_name is the file name where tcpdump writes its output.
> + * mark is the string that is sent in the UDP packet.
> + * repeat specifies if the function should send multiple packets.
> + *
> + * Device "lo" should be up in the namespace for this to work.  This
> + * function should be called in the same network namespace as a
> + * tmonitor_ctx created for in order to create a socket for sending mark
> + * packets.
> + */
> +static int traffic_monitor_sync(const char *log_name, const char *mark,
> +				bool repeat)
> +{
> +	const int max_loop = 1000; /* 10s */
> +	char mark_pkt_pattern[64];
> +	struct sockaddr_in addr;
> +	int sock, log_fd, rd_pos = 0;
> +	int pattern_size;
> +	struct stat st;
> +	char buf[4096];
> +	int send_cnt = repeat ? max_loop : 1;
> +	bool found;
> +	int i, n;
> +
> +	sock = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (sock < 0) {
> +		log_err("Failed to create socket");
> +		return -1;
> +	}
> +
> +	/* Check only the destination and the payload length */
> +	pattern_size = snprintf(mark_pkt_pattern, sizeof(mark_pkt_pattern),
> +				" > 127.0.0.241.4321: UDP, length %ld",
> +				strlen(mark));
> +
> +	addr.sin_family = AF_INET;
> +	addr.sin_addr.s_addr = inet_addr("127.0.0.241");
> +	addr.sin_port = htons(4321);
> +
> +	/* Wait for the log file to be created */
> +	for (i = 0; i < max_loop; i++) {
> +		log_fd = open(log_name, O_RDONLY);
> +		if (log_fd >= 0) {
> +			fstat(log_fd, &st);
> +			rd_pos = st.st_size;
> +			break;
> +		}
> +		usleep(10000);
> +	}
> +	/* Wait for the mark packet */
> +	for (found = false; i < max_loop && !found; i++) {
> +		if (send_cnt-- > 0) {
> +			/* Send an UDP packet */
> +			if (sendto(sock, mark, strlen(mark), 0,
> +				   (struct sockaddr *)&addr,
> +				   sizeof(addr)) != strlen(mark))
> +				log_err("Failed to sendto");
> +		}
> +
> +		usleep(10000);
> +		fstat(log_fd, &st);
> +		/* Check the content of the log file */
> +		while (rd_pos + pattern_size <= st.st_size) {
> +			lseek(log_fd, rd_pos, SEEK_SET);
> +			n = read(log_fd, buf, sizeof(buf) - 1);
> +			if (n < pattern_size)
> +				break;
> +			buf[n] = 0;
> +			if (strstr(buf, mark_pkt_pattern)) {
> +				found = true;
> +				break;
> +			}
> +			rd_pos += n - pattern_size + 1;
> +		}
> +	}
> +
> +	close(log_fd);
> +	close(sock);
> +
> +	if (!found) {
> +		log_err("Waited too long for synchronizing traffic monitor");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Start a tcpdump process to monitor traffic.
> + *
> + * netns specifies what network namespace you want to monitor. It will
> + * monitor the current namespace if netns is NULL.
> + */
> +struct tmonitor_ctx *traffic_monitor_start(const char *netns)
> +{
> +	struct tmonitor_ctx *ctx = NULL;
> +	struct nstoken *nstoken = NULL;
> +	char log_name[PATH_MAX];
> +	int status, log_fd;
> +	pid_t pid;
> +
> +	if (netns) {
> +		nstoken = open_netns(netns);
> +		if (!nstoken)
> +			return NULL;
> +	}
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		log_err("Failed to fork");
> +		goto error;
> +	}
> +
> +	if (pid == 0) {
> +		/* Child */
> +		pid = getpid();
> +		snprintf(log_name, sizeof(log_name), "/tmp/tmon_tcpdump_%d.log", pid);
> +		log_fd = open(log_name, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> +		dup2(log_fd, STDOUT_FILENO);
> +		dup2(log_fd, STDERR_FILENO);
> +		if (log_fd != STDOUT_FILENO && log_fd != STDERR_FILENO)
> +			close(log_fd);
> +
> +		/* -n don't convert addresses to hostnames.
> +		 *
> +		 * --immediate-mode handle captured packets immediately.
> +		 *
> +		 * -l print messages with line buffer. With this option,
> +		 * the output will be written at the end of each line
> +		 * rather than when the output buffer is full. This is
> +		 * needed to sync with tcpdump efficiently.
> +		 */
> +		execlp("tcpdump", "tcpdump", "-i", "any", "-n", "--immediate-mode", "-l", NULL);
> +		log_err("Failed to exec tcpdump");
> +		exit(1);
> +	}
> +
> +	ctx = malloc(sizeof(*ctx));
> +	if (!ctx) {
> +		log_err("Failed to malloc ctx");
> +		goto error;
> +	}
> +
> +	ctx->pid = pid;
> +	ctx->netns = netns;
> +	snprintf(ctx->log_name, sizeof(ctx->log_name), "/tmp/tmon_tcpdump_%d.log", pid);
> +
> +	/* Wait for tcpdump to be ready */
> +	if (traffic_monitor_sync(ctx->log_name, "hello", true)) {
> +		status = 0;
> +		if (waitpid(pid, &status, WNOHANG) >= 0 &&
> +		    !WIFEXITED(status) && !WIFSIGNALED(status))
> +			log_err("Wait too long for tcpdump");
> +		else
> +			log_err("Fail to start tcpdump");
> +		goto error;
> +	}
> +
> +	close_netns(nstoken);
> +
> +	return ctx;
> +
> +error:
> +	close_netns(nstoken);
> +	if (pid > 0) {
> +		kill(pid, SIGTERM);
> +		waitpid(pid, NULL, 0);
> +		snprintf(log_name, sizeof(log_name), "/tmp/tmon_tcpdump_%d.log", pid);
> +		unlink(log_name);
> +	}
> +	free(ctx);
> +
> +	return NULL;
> +}
> +
> +void traffic_monitor_stop(struct tmonitor_ctx *ctx)
> +{
> +	if (!ctx)
> +		return;
> +	kill(ctx->pid, SIGTERM);
> +	/* Wait the tcpdump process in case that the log file is created
> +	 * after this line.
> +	 */
> +	waitpid(ctx->pid, NULL, 0);
> +	unlink(ctx->log_name);
> +	free(ctx);
> +}
> +
> +/* Report the traffic monitored by tcpdump.
> + *
> + * The function reads the log file created by tcpdump and writes the
> + * content to stderr.
> + */
> +void traffic_monitor_report(struct tmonitor_ctx *ctx)
> +{
> +	struct nstoken *nstoken = NULL;
> +	char buf[4096];
> +	int log_fd, n;

log_fd should be initialized. I will fix it in the next version.

> +
> +	if (!ctx)
> +		return;
> +
> +	/* Make sure all previous packets have been handled by
> +	 * tcpdump.
> +	 */
> +	if (ctx->netns) {
> +		nstoken = open_netns(ctx->netns);
> +		if (!nstoken) {
> +			log_err("Failed to open netns: %s", ctx->netns);
> +			goto out;
> +		}
> +	}
> +	traffic_monitor_sync(ctx->log_name, "sync for report", false);
> +	close_netns(nstoken);
> +
> +	/* Read the log file and write to stderr */
> +	log_fd = open(ctx->log_name, O_RDONLY);
> +	if (log_fd < 0) {
> +		log_err("Failed to open log file");
> +		return;
> +	}
> +
> +	while ((n = read(log_fd, buf, sizeof(buf))) > 0)
> +		fwrite(buf, n, 1, stderr);
> +
> +out:
> +	close(log_fd);
> +}
> +
>   struct send_recv_arg {
>   	int		fd;
>   	uint32_t	bytes;
> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> index 9ea36524b9db..d757e495fb39 100644
> --- a/tools/testing/selftests/bpf/network_helpers.h
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -72,6 +72,11 @@ int get_socket_local_port(int sock_fd);
>   int get_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param);
>   int set_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param);
>   
> +struct tmonitor_ctx;
> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
> +void traffic_monitor_report(struct tmonitor_ctx *ctx);
> +
>   struct nstoken;
>   /**
>    * open_netns() - Switch to specified network namespace by name.

Re: [PATCH bpf-next 1/4] selftests/bpf: Add traffic monitor functions.

[kernel test robot]

Hi Kui-Feng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Kui-Feng-Lee/selftests-bpf-Add-traffic-monitor-functions/20240713-140129
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20240713055552.2482367-2-thinker.li%40gmail.com
patch subject: [PATCH bpf-next 1/4] selftests/bpf: Add traffic monitor functions.
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202407132213.2rer2R4r-lkp@intel.com/

includecheck warnings: (new ones prefixed by >>)
>> tools/testing/selftests/bpf/network_helpers.c: sys/stat.h is included more than once.

vim +13 tools/testing/selftests/bpf/network_helpers.c

    10	
    11	#include <arpa/inet.h>
    12	#include <sys/mount.h>
  > 13	#include <sys/stat.h>
    14	#include <sys/un.h>
    15	#include <sys/types.h>
  > 16	#include <sys/stat.h>
    17	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Stanislav Fomichev]

On 07/12, Kui-Feng Lee wrote:
> Run tcpdump in the background for flaky test cases related to network
> features.

Have you considered linking against libpcap instead of shelling out
to tcpdump? As long as we have this lib installed on the runners
(likely?) that should be a bit cleaner than doing tcpdump.. WDYT?

Re: [PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Kui-Feng Lee]

On 7/15/24 14:33, Stanislav Fomichev wrote:
> On 07/12, Kui-Feng Lee wrote:
>> Run tcpdump in the background for flaky test cases related to network
>> features.
> 
> Have you considered linking against libpcap instead of shelling out
> to tcpdump? As long as we have this lib installed on the runners
> (likely?) that should be a bit cleaner than doing tcpdump.. WDYT?

I just checked the script building the root image for vmtest. [1]
It doesn't install libpcap.

If our approach is to capture the packets in a file, and let developers
download the file, it would be a simple and straight forward solution.
If we want a log in text, it would be more complicated to parse
packets.

Martin & Stanislay,

WDYT about capture packets in a file and using libpcap directly?
Developers can download the file and parse it with tcpdump locally.

[1] https://github.com/libbpf/ci/blob/main/rootfs/mkrootfs_debian.sh

Re: [PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Martin KaFai Lau]

On 7/15/24 3:07 PM, Kui-Feng Lee wrote:
> 
> 
> On 7/15/24 14:33, Stanislav Fomichev wrote:
>> On 07/12, Kui-Feng Lee wrote:
>>> Run tcpdump in the background for flaky test cases related to network
>>> features.
>>
>> Have you considered linking against libpcap instead of shelling out
>> to tcpdump? As long as we have this lib installed on the runners
>> (likely?) that should be a bit cleaner than doing tcpdump.. WDYT?
> 
> I just checked the script building the root image for vmtest. [1]
> It doesn't install libpcap.
> 
> If our approach is to capture the packets in a file, and let developers
> download the file, it would be a simple and straight forward solution.
> If we want a log in text, it would be more complicated to parse
> packets.
> 
> Martin & Stanislay,
> 
> WDYT about capture packets in a file and using libpcap directly?
> Developers can download the file and parse it with tcpdump locally.

thinking out loud...

Re: libpcap (instead of tcpdump) part. I am not very experienced in libpcap. I 
don't have a strong preference. I do hope patch 1 could be more straight forward 
that no need to use loops and artificial udp packets to ensure the tcpdump is 
fully ready to capture. I assume using libpcap can make this sync part 
easier/cleaner (pthread_cond?) and not too much code is needed to use libpcap?

Re: dump to file and download by developer. If I read patch 1 correctly, it only 
dumps everything at the end of the test (by calling traffic_monitor_report). 
imo, it lost the chronological ordering with other ASSERT_* logs and does not 
make a big difference (vs downloading as a file).

The developer needs to go through another exercise to figure out (e.g.) a 
captured packet may be related to a ASSERT_* failure by connecting the timestamp 
between the ASSERT_* log and the captured packet (afaik, there is a timestamp in 
the CI raw log). Ideally, the packet can be logged to stderr/out as soon as it 
is captured such that the developer can still sort of relate the packet with the 
other ASSERT_*() log around it.

Re: [PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Kui-Feng Lee]

On 7/15/24 16:56, Martin KaFai Lau wrote:
> On 7/15/24 3:07 PM, Kui-Feng Lee wrote:
>>
>>
>> On 7/15/24 14:33, Stanislav Fomichev wrote:
>>> On 07/12, Kui-Feng Lee wrote:
>>>> Run tcpdump in the background for flaky test cases related to network
>>>> features.
>>>
>>> Have you considered linking against libpcap instead of shelling out
>>> to tcpdump? As long as we have this lib installed on the runners
>>> (likely?) that should be a bit cleaner than doing tcpdump.. WDYT?
>>
>> I just checked the script building the root image for vmtest. [1]
>> It doesn't install libpcap.
>>
>> If our approach is to capture the packets in a file, and let developers
>> download the file, it would be a simple and straight forward solution.
>> If we want a log in text, it would be more complicated to parse
>> packets.
>>
>> Martin & Stanislay,
>>
>> WDYT about capture packets in a file and using libpcap directly?
>> Developers can download the file and parse it with tcpdump locally.
> 
> thinking out loud...
> 
> Re: libpcap (instead of tcpdump) part. I am not very experienced in 
> libpcap. I don't have a strong preference. I do hope patch 1 could be 
> more straight forward that no need to use loops and artificial udp 
> packets to ensure the tcpdump is fully ready to capture. I assume using 
> libpcap can make this sync part easier/cleaner (pthread_cond?) and not 
> too much code is needed to use libpcap?

Yes, it would be easier and cleaner if we don't parse the payload
of packets.

> 
> Re: dump to file and download by developer. If I read patch 1 correctly, 
> it only dumps everything at the end of the test (by calling 
> traffic_monitor_report). imo, it lost the chronological ordering with 
> other ASSERT_* logs and does not make a big difference (vs downloading 
> as a file).
> 
> The developer needs to go through another exercise to figure out (e.g.) 
> a captured packet may be related to a ASSERT_* failure by connecting the 
> timestamp between the ASSERT_* log and the captured packet (afaik, there 
> is a timestamp in the CI raw log). Ideally, the packet can be logged to 
> stderr/out as soon as it is captured such that the developer can still 
> sort of relate the packet with the other ASSERT_*() log around it.
> 

We can print an ordered number for each received packets to stderr asap
and use libpcap without parsing packets. Developers use tcpdump or
wireshark to parse packets.

Or, we can just run tcpdump on background and let it write to stderr
directly. This is convenient for developers. However, we still need to
wait for tcpdump ready.

Another hybrid solution is to have a libpcap thread to capture packets 
and feed packets to tcpdump through a pipe to parse packets. With this,
we don't need to wait for tcpdump anymore. However, I am not sure
how long tcpdump gets ready. It may make all these efforts of keeping
order useless. But, I can try it.

Re: [PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Stanislav Fomichev]

On 07/15, Kui-Feng Lee wrote:
> 
> 
> On 7/15/24 16:56, Martin KaFai Lau wrote:
> > On 7/15/24 3:07 PM, Kui-Feng Lee wrote:
> > > 
> > > 
> > > On 7/15/24 14:33, Stanislav Fomichev wrote:
> > > > On 07/12, Kui-Feng Lee wrote:
> > > > > Run tcpdump in the background for flaky test cases related to network
> > > > > features.
> > > > 
> > > > Have you considered linking against libpcap instead of shelling out
> > > > to tcpdump? As long as we have this lib installed on the runners
> > > > (likely?) that should be a bit cleaner than doing tcpdump.. WDYT?
> > > 
> > > I just checked the script building the root image for vmtest. [1]
> > > It doesn't install libpcap.
> > > 
> > > If our approach is to capture the packets in a file, and let developers
> > > download the file, it would be a simple and straight forward solution.
> > > If we want a log in text, it would be more complicated to parse
> > > packets.
> > > 
> > > Martin & Stanislay,
> > > 
> > > WDYT about capture packets in a file and using libpcap directly?
> > > Developers can download the file and parse it with tcpdump locally.
> > 
> > thinking out loud...
> > 
> > Re: libpcap (instead of tcpdump) part. I am not very experienced in
> > libpcap. I don't have a strong preference. I do hope patch 1 could be
> > more straight forward that no need to use loops and artificial udp
> > packets to ensure the tcpdump is fully ready to capture. I assume using
> > libpcap can make this sync part easier/cleaner (pthread_cond?) and not
> > too much code is needed to use libpcap?
> 
> Yes, it would be easier and cleaner if we don't parse the payload
> of packets.

Yeah, same, no strong preference; was just wondering whether you've
made a conscious choice of not using it because it definitely makes things
a bit easier wrt to the part where you try to sync with tcpdump..

Also +1 on saving the raw file (via libpcap or tcpdump -w). 

Re: [PATCH bpf-next 0/4] monitor network traffic for flaky test cases

[Kui-Feng Lee]

On 7/15/24 20:25, Stanislav Fomichev wrote:
> On 07/15, Kui-Feng Lee wrote:
>>
>>
>> On 7/15/24 16:56, Martin KaFai Lau wrote:
>>> On 7/15/24 3:07 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 7/15/24 14:33, Stanislav Fomichev wrote:
>>>>> On 07/12, Kui-Feng Lee wrote:
>>>>>> Run tcpdump in the background for flaky test cases related to network
>>>>>> features.
>>>>>
>>>>> Have you considered linking against libpcap instead of shelling out
>>>>> to tcpdump? As long as we have this lib installed on the runners
>>>>> (likely?) that should be a bit cleaner than doing tcpdump.. WDYT?
>>>>
>>>> I just checked the script building the root image for vmtest. [1]
>>>> It doesn't install libpcap.
>>>>
>>>> If our approach is to capture the packets in a file, and let developers
>>>> download the file, it would be a simple and straight forward solution.
>>>> If we want a log in text, it would be more complicated to parse
>>>> packets.
>>>>
>>>> Martin & Stanislay,
>>>>
>>>> WDYT about capture packets in a file and using libpcap directly?
>>>> Developers can download the file and parse it with tcpdump locally.
>>>
>>> thinking out loud...
>>>
>>> Re: libpcap (instead of tcpdump) part. I am not very experienced in
>>> libpcap. I don't have a strong preference. I do hope patch 1 could be
>>> more straight forward that no need to use loops and artificial udp
>>> packets to ensure the tcpdump is fully ready to capture. I assume using
>>> libpcap can make this sync part easier/cleaner (pthread_cond?) and not
>>> too much code is needed to use libpcap?
>>
>> Yes, it would be easier and cleaner if we don't parse the payload
>> of packets.
> 
> Yeah, same, no strong preference; was just wondering whether you've
> made a conscious choice of not using it because it definitely makes things
> a bit easier wrt to the part where you try to sync with tcpdump..
> 
> Also +1 on saving the raw file (via libpcap or tcpdump -w).

I agree to do it with libpcap. And, will print a number to stdout/stderr
as an index to the packets in the raw file. However, I need to figure
out how to make the raw file available to developers at first.