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

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

[Kui-Feng Lee]

Capture packets 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. Capturing packets, the CI log and
packet files may help developers to fix these flaky test cases.

This patch set monitors a few test cases. Recently, they have been
showing flaky behavior.

    IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, ifindex 1, SYN
    IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, ifindex 1, SYN, ACK
    IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, ifindex 1, ACK
    IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, ACK
    IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, ifindex 1, FIN, ACK
    IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, RST, ACK
    Packet file: packets-2172-86-select_reuseport:sockhash-test.log
    #280/87  select_reuseport/sockhash IPv4/TCP LOOPBACK test_detach_bpf:OK 

The above block is the log of a test case. It shows every packets of a
connection. The captured packets are stored in the file called
packets-2172-86-select_reuseport:sockhash-test.log.

We have a set of high-level helpers and a test_progs option to
simplify the process of enabling the traffic monitor. netns_new() and
netns_free() are helpers used to create and delete namespaces while
also enabling the traffic monitor for the namespace based on the
patterns provided by the "-m" option of test_progs. The value of the
"-m" option is a list of patterns used to enable the traffic monitor
for a group of tests or a file containing patterns. CI can utilize
this option to enable monitoring.

traffic_monitor_start() and traffic_monitor_stop() are low-level
functions to start monitoring explicitly. You can have more controls,
however high-level helpers are preferred.

The following block is an example that monitors the network traffic of
a test case in a network namespace.

    struct netns_obj *netns;
    
    ...
    netns = netns_new("test", true);
    if (!ASSERT_TRUE(netns, "netns_new"))
        goto err;
    
    ... test ...
    
    netns_free(netns);

netns_new() will create a network namespace named "test". By passing
"true" as the 2nd argument, it will set the network namespace of the
current process to "test".netns_free() will destroy the namespace, and
the process will leave the "test" namespace if the struct netns_obj
returned by netns_new() is created with "true" as the 2nd argument. If
the name of the test matches the patterns given by the "-m" option,
the traffic monitor will be enabled for the "test" namespace as well.

The packet files are located in the directory "/tmp/tmon_pcap/". The
directory is intended to be compressed as a file so that developers
can download it from the CI.

This feature is enabled only if BPF selftests are built with
TRAFFIC_MONITOR variable being defined. For example,

    make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf

This command will enable traffic monitoring for BPF selftests. That
means we have to turn it on to get the log at CI.

---

Changes from v3:

 - Rebase to the latest tip of bpf-next/for-next

 - Change verb back to C string.

Changes from v2:

 - Include pcap header files conditionally.

 - Move the implementation of traffic monitor to test_progs.c.

 - Include test name and namespace as a part of names of packet files.

 - Parse and print ICMP(v4|v6) packets.

 - Add netns_new() and netns_free() to create and delete network
   namespaces.

   - Make tc_redirect, sockmap_listen and select_reuseport test in a
     network namespace.

 - Add the "-m" option to test_progs to enable traffic monitor for the
   tests matching the pattern. CI may use this option to enable
   monitoring for a given set of tests.

Changes from v1:

 - Move to calling libpcap directly to capture packets in a background
   thread.

 - Print parsed packet information for TCP and UDP packets.

v1: https://lore.kernel.org/all/20240713055552.2482367-5-thinker.li@gmail.com/
v2: https://lore.kernel.org/all/20240723182439.1434795-1-thinker.li@gmail.com/
v3: https://lore.kernel.org/all/20240730002745.1484204-1-thinker.li@gmail.com/

Kui-Feng Lee (6):
  selftests/bpf: Add traffic monitor functions.
  selftests/bpf: Add the traffic monitor option to test_progs.
  selftests/bpf: netns_new() and netns_free() helpers.
  selftests/bpf: Monitor traffic for tc_redirect.
  selftests/bpf: Monitor traffic for sockmap_listen.
  selftests/bpf: Monitor traffic for select_reuseport.

 tools/testing/selftests/bpf/Makefile          |   5 +
 tools/testing/selftests/bpf/network_helpers.c |  26 +
 tools/testing/selftests/bpf/network_helpers.h |   2 +
 .../bpf/prog_tests/select_reuseport.c         |  39 +-
 .../selftests/bpf/prog_tests/sockmap_listen.c |   9 +
 .../selftests/bpf/prog_tests/tc_redirect.c    |  33 +-
 tools/testing/selftests/bpf/test_progs.c      | 597 +++++++++++++++++-
 tools/testing/selftests/bpf/test_progs.h      |  22 +
 8 files changed, 678 insertions(+), 55 deletions(-)

-- 
2.34.1

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

[Kui-Feng Lee]

Add functions that capture packets and print log in the background. They
are supposed to be used for debugging flaky network test cases. A monitored
test case should call traffic_monitor_start() to start a thread to capture
packets in the background for a given namespace and call
traffic_monitor_stop() to stop capturing. (Or, option '-m' implemented by
the later patches.)

    IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, ifindex 1, SYN
    IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, ifindex 1, SYN, ACK
    IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, ifindex 1, ACK
    IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, ACK
    IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, ifindex 1, FIN, ACK
    IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, RST, ACK
    Packet file: packets-2172-86-select_reuseport:sockhash-test.log
    #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK test_detach_bpf:OK

The above is the output of an example. It shows the packets of a connection
and the name of the file that contains captured packets in the directory
/tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.

This feature only works if TRAFFIC_MONITOR variable has been passed to
build BPF selftests. For example,

  make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf

This command will build BPF selftests with this feature enabled.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/bpf/Makefile     |   5 +
 tools/testing/selftests/bpf/test_progs.c | 432 +++++++++++++++++++++++
 tools/testing/selftests/bpf/test_progs.h |  16 +
 3 files changed, 453 insertions(+)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 774c6270e377..0a3108311be7 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -41,6 +41,11 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic					\
 LDFLAGS += $(SAN_LDFLAGS)
 LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
 
+ifneq ($(TRAFFIC_MONITOR),)
+LDLIBS += -lpcap
+CFLAGS += -DTRAFFIC_MONITOR=1
+endif
+
 # The following tests perform type punning and they may break strict
 # aliasing rules, which are exploited by both GCC and clang by default
 # while optimizing.  This can lead to broken programs.
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 60fafa2f1ed7..ab54d8420603 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -14,10 +14,25 @@
 #include <netinet/in.h>
 #include <sys/select.h>
 #include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 #include <sys/un.h>
 #include <bpf/btf.h>
 #include "json_writer.h"
 
+#include <linux/ip.h>
+#include <linux/udp.h>
+#include <netinet/tcp.h>
+#include <net/if.h>
+#include "network_helpers.h"
+
+#ifdef TRAFFIC_MONITOR
+/* Prevent pcap.h from including pcap/bpf.h and causing conflicts */
+#define PCAP_DONT_INCLUDE_PCAP_BPF_H 1
+#include <pcap/pcap.h>
+#include <pcap/dlt.h>
+#endif
+
 #ifdef __GLIBC__
 #include <execinfo.h> /* backtrace */
 #endif
@@ -416,6 +431,423 @@ static void restore_netns(void)
 	}
 }
 
+#ifdef TRAFFIC_MONITOR
+struct tmonitor_ctx {
+	pcap_t *pcap;
+	pcap_dumper_t *dumper;
+	pthread_t thread;
+	int wake_fd_r;
+	int wake_fd_w;
+
+	bool done;
+	char pkt_fname[PATH_MAX];
+	int pcap_fd;
+};
+
+/* Is this packet captured with a Ethernet protocol type? */
+static bool is_ethernet(const u_char *packet)
+{
+	u16 arphdr_type;
+
+	memcpy(&arphdr_type, packet + 8, 2);
+	arphdr_type = ntohs(arphdr_type);
+
+	/* Except the following cases, the protocol type contains the
+	 * Ethernet protocol type for the packet.
+	 *
+	 * https://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL2.html
+	 */
+	switch (arphdr_type) {
+	case 770: /* ARPHRD_FRAD */
+	case 778: /* ARPHDR_IPGRE */
+	case 803: /* ARPHRD_IEEE80211_RADIOTAP */
+		return false;
+	}
+	return true;
+}
+
+/* Show the information of the transport layer in the packet */
+static void show_transport(const u_char *packet, u16 len, u32 ifindex,
+			   const char *src_addr, const char *dst_addr,
+			   u16 proto, bool ipv6)
+{
+	struct udphdr *udp;
+	struct tcphdr *tcp;
+	u16 src_port, dst_port;
+	const char *transport_str;
+	char *ifname, _ifname[IF_NAMESIZE];
+
+	ifname = if_indextoname(ifindex, _ifname);
+	if (!ifname) {
+		snprintf(_ifname, sizeof(_ifname), "unknown(%d)", ifindex);
+		ifname = _ifname;
+	}
+
+	if (proto == IPPROTO_UDP) {
+		udp = (struct udphdr *)packet;
+		src_port = ntohs(udp->source);
+		dst_port = ntohs(udp->dest);
+		transport_str = "UDP";
+	} else if (proto == IPPROTO_TCP) {
+		tcp = (struct tcphdr *)packet;
+		src_port = ntohs(tcp->source);
+		dst_port = ntohs(tcp->dest);
+		transport_str = "TCP"
+;
+	} else if (proto == IPPROTO_ICMP) {
+		printf("IPv4 ICMP packet: %s -> %s, len %d, type %d, code %d, ifname %s\n",
+		       src_addr, dst_addr, len, packet[0], packet[1], ifname);
+		return;
+	} else if (proto == IPPROTO_ICMPV6) {
+		printf("IPv6 ICMPv6 packet: %s -> %s, len %d, type %d, code %d, ifname %s\n",
+		       src_addr, dst_addr, len, packet[0], packet[1], ifname);
+		return;
+	} else {
+		printf("%s (proto %d): %s -> %s, ifname %s\n",
+		       ipv6 ? "IPv6" : "IPv4", proto, src_addr, dst_addr, ifname);
+		return;
+	}
+
+	/* TCP */
+
+	if (ipv6)
+		printf("IPv6 %s packet: [%s]:%d -> [%s]:%d, len %d, ifname %s",
+		       transport_str, src_addr, src_port,
+		       dst_addr, dst_port, len, ifname);
+	else
+		printf("IPv4 %s packet: %s:%d -> %s:%d, len %d, ifname %s",
+		       transport_str, src_addr, src_port,
+		       dst_addr, dst_port, len, ifname);
+
+	if (proto == IPPROTO_TCP) {
+		if (tcp->fin)
+			printf(", FIN");
+		if (tcp->syn)
+			printf(", SYN");
+		if (tcp->rst)
+			printf(", RST");
+		if (tcp->ack)
+			printf(", ACK");
+	}
+
+	printf("\n");
+}
+
+static void show_ipv6_packet(const u_char *packet, u32 ifindex)
+{
+	struct ipv6hdr *pkt = (struct ipv6hdr *)packet;
+	struct in6_addr src;
+	struct in6_addr dst;
+	char src_str[INET6_ADDRSTRLEN], dst_str[INET6_ADDRSTRLEN];
+	u_char proto;
+
+	memcpy(&src, &pkt->saddr, sizeof(src));
+	memcpy(&dst, &pkt->daddr, sizeof(dst));
+	inet_ntop(AF_INET6, &src, src_str, sizeof(src_str));
+	inet_ntop(AF_INET6, &dst, dst_str, sizeof(dst_str));
+	proto = pkt->nexthdr;
+	show_transport(packet + sizeof(struct ipv6hdr),
+		       ntohs(pkt->payload_len),
+		       ifindex, src_str, dst_str, proto, true);
+}
+
+static void show_ipv4_packet(const u_char *packet, u32 ifindex)
+{
+	struct iphdr *pkt = (struct iphdr *)packet;
+	struct in_addr src;
+	struct in_addr dst;
+	u_char proto;
+	char src_str[INET_ADDRSTRLEN], dst_str[INET_ADDRSTRLEN];
+
+	memcpy(&src, &pkt->saddr, sizeof(src));
+	memcpy(&dst, &pkt->daddr, sizeof(dst));
+	inet_ntop(AF_INET, &src, src_str, sizeof(src_str));
+	inet_ntop(AF_INET, &dst, dst_str, sizeof(dst_str));
+	proto = pkt->protocol;
+	show_transport(packet + sizeof(struct iphdr),
+		       ntohs(pkt->tot_len),
+		       ifindex, src_str, dst_str, proto, false);
+}
+
+static void *traffic_monitor_thread(void *arg)
+{
+	char *ifname, _ifname[IF_NAMESIZE];
+	const u_char *packet, *payload;
+	struct tmonitor_ctx *ctx = arg;
+	struct pcap_pkthdr header;
+	pcap_t *pcap = ctx->pcap;
+	pcap_dumper_t *dumper = ctx->dumper;
+	int fd = ctx->pcap_fd;
+	int wake_fd = ctx->wake_fd_r;
+	u16 proto;
+	u32 ifindex;
+	fd_set fds;
+	int nfds, r;
+
+	nfds = (fd > wake_fd ? fd : wake_fd) + 1;
+	FD_ZERO(&fds);
+
+	while (!ctx->done) {
+		FD_SET(fd, &fds);
+		FD_SET(wake_fd, &fds);
+		r = select(nfds, &fds, NULL, NULL, NULL);
+		if (!r)
+			continue;
+		if (r < 0) {
+			if (errno == EINTR)
+				continue;
+			log_err("Fail to select on pcap fd and wake fd: %s", strerror(errno));
+			break;
+		}
+
+		packet = pcap_next(pcap, &header);
+		if (!packet)
+			continue;
+
+		/* According to the man page of pcap_dump(), first argument
+		 * is the pcap_dumper_t pointer even it's argument type is
+		 * u_char *.
+		 */
+		pcap_dump((u_char *)dumper, &header, packet);
+
+		/* Not sure what other types of packets look like. Here, we
+		 * parse only Ethernet and compatible packets.
+		 */
+		if (!is_ethernet(packet)) {
+			printf("Packet captured\n");
+			continue;
+		}
+
+		/* Skip SLL2 header
+		 * https://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL2.html
+		 *
+		 * Although the document doesn't mention that, the payload
+		 * doesn't include the Ethernet header. The payload starts
+		 * from the first byte of the network layer header.
+		 */
+		payload = packet + 20;
+
+		memcpy(&proto, packet, 2);
+		proto = ntohs(proto);
+		memcpy(&ifindex, packet + 4, 4);
+		ifindex = ntohl(ifindex);
+
+		if (proto == ETH_P_IPV6) {
+			show_ipv6_packet(payload, ifindex);
+		} else if (proto == ETH_P_IP) {
+			show_ipv4_packet(payload, ifindex);
+		} else {
+			ifname = if_indextoname(ifindex, _ifname);
+			if (!ifname) {
+				snprintf(_ifname, sizeof(_ifname), "unknown(%d)", ifindex);
+				ifname = _ifname;
+			}
+
+			printf("Unknown network protocol type %x, ifname %s\n", proto, ifname);
+		}
+	}
+
+	return NULL;
+}
+
+/* Prepare the pcap handle to capture packets.
+ *
+ * This pcap is non-blocking and immediate mode is enabled to receive
+ * captured packets as soon as possible.  The snaplen is set to 1024 bytes
+ * to limit the size of captured content. The format of the link-layer
+ * header is set to DLT_LINUX_SLL2 to enable handling various link-layer
+ * technologies.
+ */
+static pcap_t *traffic_monitor_prepare_pcap(void)
+{
+	char errbuf[PCAP_ERRBUF_SIZE];
+	pcap_t *pcap;
+	int r;
+
+	/* Listen on all NICs in the namespace */
+	pcap = pcap_create("any", errbuf);
+	if (!pcap) {
+		log_err("Failed to open pcap: %s", errbuf);
+		return NULL;
+	}
+	/* Limit the size of the packet (first N bytes) */
+	r = pcap_set_snaplen(pcap, 1024);
+	if (r) {
+		log_err("Failed to set snaplen: %s", pcap_geterr(pcap));
+		goto error;
+	}
+	/* To receive packets as fast as possible */
+	r = pcap_set_immediate_mode(pcap, 1);
+	if (r) {
+		log_err("Failed to set immediate mode: %s", pcap_geterr(pcap));
+		goto error;
+	}
+	r = pcap_setnonblock(pcap, 1, errbuf);
+	if (r) {
+		log_err("Failed to set nonblock: %s", errbuf);
+		goto error;
+	}
+	r = pcap_activate(pcap);
+	if (r) {
+		log_err("Failed to activate pcap: %s", pcap_geterr(pcap));
+		goto error;
+	}
+	/* Determine the format of the link-layer header */
+	r = pcap_set_datalink(pcap, DLT_LINUX_SLL2);
+	if (r) {
+		log_err("Failed to set datalink: %s", pcap_geterr(pcap));
+		goto error;
+	}
+
+	return pcap;
+error:
+	pcap_close(pcap);
+	return NULL;
+}
+
+static void encode_test_name(char *buf, size_t len)
+{
+	struct prog_test_def *test = env.test;
+	struct subtest_state *subtest_state = env.subtest_state;
+	char *p;
+
+	if (subtest_state)
+		snprintf(buf, len, "%s:%s", test->test_name, subtest_state->name);
+	else
+		snprintf(buf, len, "%s", test->test_name);
+	while ((p = strchr(buf, '/')))
+		*p = '_';
+	while ((p = strchr(buf, ' ')))
+		*p = '_';
+}
+
+#define PCAP_DIR "/tmp/tmon_pcap"
+
+/* Start to monitor the network traffic in the given network namespace.
+ *
+ * netns: the name of the network namespace to monitor. If NULL, the
+ * current network namespace is monitored.
+ *
+ * This function will start a thread to capture packets going through NICs
+ * in the give network namespace.
+ */
+struct tmonitor_ctx *traffic_monitor_start(const char *netns)
+{
+	struct tmonitor_ctx *ctx = NULL;
+	struct nstoken *nstoken = NULL;
+	int pipefd[2] = {-1, -1};
+	static int tmon_seq;
+	char test_name[64];
+	int r;
+
+	if (netns) {
+		nstoken = open_netns(netns);
+		if (!nstoken)
+			return NULL;
+	}
+	ctx = malloc(sizeof(*ctx));
+	if (!ctx) {
+		log_err("Failed to malloc ctx");
+		goto fail_ctx;
+	}
+	memset(ctx, 0, sizeof(*ctx));
+
+	encode_test_name(test_name, sizeof(test_name));
+	snprintf(ctx->pkt_fname, sizeof(ctx->pkt_fname),
+		 PCAP_DIR "/packets-%d-%d-%s-%s.log", getpid(), tmon_seq++,
+		 test_name, netns ? netns : "unknown");
+
+	r = mkdir(PCAP_DIR, 0755);
+	if (r && errno != EEXIST) {
+		log_err("Failed to create " PCAP_DIR);
+		goto fail_pcap;
+	}
+
+	ctx->pcap = traffic_monitor_prepare_pcap();
+	if (!ctx->pcap)
+		goto fail_pcap;
+	ctx->pcap_fd = pcap_get_selectable_fd(ctx->pcap);
+	if (ctx->pcap_fd < 0) {
+		log_err("Failed to get pcap fd");
+		goto fail_dumper;
+	}
+
+	/* Create a packet file */
+	ctx->dumper = pcap_dump_open(ctx->pcap, ctx->pkt_fname);
+	if (!ctx->dumper) {
+		log_err("Failed to open pcap dump: %s", ctx->pkt_fname);
+		goto fail_dumper;
+	}
+
+	/* Create a pipe to wake up the monitor thread */
+	r = pipe(pipefd);
+	if (r) {
+		log_err("Failed to create pipe: %s", strerror(errno));
+		goto fail;
+	}
+	ctx->wake_fd_r = pipefd[0];
+	ctx->wake_fd_w = pipefd[1];
+
+	r = pthread_create(&ctx->thread, NULL, traffic_monitor_thread, ctx);
+	if (r) {
+		log_err("Failed to create thread: %s", strerror(r));
+		goto fail;
+	}
+
+	close_netns(nstoken);
+
+	return ctx;
+
+fail:
+	close(pipefd[0]);
+	close(pipefd[1]);
+
+	pcap_dump_close(ctx->dumper);
+	unlink(ctx->pkt_fname);
+
+fail_dumper:
+	pcap_close(ctx->pcap);
+
+fail_pcap:
+	free(ctx);
+
+fail_ctx:
+	close_netns(nstoken);
+
+	return NULL;
+}
+
+static void traffic_monitor_release(struct tmonitor_ctx *ctx)
+{
+	pcap_close(ctx->pcap);
+	pcap_dump_close(ctx->dumper);
+
+	close(ctx->wake_fd_r);
+	close(ctx->wake_fd_w);
+
+	free(ctx);
+}
+
+/* Stop the network traffic monitor.
+ *
+ * ctx: the context returned by traffic_monitor_start()
+ */
+void traffic_monitor_stop(struct tmonitor_ctx *ctx)
+{
+	if (!ctx)
+		return;
+
+	/* Stop the monitor thread */
+	ctx->done = true;
+	write(ctx->wake_fd_w, "x", 1);
+	pthread_join(ctx->thread, NULL);
+
+	printf("Packet file: %s\n", strrchr(ctx->pkt_fname, '/') + 1);
+
+	traffic_monitor_release(ctx);
+}
+#endif /* TRAFFIC_MONITOR */
+
 void test__end_subtest(void)
 {
 	struct prog_test_def *test = env.test;
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index cb9d6d46826b..5d4e61fa26a1 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -473,4 +473,20 @@ extern void test_loader_fini(struct test_loader *tester);
 	test_loader_fini(&tester);					       \
 })
 
+struct tmonitor_ctx;
+
+#ifdef TRAFFIC_MONITOR
+struct tmonitor_ctx *traffic_monitor_start(const char *netns);
+void traffic_monitor_stop(struct tmonitor_ctx *ctx);
+#else
+static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns)
+{
+	return (struct tmonitor_ctx *)-1;
+}
+
+static inline void traffic_monitor_stop(struct tmonitor_ctx *ctx)
+{
+}
+#endif
+
 #endif /* __TEST_PROGS_H */
-- 
2.34.1

[PATCH bpf-next v4 2/6] selftests/bpf: Add the traffic monitor option to test_progs.

[Kui-Feng Lee]

Add option '-m' to test_progs to accept names and patterns of test cases.
This option will be used later to enable traffic monitor that capture
network packets generated by test cases.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/bpf/test_progs.c | 85 +++++++++++++++++-------
 tools/testing/selftests/bpf/test_progs.h |  2 +
 2 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index ab54d8420603..95643cd3119a 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -170,6 +170,7 @@ struct prog_test_def {
 	void (*run_serial_test)(void);
 	bool should_run;
 	bool need_cgroup_cleanup;
+	bool should_tmon;
 };
 
 /* Override C runtime library's usleep() implementation to ensure nanosleep()
@@ -207,46 +208,59 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
 	return num < sel->num_set_len && sel->num_set[num];
 }
 
-static bool should_run_subtest(struct test_selector *sel,
-			       struct test_selector *subtest_sel,
-			       int subtest_num,
-			       const char *test_name,
-			       const char *subtest_name)
+static bool match_subtest(struct test_filter_set *filter,
+			  const char *test_name,
+			  const char *subtest_name)
 {
 	int i, j;
 
-	for (i = 0; i < sel->blacklist.cnt; i++) {
-		if (glob_match(test_name, sel->blacklist.tests[i].name)) {
-			if (!sel->blacklist.tests[i].subtest_cnt)
-				return false;
-
-			for (j = 0; j < sel->blacklist.tests[i].subtest_cnt; j++) {
-				if (glob_match(subtest_name,
-					       sel->blacklist.tests[i].subtests[j]))
-					return false;
-			}
-		}
-	}
-
-	for (i = 0; i < sel->whitelist.cnt; i++) {
-		if (glob_match(test_name, sel->whitelist.tests[i].name)) {
-			if (!sel->whitelist.tests[i].subtest_cnt)
+	for (i = 0; i < filter->cnt; i++) {
+		if (glob_match(test_name, filter->tests[i].name)) {
+			if (!filter->tests[i].subtest_cnt)
 				return true;
 
-			for (j = 0; j < sel->whitelist.tests[i].subtest_cnt; j++) {
+			for (j = 0; j < filter->tests[i].subtest_cnt; j++) {
 				if (glob_match(subtest_name,
-					       sel->whitelist.tests[i].subtests[j]))
+					       filter->tests[i].subtests[j]))
 					return true;
 			}
 		}
 	}
 
+	return false;
+}
+
+static bool should_run_subtest(struct test_selector *sel,
+			       struct test_selector *subtest_sel,
+			       int subtest_num,
+			       const char *test_name,
+			       const char *subtest_name)
+{
+	if (match_subtest(&sel->blacklist, test_name, subtest_name))
+		return false;
+
+	if (match_subtest(&sel->whitelist, test_name, subtest_name))
+		return true;
+
 	if (!sel->whitelist.cnt && !subtest_sel->num_set)
 		return true;
 
 	return subtest_num < subtest_sel->num_set_len && subtest_sel->num_set[subtest_num];
 }
 
+static bool should_tmon(struct test_selector *sel, int num, const char *name)
+{
+	int i;
+
+	for (i = 0; i < sel->whitelist.cnt; i++) {
+		if (glob_match(name, sel->whitelist.tests[i].name) &&
+		    !sel->whitelist.tests[i].subtest_cnt)
+			return true;
+	}
+
+	return false;
+}
+
 static char *test_result(bool failed, bool skipped)
 {
 	return failed ? "FAIL" : (skipped ? "SKIP" : "OK");
@@ -920,6 +934,10 @@ bool test__start_subtest(const char *subtest_name)
 		return false;
 	}
 
+	subtest_state->should_tmon = match_subtest(&env.tmon_selector.whitelist,
+						   test->test_name,
+						   subtest_name);
+
 	env.subtest_state = subtest_state;
 	stdio_hijack_init(&subtest_state->log_buf, &subtest_state->log_cnt);
 
@@ -1099,7 +1117,8 @@ enum ARG_KEYS {
 	ARG_TEST_NAME_GLOB_DENYLIST = 'd',
 	ARG_NUM_WORKERS = 'j',
 	ARG_DEBUG = -1,
-	ARG_JSON_SUMMARY = 'J'
+	ARG_JSON_SUMMARY = 'J',
+	ARG_TRAFFIC_MONITOR = 'm',
 };
 
 static const struct argp_option opts[] = {
@@ -1126,6 +1145,8 @@ static const struct argp_option opts[] = {
 	{ "debug", ARG_DEBUG, NULL, 0,
 	  "print extra debug information for test_progs." },
 	{ "json-summary", ARG_JSON_SUMMARY, "FILE", 0, "Write report in json format to this file."},
+	{ "traffic-monitor", ARG_TRAFFIC_MONITOR, "NAMES", 0,
+	  "Monitor network traffic of tests with name matching the pattern (supports '*' wildcard)." },
 	{},
 };
 
@@ -1337,6 +1358,18 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 		break;
 	case ARGP_KEY_END:
 		break;
+	case ARG_TRAFFIC_MONITOR: {
+		if (arg[0] == '@')
+			err = parse_test_list_file(arg + 1,
+						   &env->tmon_selector.whitelist,
+						   true);
+		else
+			err = parse_test_list(arg,
+					      &env->tmon_selector.whitelist,
+					      true);
+
+		break;
+	}
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
@@ -2168,6 +2201,9 @@ int main(int argc, char **argv)
 				test->test_num, test->test_name, test->test_name, test->test_name);
 			exit(EXIT_ERR_SETUP_INFRA);
 		}
+		if (test->should_run)
+			test->should_tmon = should_tmon(&env.tmon_selector,
+							test->test_num, test->test_name);
 	}
 
 	/* ignore workers if we are just listing */
@@ -2252,6 +2288,7 @@ int main(int argc, char **argv)
 
 	free_test_selector(&env.test_selector);
 	free_test_selector(&env.subtest_selector);
+	free_test_selector(&env.tmon_selector);
 	free_test_states();
 
 	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 5d4e61fa26a1..ceda86a5a524 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -74,6 +74,7 @@ struct subtest_state {
 	int error_cnt;
 	bool skipped;
 	bool filtered;
+	bool should_tmon;
 
 	FILE *stdout_saved;
 };
@@ -98,6 +99,7 @@ struct test_state {
 struct test_env {
 	struct test_selector test_selector;
 	struct test_selector subtest_selector;
+	struct test_selector tmon_selector;
 	bool verifier_stats;
 	bool debug;
 	enum verbosity verbosity;
-- 
2.34.1

[PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers.

[Kui-Feng Lee]

netns_new()/netns_free() create/delete network namespaces. They support the
option '-m' of test_progs to start/stop traffic monitor for the network
namespace being created for matched tests.

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

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index a3f0a49fb26f..f2cf43382a8e 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -432,6 +432,32 @@ char *ping_command(int family)
 	return "ping";
 }
 
+int make_netns(const char *name)
+{
+	char cmd[128];
+	int r;
+
+	snprintf(cmd, sizeof(cmd), "ip netns add %s", name);
+	r = system(cmd);
+	if (r > 0)
+		/* exit code */
+		return -r;
+	return r;
+}
+
+int remove_netns(const char *name)
+{
+	char cmd[128];
+	int r;
+
+	snprintf(cmd, sizeof(cmd), "ip netns del %s >/dev/null 2>&1", name);
+	r = system(cmd);
+	if (r > 0)
+		/* exit code */
+		return -r;
+	return r;
+}
+
 struct nstoken {
 	int orig_netns_fd;
 };
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index cce56955371f..f8aa8680a640 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -93,6 +93,8 @@ struct nstoken;
 struct nstoken *open_netns(const char *name);
 void close_netns(struct nstoken *token);
 int send_recv_data(int lfd, int fd, uint32_t total_bytes);
+int make_netns(const char *name);
+int remove_netns(const char *name);
 
 static __u16 csum_fold(__u32 csum)
 {
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 95643cd3119a..f86d47efe06e 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1074,6 +1074,86 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
 	return err;
 }
 
+struct netns_obj {
+	char nsname[128];
+	struct tmonitor_ctx *tmon;
+	struct nstoken *nstoken;
+};
+
+/* Create a new network namespace with the given name.
+ *
+ * Create a new network namespace and set the network namespace of the
+ * current process to the new network namespace if the argument "open" is
+ * true. This function should be paired with netns_free() to release the
+ * resource and delete the network namespace.
+ *
+ * It also implements the functionality of the option "-m" by starting
+ * traffic monitor on the background to capture the packets in this network
+ * namespace if the current test or subtest matching the pattern.
+ *
+ * name: the name of the network namespace to create.
+ * open: open the network namespace if true.
+ *
+ * Return: the network namespace object on success, NULL on failure.
+ */
+struct netns_obj *netns_new(const char *name, bool open)
+{
+	struct netns_obj *netns_obj = malloc(sizeof(*netns_obj));
+	int r;
+
+	if (!netns_obj)
+		return NULL;
+	memset(netns_obj, 0, sizeof(*netns_obj));
+
+	strncpy(netns_obj->nsname, name, sizeof(netns_obj->nsname));
+	netns_obj->nsname[sizeof(netns_obj->nsname) - 1] = '\0';
+
+	/* Create the network namespace */
+	r = make_netns(name);
+	if (r)
+		goto fail;
+
+	/* Set the network namespace of the current process */
+	if (open) {
+		netns_obj->nstoken = open_netns(name);
+		if (!netns_obj->nstoken)
+			goto fail;
+	}
+
+	/* Start traffic monitor */
+	if (env.test->should_tmon ||
+	    (env.subtest_state && env.subtest_state->should_tmon)) {
+		netns_obj->tmon = traffic_monitor_start(name);
+		if (!netns_obj->tmon)
+			goto fail;
+	} else {
+		netns_obj->tmon = NULL;
+	}
+
+	return netns_obj;
+fail:
+	close_netns(netns_obj->nstoken);
+	remove_netns(name);
+	free(netns_obj);
+	return NULL;
+}
+
+/* Delete the network namespace.
+ *
+ * This function should be paired with netns_new() to delete the namespace
+ * created by netns_new().
+ */
+void netns_free(struct netns_obj *netns_obj)
+{
+	if (!netns_obj)
+		return;
+	if (netns_obj->tmon)
+		traffic_monitor_stop(netns_obj->tmon);
+	close_netns(netns_obj->nstoken);
+	remove_netns(netns_obj->nsname);
+	free(netns_obj);
+}
+
 /* extern declarations for test funcs */
 #define DEFINE_TEST(name)				\
 	extern void test_##name(void) __weak;		\
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index ceda86a5a524..e025ac6f5a8d 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -430,6 +430,10 @@ int write_sysctl(const char *sysctl, const char *value);
 int get_bpf_max_tramp_links_from(struct btf *btf);
 int get_bpf_max_tramp_links(void);
 
+struct netns_obj;
+struct netns_obj *netns_new(const char *name, bool open);
+void netns_free(struct netns_obj *netns);
+
 #ifdef __x86_64__
 #define SYS_NANOSLEEP_KPROBE_NAME "__x64_sys_nanosleep"
 #elif defined(__s390x__)
-- 
2.34.1

[PATCH bpf-next v4 4/6] selftests/bpf: Monitor traffic for tc_redirect.

[Kui-Feng Lee]

Enable traffic monitoring for the test case tc_redirect.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
index 53b8ffc943dc..54da6b1f23c1 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
@@ -68,6 +68,7 @@
 		__FILE__, __LINE__, strerror(errno), ##__VA_ARGS__)
 
 static const char * const namespaces[] = {NS_SRC, NS_FWD, NS_DST, NULL};
+static struct netns_obj *netns_objs[3];
 
 static int write_file(const char *path, const char *newval)
 {
@@ -88,13 +89,25 @@ static int write_file(const char *path, const char *newval)
 static int netns_setup_namespaces(const char *verb)
 {
 	const char * const *ns = namespaces;
-	char cmd[128];
+	struct netns_obj **ns_obj = netns_objs;
 
 	while (*ns) {
-		snprintf(cmd, sizeof(cmd), "ip netns %s %s", verb, *ns);
-		if (!ASSERT_OK(system(cmd), cmd))
-			return -1;
+		if (strcmp(verb, "add") == 0) {
+			*ns_obj = netns_new(*ns, false);
+			if (!*ns_obj) {
+				log_err("netns_new failed");
+				return -1;
+			}
+		} else {
+			if (!*ns_obj) {
+				log_err("netns_obj is NULL");
+				return -1;
+			}
+			netns_free(*ns_obj);
+			*ns_obj = NULL;
+		}
 		ns++;
+		ns_obj++;
 	}
 	return 0;
 }
@@ -102,12 +115,18 @@ static int netns_setup_namespaces(const char *verb)
 static void netns_setup_namespaces_nofail(const char *verb)
 {
 	const char * const *ns = namespaces;
-	char cmd[128];
+	struct netns_obj **ns_obj = netns_objs;
 
 	while (*ns) {
-		snprintf(cmd, sizeof(cmd), "ip netns %s %s > /dev/null 2>&1", verb, *ns);
-		system(cmd);
+		if (strcmp(verb, "add") == 0) {
+			*ns_obj = netns_new(*ns, false);
+		} else {
+			if (*ns_obj)
+				netns_free(*ns_obj);
+			*ns_obj = NULL;
+		}
 		ns++;
+		ns_obj++;
 	}
 }
 
-- 
2.34.1

[PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen.

[Kui-Feng Lee]

Enable traffic monitor for each subtest of sockmap_listen.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 9ce0e0e0b7da..2030472fb8e8 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1926,14 +1926,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 netns_obj *netns;
 
 	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;
+
+	netns = netns_new("test", true);
+	if (!ASSERT_OK_PTR(netns, "netns_new"))
+		return;
+	system("ip link set lo up");
+
 	inet_unix_skb_redir_to_connected(skel, map, family);
 	unix_inet_skb_redir_to_connected(skel, map, family);
+
+	netns_free(netns);
 }
 
 static void run_tests(struct test_sockmap_listen *skel, struct bpf_map *map,
-- 
2.34.1

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

[Kui-Feng Lee]

Enable traffic monitoring for the subtests of select_reuseport.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
index 64c5f5eb2994..1bb29137cd0a 100644
--- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
+++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
@@ -37,9 +37,7 @@ static int sk_fds[REUSEPORT_ARRAY_SIZE];
 static int reuseport_array = -1, outer_map = -1;
 static enum bpf_map_type inner_map_type;
 static int select_by_skb_data_prog;
-static int saved_tcp_syncookie = -1;
 static struct bpf_object *obj;
-static int saved_tcp_fo = -1;
 static __u32 index_zero;
 static int epfd;
 
@@ -193,14 +191,6 @@ static int write_int_sysctl(const char *sysctl, int v)
 	return 0;
 }
 
-static void restore_sysctls(void)
-{
-	if (saved_tcp_fo != -1)
-		write_int_sysctl(TCP_FO_SYSCTL, saved_tcp_fo);
-	if (saved_tcp_syncookie != -1)
-		write_int_sysctl(TCP_SYNCOOKIE_SYSCTL, saved_tcp_syncookie);
-}
-
 static int enable_fastopen(void)
 {
 	int fo;
@@ -795,6 +785,7 @@ static void test_config(int sotype, sa_family_t family, bool inany)
 	};
 	char s[MAX_TEST_NAME];
 	const struct test *t;
+	struct netns_obj *netns;
 
 	for (t = tests; t < tests + ARRAY_SIZE(tests); t++) {
 		if (t->need_sotype && t->need_sotype != sotype)
@@ -808,9 +799,23 @@ static void test_config(int sotype, sa_family_t family, bool inany)
 		if (!test__start_subtest(s))
 			continue;
 
+		netns = netns_new("test", true);
+		if (!ASSERT_OK_PTR(netns, "netns_new"))
+			continue;
+
+		system("ip link set dev lo up");
+
+		if (CHECK_FAIL(enable_fastopen()))
+			goto out;
+		if (CHECK_FAIL(disable_syncookie()))
+			goto out;
+
 		setup_per_test(sotype, family, inany, t->no_inner_map);
 		t->fn(sotype, family);
 		cleanup_per_test(t->no_inner_map);
+
+out:
+		netns_free(netns);
 	}
 }
 
@@ -850,21 +855,7 @@ void test_map_type(enum bpf_map_type mt)
 
 void serial_test_select_reuseport(void)
 {
-	saved_tcp_fo = read_int_sysctl(TCP_FO_SYSCTL);
-	if (saved_tcp_fo < 0)
-		goto out;
-	saved_tcp_syncookie = read_int_sysctl(TCP_SYNCOOKIE_SYSCTL);
-	if (saved_tcp_syncookie < 0)
-		goto out;
-
-	if (enable_fastopen())
-		goto out;
-	if (disable_syncookie())
-		goto out;
-
 	test_map_type(BPF_MAP_TYPE_REUSEPORT_SOCKARRAY);
 	test_map_type(BPF_MAP_TYPE_SOCKMAP);
 	test_map_type(BPF_MAP_TYPE_SOCKHASH);
-out:
-	restore_sysctls();
 }
-- 
2.34.1

Re: [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers.

[Stanislav Fomichev]

On 07/31, Kui-Feng Lee wrote:
> netns_new()/netns_free() create/delete network namespaces. They support the
> option '-m' of test_progs to start/stop traffic monitor for the network
> namespace being created for matched tests.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/testing/selftests/bpf/network_helpers.c | 26 ++++++
>  tools/testing/selftests/bpf/network_helpers.h |  2 +
>  tools/testing/selftests/bpf/test_progs.c      | 80 +++++++++++++++++++
>  tools/testing/selftests/bpf/test_progs.h      |  4 +
>  4 files changed, 112 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index a3f0a49fb26f..f2cf43382a8e 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -432,6 +432,32 @@ char *ping_command(int family)
>  	return "ping";
>  }
>  
> +int make_netns(const char *name)
> +{

[..]

> +	char cmd[128];
> +	int r;
> +
> +	snprintf(cmd, sizeof(cmd), "ip netns add %s", name);
> +	r = system(cmd);

I doubt that we're gonna see any real problems with that in the tests,
but maybe easier to use apsrint and avoid dealing with fixed 128-byte
string?

> +	if (r > 0)
> +		/* exit code */
> +		return -r;
> +	return r;
> +}
> +
> +int remove_netns(const char *name)
> +{
> +	char cmd[128];
> +	int r;
> +
> +	snprintf(cmd, sizeof(cmd), "ip netns del %s >/dev/null 2>&1", name);
> +	r = system(cmd);
> +	if (r > 0)
> +		/* exit code */
> +		return -r;
> +	return r;
> +}
> +
>  struct nstoken {
>  	int orig_netns_fd;
>  };
> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> index cce56955371f..f8aa8680a640 100644
> --- a/tools/testing/selftests/bpf/network_helpers.h
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -93,6 +93,8 @@ struct nstoken;
>  struct nstoken *open_netns(const char *name);
>  void close_netns(struct nstoken *token);
>  int send_recv_data(int lfd, int fd, uint32_t total_bytes);
> +int make_netns(const char *name);
> +int remove_netns(const char *name);
>  
>  static __u16 csum_fold(__u32 csum)
>  {
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 95643cd3119a..f86d47efe06e 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -1074,6 +1074,86 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
>  	return err;
>  }
>  
> +struct netns_obj {
> +	char nsname[128];
> +	struct tmonitor_ctx *tmon;
> +	struct nstoken *nstoken;
> +};
> +
> +/* Create a new network namespace with the given name.
> + *
> + * Create a new network namespace and set the network namespace of the
> + * current process to the new network namespace if the argument "open" is
> + * true. This function should be paired with netns_free() to release the
> + * resource and delete the network namespace.
> + *
> + * It also implements the functionality of the option "-m" by starting
> + * traffic monitor on the background to capture the packets in this network
> + * namespace if the current test or subtest matching the pattern.
> + *
> + * name: the name of the network namespace to create.
> + * open: open the network namespace if true.
> + *
> + * Return: the network namespace object on success, NULL on failure.
> + */
> +struct netns_obj *netns_new(const char *name, bool open)
> +{
> +	struct netns_obj *netns_obj = malloc(sizeof(*netns_obj));
> +	int r;
> +
> +	if (!netns_obj)
> +		return NULL;
> +	memset(netns_obj, 0, sizeof(*netns_obj));
> +
> +	strncpy(netns_obj->nsname, name, sizeof(netns_obj->nsname));
> +	netns_obj->nsname[sizeof(netns_obj->nsname) - 1] = '\0';

Same here. Seems easier to have "char *nsname" and do
netns_obj->nsname = strdup(name) here. Trimming the name, in theory,
is problematic because do do remove_netns(netns_obj->nsname) later
on (with potentially trimmed name).

But, again, probably not a huge deal in the selftests. So up to you on
whether you want to address it or not.

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

[Stanislav Fomichev]

On 07/31, Kui-Feng Lee wrote:
> Add functions that capture packets and print log in the background. They
> are supposed to be used for debugging flaky network test cases. A monitored
> test case should call traffic_monitor_start() to start a thread to capture
> packets in the background for a given namespace and call
> traffic_monitor_stop() to stop capturing. (Or, option '-m' implemented by
> the later patches.)
> 
>     IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, ifindex 1, SYN
>     IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, ifindex 1, SYN, ACK
>     IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, ifindex 1, ACK
>     IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, ACK
>     IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, ifindex 1, FIN, ACK
>     IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, RST, ACK
>     Packet file: packets-2172-86-select_reuseport:sockhash-test.log
>     #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK test_detach_bpf:OK
> 
> The above is the output of an example. It shows the packets of a connection
> and the name of the file that contains captured packets in the directory
> /tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.
> 
> This feature only works if TRAFFIC_MONITOR variable has been passed to
> build BPF selftests. For example,
> 
>   make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf
> 
> This command will build BPF selftests with this feature enabled.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/testing/selftests/bpf/Makefile     |   5 +
>  tools/testing/selftests/bpf/test_progs.c | 432 +++++++++++++++++++++++
>  tools/testing/selftests/bpf/test_progs.h |  16 +
>  3 files changed, 453 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 774c6270e377..0a3108311be7 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -41,6 +41,11 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic					\
>  LDFLAGS += $(SAN_LDFLAGS)
>  LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
>  
> +ifneq ($(TRAFFIC_MONITOR),)
> +LDLIBS += -lpcap
> +CFLAGS += -DTRAFFIC_MONITOR=1
> +endif

Optionally: can make this more automagical with the following:

LDLIBS += $(shell pkg-config --libs 2>/dev/null)
CFLAGS += $(shell pkg-config --cflags 2>/dev/null)
CFLAGS += $(shell pkg-config --exists libpcap 2>/dev/null && echo "-DTRAFFIC_MONITOR=1")

Re: [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen.

[Stanislav Fomichev]

On 07/31, Kui-Feng Lee wrote:
> Enable traffic monitor for each subtest of sockmap_listen.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/sockmap_listen.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index 9ce0e0e0b7da..2030472fb8e8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1926,14 +1926,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 netns_obj *netns;
>  
>  	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;
> +
> +	netns = netns_new("test", true);
> +	if (!ASSERT_OK_PTR(netns, "netns_new"))
> +		return;

[..]

> +	system("ip link set lo up");

Let's do this in netns_new? We almost always want it in a new ns. The
tests that don't need a loopback can do "lo down".

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

[Martin KaFai Lau]

On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
> Add functions that capture packets and print log in the background. They
> are supposed to be used for debugging flaky network test cases. A monitored
> test case should call traffic_monitor_start() to start a thread to capture
> packets in the background for a given namespace and call
> traffic_monitor_stop() to stop capturing. (Or, option '-m' implemented by
> the later patches.)
> 
>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, ifindex 1, SYN
>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, ifindex 1, SYN, ACK
>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, ifindex 1, ACK
>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, ACK
>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, ifindex 1, FIN, ACK
>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, RST, ACK

nit. Instead of ifindex, it should be ifname now.

>      Packet file: packets-2172-86-select_reuseport:sockhash-test.log
>      #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK test_detach_bpf:OK
> 
> The above is the output of an example. It shows the packets of a connection
> and the name of the file that contains captured packets in the directory
> /tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.
> 
> This feature only works if TRAFFIC_MONITOR variable has been passed to
> build BPF selftests. For example,
> 
>    make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf
> 
> This command will build BPF selftests with this feature enabled.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   tools/testing/selftests/bpf/Makefile     |   5 +
>   tools/testing/selftests/bpf/test_progs.c | 432 +++++++++++++++++++++++

In the cover letter, it mentioned the traffic monitoring implementation is moved 
from the network_helpers.c to test_progs.c.

Can you share more about the reason?

Is it because the traffic monitor now depends on the test_progs's test name, 
should_tmon...etc ? Can the test name and should_tmon be exported for the 
network_helpers to use?

What other compilation issues did it hit if the traffic monitor codes stay in 
the network_helpers.c? Some individual binaries (with main()) like 
test_tcp_check_syncookie_user that links to network_helpers.o but not to 
test_progs.o?

> +#ifdef TRAFFIC_MONITOR
> +struct tmonitor_ctx {
> +	pcap_t *pcap;
> +	pcap_dumper_t *dumper;
> +	pthread_t thread;
> +	int wake_fd_r;
> +	int wake_fd_w;
> +
> +	bool done;
> +	char pkt_fname[PATH_MAX];
> +	int pcap_fd;
> +};
> +
> +/* Is this packet captured with a Ethernet protocol type? */
> +static bool is_ethernet(const u_char *packet)
> +{
> +	u16 arphdr_type;
> +
> +	memcpy(&arphdr_type, packet + 8, 2);
> +	arphdr_type = ntohs(arphdr_type);
> +
> +	/* Except the following cases, the protocol type contains the
> +	 * Ethernet protocol type for the packet.
> +	 *
> +	 * https://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL2.html
> +	 */
> +	switch (arphdr_type) {
> +	case 770: /* ARPHRD_FRAD */
> +	case 778: /* ARPHDR_IPGRE */
> +	case 803: /* ARPHRD_IEEE80211_RADIOTAP */
> +		return false;
> +	}
> +	return true;
> +}
> +
> +/* Show the information of the transport layer in the packet */
> +static void show_transport(const u_char *packet, u16 len, u32 ifindex,
> +			   const char *src_addr, const char *dst_addr,
> +			   u16 proto, bool ipv6)
> +{
> +	struct udphdr *udp;
> +	struct tcphdr *tcp;
> +	u16 src_port, dst_port;
> +	const char *transport_str;
> +	char *ifname, _ifname[IF_NAMESIZE];
> +
> +	ifname = if_indextoname(ifindex, _ifname);
> +	if (!ifname) {
> +		snprintf(_ifname, sizeof(_ifname), "unknown(%d)", ifindex);
> +		ifname = _ifname;
> +	}
> +
> +	if (proto == IPPROTO_UDP) {
> +		udp = (struct udphdr *)packet;
> +		src_port = ntohs(udp->source);
> +		dst_port = ntohs(udp->dest);
> +		transport_str = "UDP";
> +	} else if (proto == IPPROTO_TCP) {
> +		tcp = (struct tcphdr *)packet;
> +		src_port = ntohs(tcp->source);
> +		dst_port = ntohs(tcp->dest);
> +		transport_str = "TCP"
> +;
> +	} else if (proto == IPPROTO_ICMP) {
> +		printf("IPv4 ICMP packet: %s -> %s, len %d, type %d, code %d, ifname %s\n",
> +		       src_addr, dst_addr, len, packet[0], packet[1], ifname);
> +		return;
> +	} else if (proto == IPPROTO_ICMPV6) {
> +		printf("IPv6 ICMPv6 packet: %s -> %s, len %d, type %d, code %d, ifname %s\n",
> +		       src_addr, dst_addr, len, packet[0], packet[1], ifname);
> +		return;
> +	} else {
> +		printf("%s (proto %d): %s -> %s, ifname %s\n",
> +		       ipv6 ? "IPv6" : "IPv4", proto, src_addr, dst_addr, ifname);
> +		return;
> +	}
> +
> +	/* TCP */
> +
> +	if (ipv6)
> +		printf("IPv6 %s packet: [%s]:%d -> [%s]:%d, len %d, ifname %s",
> +		       transport_str, src_addr, src_port,
> +		       dst_addr, dst_port, len, ifname);
> +	else
> +		printf("IPv4 %s packet: %s:%d -> %s:%d, len %d, ifname %s",
> +		       transport_str, src_addr, src_port,
> +		       dst_addr, dst_port, len, ifname);
> +
> +	if (proto == IPPROTO_TCP) {
> +		if (tcp->fin)
> +			printf(", FIN");
> +		if (tcp->syn)
> +			printf(", SYN");
> +		if (tcp->rst)
> +			printf(", RST");
> +		if (tcp->ack)
> +			printf(", ACK");
> +	}
> +
> +	printf("\n");

The TCP packet output is done by multiple printf. There is a good chance that 
one TCP packet is interleaved with the other printf(s), considering the traffic 
monitoring is done in its own thread. I am seeing this in the tc_redirect test.

Does it help to do multiple snprintf (for the tcp flags?) first and then calls 
one printf?

> +}
> +
> +static void show_ipv6_packet(const u_char *packet, u32 ifindex)
> +{
> +	struct ipv6hdr *pkt = (struct ipv6hdr *)packet;
> +	struct in6_addr src;
> +	struct in6_addr dst;
> +	char src_str[INET6_ADDRSTRLEN], dst_str[INET6_ADDRSTRLEN];
> +	u_char proto;
> +
> +	memcpy(&src, &pkt->saddr, sizeof(src));
> +	memcpy(&dst, &pkt->daddr, sizeof(dst));
> +	inet_ntop(AF_INET6, &src, src_str, sizeof(src_str));
> +	inet_ntop(AF_INET6, &dst, dst_str, sizeof(dst_str));
> +	proto = pkt->nexthdr;
> +	show_transport(packet + sizeof(struct ipv6hdr),
> +		       ntohs(pkt->payload_len),
> +		       ifindex, src_str, dst_str, proto, true);
> +}
> +
> +static void show_ipv4_packet(const u_char *packet, u32 ifindex)
> +{
> +	struct iphdr *pkt = (struct iphdr *)packet;
> +	struct in_addr src;
> +	struct in_addr dst;
> +	u_char proto;
> +	char src_str[INET_ADDRSTRLEN], dst_str[INET_ADDRSTRLEN];
> +
> +	memcpy(&src, &pkt->saddr, sizeof(src));
> +	memcpy(&dst, &pkt->daddr, sizeof(dst));
> +	inet_ntop(AF_INET, &src, src_str, sizeof(src_str));
> +	inet_ntop(AF_INET, &dst, dst_str, sizeof(dst_str));
> +	proto = pkt->protocol;
> +	show_transport(packet + sizeof(struct iphdr),
> +		       ntohs(pkt->tot_len),
> +		       ifindex, src_str, dst_str, proto, false);
> +}
> +
> +static void *traffic_monitor_thread(void *arg)
> +{
> +	char *ifname, _ifname[IF_NAMESIZE];
> +	const u_char *packet, *payload;
> +	struct tmonitor_ctx *ctx = arg;
> +	struct pcap_pkthdr header;
> +	pcap_t *pcap = ctx->pcap;
> +	pcap_dumper_t *dumper = ctx->dumper;
> +	int fd = ctx->pcap_fd;
> +	int wake_fd = ctx->wake_fd_r;
> +	u16 proto;
> +	u32 ifindex;
> +	fd_set fds;
> +	int nfds, r;
> +
> +	nfds = (fd > wake_fd ? fd : wake_fd) + 1;
> +	FD_ZERO(&fds);
> +
> +	while (!ctx->done) {
> +		FD_SET(fd, &fds);
> +		FD_SET(wake_fd, &fds);
> +		r = select(nfds, &fds, NULL, NULL, NULL);
> +		if (!r)
> +			continue;
> +		if (r < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			log_err("Fail to select on pcap fd and wake fd: %s", strerror(errno));
> +			break;
> +		}
> +
> +		packet = pcap_next(pcap, &header);
> +		if (!packet)
> +			continue;
> +
> +		/* According to the man page of pcap_dump(), first argument
> +		 * is the pcap_dumper_t pointer even it's argument type is
> +		 * u_char *.
> +		 */
> +		pcap_dump((u_char *)dumper, &header, packet);

The captured file has the "In", "Out", and "M" (Multicast) when it is read by 
tcpdump. Is it easy to printf that in show_ipv[46]_packet() also? Just want to 
ensure we didn't miss it if it is easy.

> +
> +		/* Not sure what other types of packets look like. Here, we
> +		 * parse only Ethernet and compatible packets.
> +		 */
> +		if (!is_ethernet(packet)) {
> +			printf("Packet captured\n");
> +			continue;
> +		}
> +
> +		/* Skip SLL2 header
> +		 * https://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL2.html
> +		 *
> +		 * Although the document doesn't mention that, the payload
> +		 * doesn't include the Ethernet header. The payload starts
> +		 * from the first byte of the network layer header.
> +		 */
> +		payload = packet + 20;
> +
> +		memcpy(&proto, packet, 2);
> +		proto = ntohs(proto);
> +		memcpy(&ifindex, packet + 4, 4);
> +		ifindex = ntohl(ifindex);
> +
> +		if (proto == ETH_P_IPV6) {
> +			show_ipv6_packet(payload, ifindex);
> +		} else if (proto == ETH_P_IP) {
> +			show_ipv4_packet(payload, ifindex);
> +		} else {
> +			ifname = if_indextoname(ifindex, _ifname);
> +			if (!ifname) {
> +				snprintf(_ifname, sizeof(_ifname), "unknown(%d)", ifindex);
> +				ifname = _ifname;
> +			}
> +
> +			printf("Unknown network protocol type %x, ifname %s\n", proto, ifname);
> +		}
> +	}
> +
> +	return NULL;
> +}

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

[Martin KaFai Lau]

On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index cb9d6d46826b..5d4e61fa26a1 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct test_loader *tester);
>   	test_loader_fini(&tester);					       \
>   })
>   
> +struct tmonitor_ctx;
> +
> +#ifdef TRAFFIC_MONITOR
> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
> +#else
> +static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns)
> +{
> +	return (struct tmonitor_ctx *)-1;

hmm... from peeking patch 3, only NULL is checked.

While at it, if there is no libpcap during make, is the "-m" option available or 
the test_progs will error out if "-m" is used?

> +}
> +
> +static inline void traffic_monitor_stop(struct tmonitor_ctx *ctx)
> +{
> +}
> +#endif
> +
>   #endif /* __TEST_PROGS_H */

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

[Kui-Feng Lee]

On 7/31/24 14:07, Stanislav Fomichev wrote:
> On 07/31, Kui-Feng Lee wrote:
>> Add functions that capture packets and print log in the background. They
>> are supposed to be used for debugging flaky network test cases. A monitored
>> test case should call traffic_monitor_start() to start a thread to capture
>> packets in the background for a given namespace and call
>> traffic_monitor_stop() to stop capturing. (Or, option '-m' implemented by
>> the later patches.)
>>
>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, ifindex 1, SYN
>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, ifindex 1, SYN, ACK
>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, ifindex 1, ACK
>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, ACK
>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, ifindex 1, FIN, ACK
>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, RST, ACK
>>      Packet file: packets-2172-86-select_reuseport:sockhash-test.log
>>      #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK test_detach_bpf:OK
>>
>> The above is the output of an example. It shows the packets of a connection
>> and the name of the file that contains captured packets in the directory
>> /tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.
>>
>> This feature only works if TRAFFIC_MONITOR variable has been passed to
>> build BPF selftests. For example,
>>
>>    make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf
>>
>> This command will build BPF selftests with this feature enabled.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/Makefile     |   5 +
>>   tools/testing/selftests/bpf/test_progs.c | 432 +++++++++++++++++++++++
>>   tools/testing/selftests/bpf/test_progs.h |  16 +
>>   3 files changed, 453 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index 774c6270e377..0a3108311be7 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -41,6 +41,11 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic					\
>>   LDFLAGS += $(SAN_LDFLAGS)
>>   LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
>>   
>> +ifneq ($(TRAFFIC_MONITOR),)
>> +LDLIBS += -lpcap
>> +CFLAGS += -DTRAFFIC_MONITOR=1
>> +endif
> 
> Optionally: can make this more automagical with the following:
> 
> LDLIBS += $(shell pkg-config --libs 2>/dev/null)
> CFLAGS += $(shell pkg-config --cflags 2>/dev/null)
> CFLAGS += $(shell pkg-config --exists libpcap 2>/dev/null && echo "-DTRAFFIC_MONITOR=1")

Sure! I will try it!

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

[Kui-Feng Lee]

On 8/1/24 20:29, Martin KaFai Lau wrote:
> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>> Add functions that capture packets and print log in the background. They
>> are supposed to be used for debugging flaky network test cases. A 
>> monitored
>> test case should call traffic_monitor_start() to start a thread to 
>> capture
>> packets in the background for a given namespace and call
>> traffic_monitor_stop() to stop capturing. (Or, option '-m' implemented by
>> the later patches.)
>>
>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, 
>> ifindex 1, SYN
>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, 
>> ifindex 1, SYN, ACK
>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, 
>> ifindex 1, ACK
>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, 
>> ifindex 1, ACK
>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, 
>> ifindex 1, FIN, ACK
>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, 
>> ifindex 1, RST, ACK
> 
> nit. Instead of ifindex, it should be ifname now.

Sure! I will update it.

> 
>>      Packet file: packets-2172-86-select_reuseport:sockhash-test.log
>>      #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK 
>> test_detach_bpf:OK
>>
>> The above is the output of an example. It shows the packets of a 
>> connection
>> and the name of the file that contains captured packets in the directory
>> /tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.
>>
>> This feature only works if TRAFFIC_MONITOR variable has been passed to
>> build BPF selftests. For example,
>>
>>    make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf
>>
>> This command will build BPF selftests with this feature enabled.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/Makefile     |   5 +
>>   tools/testing/selftests/bpf/test_progs.c | 432 +++++++++++++++++++++++
> 
> In the cover letter, it mentioned the traffic monitoring implementation 
> is moved from the network_helpers.c to test_progs.c.
> 
> Can you share more about the reason?

network_helpers.c has been used by several test programs.
However, they don't have env that we found in test_progs.c.
That means we could not access env directly. Instead, the caller
have to pass the test name and subtest name to the function.
Leter, we also need to check if a test name matches the patterns. It is
inconvient for users. So, I move these functions to test_progs.c to make
user's life eaiser.


> 
> Is it because the traffic monitor now depends on the test_progs's test 
> name, should_tmon...etc ? Can the test name and should_tmon be exported 
> for the network_helpers to use?

Yes! And in later patches, we also introduce a list of patterns.
> 
> What other compilation issues did it hit if the traffic monitor codes 
> stay in the network_helpers.c? Some individual binaries (with main()) 
> like test_tcp_check_syncookie_user that links to network_helpers.o but 
> not to test_progs.o?

Yes, they are problems as well. These binary also need to link to
libpcap even they don't use it although this is not an important issue.


> 
>> +#ifdef TRAFFIC_MONITOR
>> +struct tmonitor_ctx {
>> +    pcap_t *pcap;
>> +    pcap_dumper_t *dumper;
>> +    pthread_t thread;
>> +    int wake_fd_r;
>> +    int wake_fd_w;
>> +
>> +    bool done;
>> +    char pkt_fname[PATH_MAX];
>> +    int pcap_fd;
>> +};
>> +
>> +/* Is this packet captured with a Ethernet protocol type? */
>> +static bool is_ethernet(const u_char *packet)
>> +{
>> +    u16 arphdr_type;
>> +
>> +    memcpy(&arphdr_type, packet + 8, 2);
>> +    arphdr_type = ntohs(arphdr_type);
>> +
>> +    /* Except the following cases, the protocol type contains the
>> +     * Ethernet protocol type for the packet.
>> +     *
>> +     * https://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL2.html
>> +     */
>> +    switch (arphdr_type) {
>> +    case 770: /* ARPHRD_FRAD */
>> +    case 778: /* ARPHDR_IPGRE */
>> +    case 803: /* ARPHRD_IEEE80211_RADIOTAP */
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +/* Show the information of the transport layer in the packet */
>> +static void show_transport(const u_char *packet, u16 len, u32 ifindex,
>> +               const char *src_addr, const char *dst_addr,
>> +               u16 proto, bool ipv6)
>> +{
>> +    struct udphdr *udp;
>> +    struct tcphdr *tcp;
>> +    u16 src_port, dst_port;
>> +    const char *transport_str;
>> +    char *ifname, _ifname[IF_NAMESIZE];
>> +
>> +    ifname = if_indextoname(ifindex, _ifname);
>> +    if (!ifname) {
>> +        snprintf(_ifname, sizeof(_ifname), "unknown(%d)", ifindex);
>> +        ifname = _ifname;
>> +    }
>> +
>> +    if (proto == IPPROTO_UDP) {
>> +        udp = (struct udphdr *)packet;
>> +        src_port = ntohs(udp->source);
>> +        dst_port = ntohs(udp->dest);
>> +        transport_str = "UDP";
>> +    } else if (proto == IPPROTO_TCP) {
>> +        tcp = (struct tcphdr *)packet;
>> +        src_port = ntohs(tcp->source);
>> +        dst_port = ntohs(tcp->dest);
>> +        transport_str = "TCP"
>> +;
>> +    } else if (proto == IPPROTO_ICMP) {
>> +        printf("IPv4 ICMP packet: %s -> %s, len %d, type %d, code %d, 
>> ifname %s\n",
>> +               src_addr, dst_addr, len, packet[0], packet[1], ifname);
>> +        return;
>> +    } else if (proto == IPPROTO_ICMPV6) {
>> +        printf("IPv6 ICMPv6 packet: %s -> %s, len %d, type %d, code 
>> %d, ifname %s\n",
>> +               src_addr, dst_addr, len, packet[0], packet[1], ifname);
>> +        return;
>> +    } else {
>> +        printf("%s (proto %d): %s -> %s, ifname %s\n",
>> +               ipv6 ? "IPv6" : "IPv4", proto, src_addr, dst_addr, 
>> ifname);
>> +        return;
>> +    }
>> +
>> +    /* TCP */
>> +
>> +    if (ipv6)
>> +        printf("IPv6 %s packet: [%s]:%d -> [%s]:%d, len %d, ifname %s",
>> +               transport_str, src_addr, src_port,
>> +               dst_addr, dst_port, len, ifname);
>> +    else
>> +        printf("IPv4 %s packet: %s:%d -> %s:%d, len %d, ifname %s",
>> +               transport_str, src_addr, src_port,
>> +               dst_addr, dst_port, len, ifname);
>> +
>> +    if (proto == IPPROTO_TCP) {
>> +        if (tcp->fin)
>> +            printf(", FIN");
>> +        if (tcp->syn)
>> +            printf(", SYN");
>> +        if (tcp->rst)
>> +            printf(", RST");
>> +        if (tcp->ack)
>> +            printf(", ACK");
>> +    }
>> +
>> +    printf("\n");
> 
> The TCP packet output is done by multiple printf. There is a good chance 
> that one TCP packet is interleaved with the other printf(s), considering 
> the traffic monitoring is done in its own thread. I am seeing this in 
> the tc_redirect test.
> 
> Does it help to do multiple snprintf (for the tcp flags?) first and then 
> calls one printf?

Sure, it would help. Or, perform flockfile() and funlockfile().

> 
>> +}
>> +
>> +static void show_ipv6_packet(const u_char *packet, u32 ifindex)
>> +{
>> +    struct ipv6hdr *pkt = (struct ipv6hdr *)packet;
>> +    struct in6_addr src;
>> +    struct in6_addr dst;
>> +    char src_str[INET6_ADDRSTRLEN], dst_str[INET6_ADDRSTRLEN];
>> +    u_char proto;
>> +
>> +    memcpy(&src, &pkt->saddr, sizeof(src));
>> +    memcpy(&dst, &pkt->daddr, sizeof(dst));
>> +    inet_ntop(AF_INET6, &src, src_str, sizeof(src_str));
>> +    inet_ntop(AF_INET6, &dst, dst_str, sizeof(dst_str));
>> +    proto = pkt->nexthdr;
>> +    show_transport(packet + sizeof(struct ipv6hdr),
>> +               ntohs(pkt->payload_len),
>> +               ifindex, src_str, dst_str, proto, true);
>> +}
>> +
>> +static void show_ipv4_packet(const u_char *packet, u32 ifindex)
>> +{
>> +    struct iphdr *pkt = (struct iphdr *)packet;
>> +    struct in_addr src;
>> +    struct in_addr dst;
>> +    u_char proto;
>> +    char src_str[INET_ADDRSTRLEN], dst_str[INET_ADDRSTRLEN];
>> +
>> +    memcpy(&src, &pkt->saddr, sizeof(src));
>> +    memcpy(&dst, &pkt->daddr, sizeof(dst));
>> +    inet_ntop(AF_INET, &src, src_str, sizeof(src_str));
>> +    inet_ntop(AF_INET, &dst, dst_str, sizeof(dst_str));
>> +    proto = pkt->protocol;
>> +    show_transport(packet + sizeof(struct iphdr),
>> +               ntohs(pkt->tot_len),
>> +               ifindex, src_str, dst_str, proto, false);
>> +}
>> +
>> +static void *traffic_monitor_thread(void *arg)
>> +{
>> +    char *ifname, _ifname[IF_NAMESIZE];
>> +    const u_char *packet, *payload;
>> +    struct tmonitor_ctx *ctx = arg;
>> +    struct pcap_pkthdr header;
>> +    pcap_t *pcap = ctx->pcap;
>> +    pcap_dumper_t *dumper = ctx->dumper;
>> +    int fd = ctx->pcap_fd;
>> +    int wake_fd = ctx->wake_fd_r;
>> +    u16 proto;
>> +    u32 ifindex;
>> +    fd_set fds;
>> +    int nfds, r;
>> +
>> +    nfds = (fd > wake_fd ? fd : wake_fd) + 1;
>> +    FD_ZERO(&fds);
>> +
>> +    while (!ctx->done) {
>> +        FD_SET(fd, &fds);
>> +        FD_SET(wake_fd, &fds);
>> +        r = select(nfds, &fds, NULL, NULL, NULL);
>> +        if (!r)
>> +            continue;
>> +        if (r < 0) {
>> +            if (errno == EINTR)
>> +                continue;
>> +            log_err("Fail to select on pcap fd and wake fd: %s", 
>> strerror(errno));
>> +            break;
>> +        }
>> +
>> +        packet = pcap_next(pcap, &header);
>> +        if (!packet)
>> +            continue;
>> +
>> +        /* According to the man page of pcap_dump(), first argument
>> +         * is the pcap_dumper_t pointer even it's argument type is
>> +         * u_char *.
>> +         */
>> +        pcap_dump((u_char *)dumper, &header, packet);
> 
> The captured file has the "In", "Out", and "M" (Multicast) when it is 
> read by tcpdump. Is it easy to printf that in show_ipv[46]_packet() 
> also? Just want to ensure we didn't miss it if it is easy.

No problem! IIRC, it is part of the SSL2 header.

> 
>> +
>> +        /* Not sure what other types of packets look like. Here, we
>> +         * parse only Ethernet and compatible packets.
>> +         */
>> +        if (!is_ethernet(packet)) {
>> +            printf("Packet captured\n");
>> +            continue;
>> +        }
>> +
>> +        /* Skip SLL2 header
>> +         * https://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL2.html
>> +         *
>> +         * Although the document doesn't mention that, the payload
>> +         * doesn't include the Ethernet header. The payload starts
>> +         * from the first byte of the network layer header.
>> +         */
>> +        payload = packet + 20;
>> +
>> +        memcpy(&proto, packet, 2);
>> +        proto = ntohs(proto);
>> +        memcpy(&ifindex, packet + 4, 4);
>> +        ifindex = ntohl(ifindex);
>> +
>> +        if (proto == ETH_P_IPV6) {
>> +            show_ipv6_packet(payload, ifindex);
>> +        } else if (proto == ETH_P_IP) {
>> +            show_ipv4_packet(payload, ifindex);
>> +        } else {
>> +            ifname = if_indextoname(ifindex, _ifname);
>> +            if (!ifname) {
>> +                snprintf(_ifname, sizeof(_ifname), "unknown(%d)", 
>> ifindex);
>> +                ifname = _ifname;
>> +            }
>> +
>> +            printf("Unknown network protocol type %x, ifname %s\n", 
>> proto, ifname);
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
> 
> 

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

[Kui-Feng Lee]

On 8/1/24 20:43, Martin KaFai Lau wrote:
> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>> diff --git a/tools/testing/selftests/bpf/test_progs.h 
>> b/tools/testing/selftests/bpf/test_progs.h
>> index cb9d6d46826b..5d4e61fa26a1 100644
>> --- a/tools/testing/selftests/bpf/test_progs.h
>> +++ b/tools/testing/selftests/bpf/test_progs.h
>> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct test_loader 
>> *tester);
>>       test_loader_fini(&tester);                           \
>>   })
>> +struct tmonitor_ctx;
>> +
>> +#ifdef TRAFFIC_MONITOR
>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
>> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>> +#else
>> +static inline struct tmonitor_ctx *traffic_monitor_start(const char 
>> *netns)
>> +{
>> +    return (struct tmonitor_ctx *)-1;
> 
> hmm... from peeking patch 3, only NULL is checked.
> 
> While at it, if there is no libpcap during make, is the "-m" option 
> available or the test_progs will error out if "-m" is used?

"-m" is still available so CI can always pass "-m" without consider
the configuration of the binary. But, it would be good idea to
print a warning message for this situation.

> 
>> +}
>> +
>> +static inline void traffic_monitor_stop(struct tmonitor_ctx *ctx)
>> +{
>> +}
>> +#endif
>> +
>>   #endif /* __TEST_PROGS_H */
> 

Re: [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers.

[Kui-Feng Lee]

On 7/31/24 14:02, Stanislav Fomichev wrote:
> On 07/31, Kui-Feng Lee wrote:
>> netns_new()/netns_free() create/delete network namespaces. They support the
>> option '-m' of test_progs to start/stop traffic monitor for the network
>> namespace being created for matched tests.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/network_helpers.c | 26 ++++++
>>   tools/testing/selftests/bpf/network_helpers.h |  2 +
>>   tools/testing/selftests/bpf/test_progs.c      | 80 +++++++++++++++++++
>>   tools/testing/selftests/bpf/test_progs.h      |  4 +
>>   4 files changed, 112 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
>> index a3f0a49fb26f..f2cf43382a8e 100644
>> --- a/tools/testing/selftests/bpf/network_helpers.c
>> +++ b/tools/testing/selftests/bpf/network_helpers.c
>> @@ -432,6 +432,32 @@ char *ping_command(int family)
>>   	return "ping";
>>   }
>>   
>> +int make_netns(const char *name)
>> +{
> 
> [..]
> 
>> +	char cmd[128];
>> +	int r;
>> +
>> +	snprintf(cmd, sizeof(cmd), "ip netns add %s", name);
>> +	r = system(cmd);
> 
> I doubt that we're gonna see any real problems with that in the tests,
> but maybe easier to use apsrint and avoid dealing with fixed 128-byte
> string?

asprintf? Sure!

> 
>> +	if (r > 0)
>> +		/* exit code */
>> +		return -r;
>> +	return r;
>> +}
>> +
>> +int remove_netns(const char *name)
>> +{
>> +	char cmd[128];
>> +	int r;
>> +
>> +	snprintf(cmd, sizeof(cmd), "ip netns del %s >/dev/null 2>&1", name);
>> +	r = system(cmd);
>> +	if (r > 0)
>> +		/* exit code */
>> +		return -r;
>> +	return r;
>> +}
>> +
>>   struct nstoken {
>>   	int orig_netns_fd;
>>   };
>> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
>> index cce56955371f..f8aa8680a640 100644
>> --- a/tools/testing/selftests/bpf/network_helpers.h
>> +++ b/tools/testing/selftests/bpf/network_helpers.h
>> @@ -93,6 +93,8 @@ struct nstoken;
>>   struct nstoken *open_netns(const char *name);
>>   void close_netns(struct nstoken *token);
>>   int send_recv_data(int lfd, int fd, uint32_t total_bytes);
>> +int make_netns(const char *name);
>> +int remove_netns(const char *name);
>>   
>>   static __u16 csum_fold(__u32 csum)
>>   {
>> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
>> index 95643cd3119a..f86d47efe06e 100644
>> --- a/tools/testing/selftests/bpf/test_progs.c
>> +++ b/tools/testing/selftests/bpf/test_progs.c
>> @@ -1074,6 +1074,86 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
>>   	return err;
>>   }
>>   
>> +struct netns_obj {
>> +	char nsname[128];
>> +	struct tmonitor_ctx *tmon;
>> +	struct nstoken *nstoken;
>> +};
>> +
>> +/* Create a new network namespace with the given name.
>> + *
>> + * Create a new network namespace and set the network namespace of the
>> + * current process to the new network namespace if the argument "open" is
>> + * true. This function should be paired with netns_free() to release the
>> + * resource and delete the network namespace.
>> + *
>> + * It also implements the functionality of the option "-m" by starting
>> + * traffic monitor on the background to capture the packets in this network
>> + * namespace if the current test or subtest matching the pattern.
>> + *
>> + * name: the name of the network namespace to create.
>> + * open: open the network namespace if true.
>> + *
>> + * Return: the network namespace object on success, NULL on failure.
>> + */
>> +struct netns_obj *netns_new(const char *name, bool open)
>> +{
>> +	struct netns_obj *netns_obj = malloc(sizeof(*netns_obj));
>> +	int r;
>> +
>> +	if (!netns_obj)
>> +		return NULL;
>> +	memset(netns_obj, 0, sizeof(*netns_obj));
>> +
>> +	strncpy(netns_obj->nsname, name, sizeof(netns_obj->nsname));
>> +	netns_obj->nsname[sizeof(netns_obj->nsname) - 1] = '\0';
> 
> Same here. Seems easier to have "char *nsname" and do
> netns_obj->nsname = strdup(name) here. Trimming the name, in theory,
> is problematic because do do remove_netns(netns_obj->nsname) later
> on (with potentially trimmed name).

You are right!

> 
> But, again, probably not a huge deal in the selftests. So up to you on
> whether you want to address it or not.

Re: [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen.

[Kui-Feng Lee]

On 7/31/24 14:09, Stanislav Fomichev wrote:
> On 07/31, Kui-Feng Lee wrote:
>> Enable traffic monitor for each subtest of sockmap_listen.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/prog_tests/sockmap_listen.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> index 9ce0e0e0b7da..2030472fb8e8 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>> @@ -1926,14 +1926,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 netns_obj *netns;
>>   
>>   	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;
>> +
>> +	netns = netns_new("test", true);
>> +	if (!ASSERT_OK_PTR(netns, "netns_new"))
>> +		return;
> 
> [..]
> 
>> +	system("ip link set lo up");
> 
> Let's do this in netns_new? We almost always want it in a new ns. The
> tests that don't need a loopback can do "lo down".
Agree!

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

[Martin KaFai Lau]

On 8/1/24 9:31 PM, Kui-Feng Lee wrote:
> 
> 
> On 8/1/24 20:29, Martin KaFai Lau wrote:
>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>> Add functions that capture packets and print log in the background. They
>>> are supposed to be used for debugging flaky network test cases. A monitored
>>> test case should call traffic_monitor_start() to start a thread to capture
>>> packets in the background for a given namespace and call
>>> traffic_monitor_stop() to stop capturing. (Or, option '-m' implemented by
>>> the later patches.)
>>>
>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, ifindex 1, SYN
>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, ifindex 1, 
>>> SYN, ACK
>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, ifindex 1, ACK
>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, ACK
>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, ifindex 1, 
>>> FIN, ACK
>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, ifindex 1, 
>>> RST, ACK
>>
>> nit. Instead of ifindex, it should be ifname now.
> 
> Sure! I will update it.
> 
>>
>>>      Packet file: packets-2172-86-select_reuseport:sockhash-test.log
>>>      #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK test_detach_bpf:OK
>>>
>>> The above is the output of an example. It shows the packets of a connection
>>> and the name of the file that contains captured packets in the directory
>>> /tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.
>>>
>>> This feature only works if TRAFFIC_MONITOR variable has been passed to
>>> build BPF selftests. For example,
>>>
>>>    make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf
>>>
>>> This command will build BPF selftests with this feature enabled.
>>>
>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>> ---
>>>   tools/testing/selftests/bpf/Makefile     |   5 +
>>>   tools/testing/selftests/bpf/test_progs.c | 432 +++++++++++++++++++++++
>>
>> In the cover letter, it mentioned the traffic monitoring implementation is 
>> moved from the network_helpers.c to test_progs.c.
>>
>> Can you share more about the reason?
> 
> network_helpers.c has been used by several test programs.
> However, they don't have env that we found in test_progs.c.
> That means we could not access env directly. Instead, the caller
> have to pass the test name and subtest name to the function.
> Leter, we also need to check if a test name matches the patterns. It is
> inconvient for users. So, I move these functions to test_progs.c to make
> user's life eaiser.
> 
> 
>>
>> Is it because the traffic monitor now depends on the test_progs's test name, 
>> should_tmon...etc ? Can the test name and should_tmon be exported for the 
>> network_helpers to use?
> 
> Yes! And in later patches, we also introduce a list of patterns.

The list of patterns matching is summarized in "should_tmon" which can be 
exported through a function?

or I have missed another criteria when deciding tmon should be enabled for a test?

>>
>> What other compilation issues did it hit if the traffic monitor codes stay in 
>> the network_helpers.c? Some individual binaries (with main()) like 
>> test_tcp_check_syncookie_user that links to network_helpers.o but not to 
>> test_progs.o?
> 
> Yes, they are problems as well. These binary also need to link to
> libpcap even they don't use it although this is not an important issue.

I don't think linking the non test_progs binaries to libpcap or not is important.

I am positive there are ways out of it without adding the networking codes to 
the test_progs.c. It sounds like an unnecessary nit now but I believe it is 
useful going forward when making changes and extension to the traffic 
monitoring. May be brainstorm a little to see if there is an way out.

One way could be putting them in a new traffic_monitor.c such that the non 
test_progs binaries won't link to it. and exports the test name and shmod_tmon 
in test_progs.h (e.g. through function).

Another way (better and my preference if it works out) is to ask the 
traffic_monitor_start() to take the the pcap file name args and makeup a 
reasonable default if no filename is given.  Not that I am promoting non 
test_progs tests, traffic_monitor_start() can then be reused by others for legit 
reason. The test_progs's tests usually should not use traffic_monitor_start() 
directly and they should stay with the netns_{new, free}. I think only netns_new 
needs the env to figure out the should_tmon and the pcap filename. May be 
netns_new() can stay in test_progs.c, or rename it to test__netns_new().

wdyt?

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

[Kui-Feng Lee]

On 8/2/24 11:58, Martin KaFai Lau wrote:
> On 8/1/24 9:31 PM, Kui-Feng Lee wrote:
>>
>>
>> On 8/1/24 20:29, Martin KaFai Lau wrote:
>>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>>> Add functions that capture packets and print log in the background. 
>>>> They
>>>> are supposed to be used for debugging flaky network test cases. A 
>>>> monitored
>>>> test case should call traffic_monitor_start() to start a thread to 
>>>> capture
>>>> packets in the background for a given namespace and call
>>>> traffic_monitor_stop() to stop capturing. (Or, option '-m' 
>>>> implemented by
>>>> the later patches.)
>>>>
>>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, 
>>>> ifindex 1, SYN
>>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, 
>>>> ifindex 1, SYN, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, 
>>>> ifindex 1, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, 
>>>> ifindex 1, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, 
>>>> ifindex 1, FIN, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, 
>>>> ifindex 1, RST, ACK
>>>
>>> nit. Instead of ifindex, it should be ifname now.
>>
>> Sure! I will update it.
>>
>>>
>>>>      Packet file: packets-2172-86-select_reuseport:sockhash-test.log
>>>>      #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK 
>>>> test_detach_bpf:OK
>>>>
>>>> The above is the output of an example. It shows the packets of a 
>>>> connection
>>>> and the name of the file that contains captured packets in the 
>>>> directory
>>>> /tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.
>>>>
>>>> This feature only works if TRAFFIC_MONITOR variable has been passed to
>>>> build BPF selftests. For example,
>>>>
>>>>    make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf
>>>>
>>>> This command will build BPF selftests with this feature enabled.
>>>>
>>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>>> ---
>>>>   tools/testing/selftests/bpf/Makefile     |   5 +
>>>>   tools/testing/selftests/bpf/test_progs.c | 432 
>>>> +++++++++++++++++++++++
>>>
>>> In the cover letter, it mentioned the traffic monitoring 
>>> implementation is moved from the network_helpers.c to test_progs.c.
>>>
>>> Can you share more about the reason?
>>
>> network_helpers.c has been used by several test programs.
>> However, they don't have env that we found in test_progs.c.
>> That means we could not access env directly. Instead, the caller
>> have to pass the test name and subtest name to the function.
>> Leter, we also need to check if a test name matches the patterns. It is
>> inconvient for users. So, I move these functions to test_progs.c to make
>> user's life eaiser.
>>
>>
>>>
>>> Is it because the traffic monitor now depends on the test_progs's 
>>> test name, should_tmon...etc ? Can the test name and should_tmon be 
>>> exported for the network_helpers to use?
>>
>> Yes! And in later patches, we also introduce a list of patterns.
> 
> The list of patterns matching is summarized in "should_tmon" which can 
> be exported through a function?

Yes! Even with a functio, it still depends on test_progs.c.

> 
> or I have missed another criteria when deciding tmon should be enabled 
> for a test?
> 
>>>
>>> What other compilation issues did it hit if the traffic monitor codes 
>>> stay in the network_helpers.c? Some individual binaries (with main()) 
>>> like test_tcp_check_syncookie_user that links to network_helpers.o 
>>> but not to test_progs.o?
>>
>> Yes, they are problems as well. These binary also need to link to
>> libpcap even they don't use it although this is not an important issue.
> 
> I don't think linking the non test_progs binaries to libpcap or not is 
> important.
> 
> I am positive there are ways out of it without adding the networking 
> codes to the test_progs.c. It sounds like an unnecessary nit now but I 
> believe it is useful going forward when making changes and extension to 
> the traffic monitoring. May be brainstorm a little to see if there is an 
> way out.
> 
> One way could be putting them in a new traffic_monitor.c such that the 
> non test_progs binaries won't link to it. and exports the test name and 
> shmod_tmon in test_progs.h (e.g. through function).
> 
> Another way (better and my preference if it works out) is to ask the 
> traffic_monitor_start() to take the the pcap file name args and makeup a 
> reasonable default if no filename is given.  Not that I am promoting non 
> test_progs tests, traffic_monitor_start() can then be reused by others 
> for legit reason. The test_progs's tests usually should not use 
> traffic_monitor_start() directly and they should stay with the 
> netns_{new, free}. I think only netns_new needs the env to figure out 
> the should_tmon and the pcap filename. May be netns_new() can stay in 
> test_progs.c, or rename it to test__netns_new().
> 
> wdyt?

How about put two ideas together?
Have traffic_monitor.c and macros in test_progs.h to collect
data from env, and pass the data to netns_new() in traffic_monitor.c.

For example,

#define test__netns_new(ns) netns_new(ns, env.test->should_tmon || \
             (env.subtest_state && env.subtest_state->should_tmon), \
             env.test->test_name,                                   \
             env.subtest_state ? env.subtest_state->name: NULL)

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

[Martin KaFai Lau]

On 8/2/24 1:37 PM, Kui-Feng Lee wrote:
>> One way could be putting them in a new traffic_monitor.c such that the non 
>> test_progs binaries won't link to it. and exports the test name and shmod_tmon 
>> in test_progs.h (e.g. through function).
>>
>> Another way (better and my preference if it works out) is to ask the 
>> traffic_monitor_start() to take the the pcap file name args and makeup a 
>> reasonable default if no filename is given.  Not that I am promoting non 
>> test_progs tests, traffic_monitor_start() can then be reused by others for 
>> legit reason. The test_progs's tests usually should not use 
>> traffic_monitor_start() directly and they should stay with the netns_{new, 
>> free}. I think only netns_new needs the env to figure out the should_tmon and 
>> the pcap filename. May be netns_new() can stay in test_progs.c, or rename it 
>> to test__netns_new().
>>
>> wdyt?
> 
> How about put two ideas together?
> Have traffic_monitor.c and macros in test_progs.h to collect
> data from env, and pass the data to netns_new() in traffic_monitor.c.
> 
> For example,
> 
> #define test__netns_new(ns) netns_new(ns, env.test->should_tmon || \
>              (env.subtest_state && env.subtest_state->should_tmon), \
>              env.test->test_name,                                   \
>              env.subtest_state ? env.subtest_state->name: NULL)
> 

The macro looks ok. I am not sure if it is easier as a macro in .h or just a 
func in test_progs.c. A quick look is the struct of env.test is not defined in 
.h. Just a thought.

If we have this macro/func, a quick thought is there is not much upside to 
create a new traffic_monitor.c instead of putting everything in 
network_helpers.c. I am fine either way. The other non test_progs binaries just 
need to link another traffic_monitor.o in the future if it wants to do 
traffic_monitor_start().

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

[Kui-Feng Lee]

On 8/1/24 21:35, Kui-Feng Lee wrote:
> 
> 
> On 8/1/24 20:43, Martin KaFai Lau wrote:
>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>> diff --git a/tools/testing/selftests/bpf/test_progs.h 
>>> b/tools/testing/selftests/bpf/test_progs.h
>>> index cb9d6d46826b..5d4e61fa26a1 100644
>>> --- a/tools/testing/selftests/bpf/test_progs.h
>>> +++ b/tools/testing/selftests/bpf/test_progs.h
>>> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct test_loader 
>>> *tester);
>>>       test_loader_fini(&tester);                           \
>>>   })
>>> +struct tmonitor_ctx;
>>> +
>>> +#ifdef TRAFFIC_MONITOR
>>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
>>> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>>> +#else
>>> +static inline struct tmonitor_ctx *traffic_monitor_start(const char 
>>> *netns)
>>> +{
>>> +    return (struct tmonitor_ctx *)-1;
>>
>> hmm... from peeking patch 3, only NULL is checked.

When traffic monitor is disable, these two functions are noop.
Returning -1 (not NULL) is convenient for the callers. They don't need
to tell if the error caused by a real error or by the disabled
feature.

>>
>> While at it, if there is no libpcap during make, is the "-m" option 
>> available or the test_progs will error out if "-m" is used?
> 
> "-m" is still available so CI can always pass "-m" without consider
> the configuration of the binary. But, it would be good idea to
> print a warning message for this situation.
> 
>>
>>> +}
>>> +
>>> +static inline void traffic_monitor_stop(struct tmonitor_ctx *ctx)
>>> +{
>>> +}
>>> +#endif
>>> +
>>>   #endif /* __TEST_PROGS_H */
>>

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

[Martin KaFai Lau]

On 8/6/24 3:07 PM, Kui-Feng Lee wrote:
> 
> 
> On 8/1/24 21:35, Kui-Feng Lee wrote:
>>
>>
>> On 8/1/24 20:43, Martin KaFai Lau wrote:
>>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>>> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/ 
>>>> selftests/bpf/test_progs.h
>>>> index cb9d6d46826b..5d4e61fa26a1 100644
>>>> --- a/tools/testing/selftests/bpf/test_progs.h
>>>> +++ b/tools/testing/selftests/bpf/test_progs.h
>>>> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct test_loader *tester);
>>>>       test_loader_fini(&tester);                           \
>>>>   })
>>>> +struct tmonitor_ctx;
>>>> +
>>>> +#ifdef TRAFFIC_MONITOR
>>>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
>>>> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>>>> +#else
>>>> +static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns)
>>>> +{
>>>> +    return (struct tmonitor_ctx *)-1;
>>>
>>> hmm... from peeking patch 3, only NULL is checked.
> 
> When traffic monitor is disable, these two functions are noop.
> Returning -1 (not NULL) is convenient for the callers. They don't need
> to tell if the error caused by a real error or by the disabled
> feature.

I pasted the code from patch 3 here only to ensure I understand the above 
explanation correctly:

+		netns_obj->tmon = traffic_monitor_start(name);
+		if (!netns_obj->tmon)
		    ^^^^^^^^^^^^^^^^

+			goto fail;

Does it mean the traffic_monitor_start() above will never be called if 
TRAFFIC_MONITOR macro is not defined such that traffic_monitor_start() returning 
-1 but testing for NULL here does not matter?

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

[Kui-Feng Lee]

On 8/6/24 15:22, Martin KaFai Lau wrote:
> On 8/6/24 3:07 PM, Kui-Feng Lee wrote:
>>
>>
>> On 8/1/24 21:35, Kui-Feng Lee wrote:
>>>
>>>
>>> On 8/1/24 20:43, Martin KaFai Lau wrote:
>>>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>>>> diff --git a/tools/testing/selftests/bpf/test_progs.h 
>>>>> b/tools/testing/ selftests/bpf/test_progs.h
>>>>> index cb9d6d46826b..5d4e61fa26a1 100644
>>>>> --- a/tools/testing/selftests/bpf/test_progs.h
>>>>> +++ b/tools/testing/selftests/bpf/test_progs.h
>>>>> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct 
>>>>> test_loader *tester);
>>>>>       test_loader_fini(&tester);                           \
>>>>>   })
>>>>> +struct tmonitor_ctx;
>>>>> +
>>>>> +#ifdef TRAFFIC_MONITOR
>>>>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
>>>>> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>>>>> +#else
>>>>> +static inline struct tmonitor_ctx *traffic_monitor_start(const 
>>>>> char *netns)
>>>>> +{
>>>>> +    return (struct tmonitor_ctx *)-1;
>>>>
>>>> hmm... from peeking patch 3, only NULL is checked.
>>
>> When traffic monitor is disable, these two functions are noop.
>> Returning -1 (not NULL) is convenient for the callers. They don't need
>> to tell if the error caused by a real error or by the disabled
>> feature.
> 
> I pasted the code from patch 3 here only to ensure I understand the 
> above explanation correctly:
> 
> +        netns_obj->tmon = traffic_monitor_start(name);
> +        if (!netns_obj->tmon)
>              ^^^^^^^^^^^^^^^^
> 
> +            goto fail;
> 
> Does it mean the traffic_monitor_start() above will never be called if 
> TRAFFIC_MONITOR macro is not defined such that traffic_monitor_start() 
> returning -1 but testing for NULL here does not matter?

Correct!

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

[Martin KaFai Lau]

On 8/6/24 4:18 PM, Kui-Feng Lee wrote:
> 
> 
> On 8/6/24 15:22, Martin KaFai Lau wrote:
>> On 8/6/24 3:07 PM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 8/1/24 21:35, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 8/1/24 20:43, Martin KaFai Lau wrote:
>>>>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>>>>> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/ 
>>>>>> selftests/bpf/test_progs.h
>>>>>> index cb9d6d46826b..5d4e61fa26a1 100644
>>>>>> --- a/tools/testing/selftests/bpf/test_progs.h
>>>>>> +++ b/tools/testing/selftests/bpf/test_progs.h
>>>>>> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct test_loader 
>>>>>> *tester);
>>>>>>       test_loader_fini(&tester);                           \
>>>>>>   })
>>>>>> +struct tmonitor_ctx;
>>>>>> +
>>>>>> +#ifdef TRAFFIC_MONITOR
>>>>>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
>>>>>> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>>>>>> +#else
>>>>>> +static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns)
>>>>>> +{
>>>>>> +    return (struct tmonitor_ctx *)-1;
>>>>>
>>>>> hmm... from peeking patch 3, only NULL is checked.
>>>
>>> When traffic monitor is disable, these two functions are noop.
>>> Returning -1 (not NULL) is convenient for the callers. They don't need
>>> to tell if the error caused by a real error or by the disabled
>>> feature.
>>
>> I pasted the code from patch 3 here only to ensure I understand the above 
>> explanation correctly:
>>
>> +        netns_obj->tmon = traffic_monitor_start(name);
>> +        if (!netns_obj->tmon)
>>              ^^^^^^^^^^^^^^^^
>>
>> +            goto fail;
>>
>> Does it mean the traffic_monitor_start() above will never be called if 
>> TRAFFIC_MONITOR macro is not defined such that traffic_monitor_start() 
>> returning -1 but testing for NULL here does not matter?
> 
> Correct!

Got it. Then I missed some understanding. Can you explain why the above 
traffic_monitor_start() will never be called?

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

[Kui-Feng Lee]

On 8/6/24 16:41, Martin KaFai Lau wrote:
> On 8/6/24 4:18 PM, Kui-Feng Lee wrote:
>>
>>
>> On 8/6/24 15:22, Martin KaFai Lau wrote:
>>> On 8/6/24 3:07 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 8/1/24 21:35, Kui-Feng Lee wrote:
>>>>>
>>>>>
>>>>> On 8/1/24 20:43, Martin KaFai Lau wrote:
>>>>>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>>>>>> diff --git a/tools/testing/selftests/bpf/test_progs.h 
>>>>>>> b/tools/testing/ selftests/bpf/test_progs.h
>>>>>>> index cb9d6d46826b..5d4e61fa26a1 100644
>>>>>>> --- a/tools/testing/selftests/bpf/test_progs.h
>>>>>>> +++ b/tools/testing/selftests/bpf/test_progs.h
>>>>>>> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct 
>>>>>>> test_loader *tester);
>>>>>>>       test_loader_fini(&tester);                           \
>>>>>>>   })
>>>>>>> +struct tmonitor_ctx;
>>>>>>> +
>>>>>>> +#ifdef TRAFFIC_MONITOR
>>>>>>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
>>>>>>> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>>>>>>> +#else
>>>>>>> +static inline struct tmonitor_ctx *traffic_monitor_start(const 
>>>>>>> char *netns)
>>>>>>> +{
>>>>>>> +    return (struct tmonitor_ctx *)-1;
>>>>>>
>>>>>> hmm... from peeking patch 3, only NULL is checked.
>>>>
>>>> When traffic monitor is disable, these two functions are noop.
>>>> Returning -1 (not NULL) is convenient for the callers. They don't need
>>>> to tell if the error caused by a real error or by the disabled
>>>> feature.
>>>
>>> I pasted the code from patch 3 here only to ensure I understand the 
>>> above explanation correctly:
>>>
>>> +        netns_obj->tmon = traffic_monitor_start(name);
>>> +        if (!netns_obj->tmon)
>>>              ^^^^^^^^^^^^^^^^
>>>
>>> +            goto fail;
>>>
>>> Does it mean the traffic_monitor_start() above will never be called 
>>> if TRAFFIC_MONITOR macro is not defined such that 
>>> traffic_monitor_start() returning -1 but testing for NULL here does 
>>> not matter?
>>
>> Correct!
> 
> Got it. Then I missed some understanding. Can you explain why the above 
> traffic_monitor_start() will never be called?
> 

Sorry! Forget my previous word.

In the case that TRAFFIC_MONITOR is not defined,
traffic_monitor_start(name) always returns -1.
So, "!netns_obj->tmon" is always false. "goto fail" is never executed.
That means the test will keep going just like that traffic monitor is
enabled and started correctly.