* [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-07-31 19:31 [PATCH bpf-next v4 0/6] monitor network traffic for flaky test cases Kui-Feng Lee
@ 2024-07-31 19:31 ` Kui-Feng Lee
2024-07-31 21:07 ` Stanislav Fomichev
` (2 more replies)
2024-07-31 19:31 ` [PATCH bpf-next v4 2/6] selftests/bpf: Add the traffic monitor option to test_progs Kui-Feng Lee
` (4 subsequent siblings)
5 siblings, 3 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-07-31 19:31 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf, geliang
Cc: sinquersw, kuifeng, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-07-31 19:31 ` [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions Kui-Feng Lee
@ 2024-07-31 21:07 ` Stanislav Fomichev
2024-08-02 3:54 ` Kui-Feng Lee
2024-08-02 3:29 ` Martin KaFai Lau
2024-08-02 3:43 ` Martin KaFai Lau
2 siblings, 1 reply; 25+ messages in thread
From: Stanislav Fomichev @ 2024-07-31 21:07 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, geliang,
sinquersw, kuifeng
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")
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-07-31 21:07 ` Stanislav Fomichev
@ 2024-08-02 3:54 ` Kui-Feng Lee
0 siblings, 0 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-02 3:54 UTC (permalink / raw)
To: Stanislav Fomichev, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, geliang, kuifeng
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!
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-07-31 19:31 ` [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions Kui-Feng Lee
2024-07-31 21:07 ` Stanislav Fomichev
@ 2024-08-02 3:29 ` Martin KaFai Lau
2024-08-02 4:31 ` Kui-Feng Lee
2024-08-02 3:43 ` Martin KaFai Lau
2 siblings, 1 reply; 25+ messages in thread
From: Martin KaFai Lau @ 2024-08-02 3:29 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, sinquersw,
kuifeng
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;
> +}
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-02 3:29 ` Martin KaFai Lau
@ 2024-08-02 4:31 ` Kui-Feng Lee
2024-08-02 18:58 ` Martin KaFai Lau
0 siblings, 1 reply; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-02 4:31 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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;
>> +}
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-02 4:31 ` Kui-Feng Lee
@ 2024-08-02 18:58 ` Martin KaFai Lau
2024-08-02 20:37 ` Kui-Feng Lee
0 siblings, 1 reply; 25+ messages in thread
From: Martin KaFai Lau @ 2024-08-02 18:58 UTC (permalink / raw)
To: Kui-Feng Lee, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-02 18:58 ` Martin KaFai Lau
@ 2024-08-02 20:37 ` Kui-Feng Lee
2024-08-02 21:12 ` Martin KaFai Lau
0 siblings, 1 reply; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-02 20:37 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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)
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-02 20:37 ` Kui-Feng Lee
@ 2024-08-02 21:12 ` Martin KaFai Lau
0 siblings, 0 replies; 25+ messages in thread
From: Martin KaFai Lau @ 2024-08-02 21:12 UTC (permalink / raw)
To: Kui-Feng Lee, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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().
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-07-31 19:31 ` [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions Kui-Feng Lee
2024-07-31 21:07 ` Stanislav Fomichev
2024-08-02 3:29 ` Martin KaFai Lau
@ 2024-08-02 3:43 ` Martin KaFai Lau
2024-08-02 4:35 ` Kui-Feng Lee
2 siblings, 1 reply; 25+ messages in thread
From: Martin KaFai Lau @ 2024-08-02 3:43 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, sinquersw,
kuifeng
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 */
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-02 3:43 ` Martin KaFai Lau
@ 2024-08-02 4:35 ` Kui-Feng Lee
2024-08-06 22:07 ` Kui-Feng Lee
0 siblings, 1 reply; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-02 4:35 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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 */
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-02 4:35 ` Kui-Feng Lee
@ 2024-08-06 22:07 ` Kui-Feng Lee
2024-08-06 22:22 ` Martin KaFai Lau
0 siblings, 1 reply; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-06 22:07 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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 */
>>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-06 22:07 ` Kui-Feng Lee
@ 2024-08-06 22:22 ` Martin KaFai Lau
2024-08-06 23:18 ` Kui-Feng Lee
0 siblings, 1 reply; 25+ messages in thread
From: Martin KaFai Lau @ 2024-08-06 22:22 UTC (permalink / raw)
To: Kui-Feng Lee, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-06 22:22 ` Martin KaFai Lau
@ 2024-08-06 23:18 ` Kui-Feng Lee
2024-08-06 23:41 ` Martin KaFai Lau
0 siblings, 1 reply; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-06 23:18 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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!
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-06 23:18 ` Kui-Feng Lee
@ 2024-08-06 23:41 ` Martin KaFai Lau
2024-08-07 0:17 ` Kui-Feng Lee
0 siblings, 1 reply; 25+ messages in thread
From: Martin KaFai Lau @ 2024-08-06 23:41 UTC (permalink / raw)
To: Kui-Feng Lee, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.
2024-08-06 23:41 ` Martin KaFai Lau
@ 2024-08-07 0:17 ` Kui-Feng Lee
0 siblings, 0 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-07 0:17 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee
Cc: bpf, ast, song, kernel-team, andrii, sdf, geliang, kuifeng
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH bpf-next v4 2/6] selftests/bpf: Add the traffic monitor option to test_progs.
2024-07-31 19:31 [PATCH bpf-next v4 0/6] monitor network traffic for flaky test cases Kui-Feng Lee
2024-07-31 19:31 ` [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions Kui-Feng Lee
@ 2024-07-31 19:31 ` Kui-Feng Lee
2024-07-31 19:31 ` [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers Kui-Feng Lee
` (3 subsequent siblings)
5 siblings, 0 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-07-31 19:31 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf, geliang
Cc: sinquersw, kuifeng, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers.
2024-07-31 19:31 [PATCH bpf-next v4 0/6] monitor network traffic for flaky test cases Kui-Feng Lee
2024-07-31 19:31 ` [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions Kui-Feng Lee
2024-07-31 19:31 ` [PATCH bpf-next v4 2/6] selftests/bpf: Add the traffic monitor option to test_progs Kui-Feng Lee
@ 2024-07-31 19:31 ` Kui-Feng Lee
2024-07-31 21:02 ` Stanislav Fomichev
2024-07-31 19:31 ` [PATCH bpf-next v4 4/6] selftests/bpf: Monitor traffic for tc_redirect Kui-Feng Lee
` (2 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Kui-Feng Lee @ 2024-07-31 19:31 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf, geliang
Cc: sinquersw, kuifeng, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers.
2024-07-31 19:31 ` [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers Kui-Feng Lee
@ 2024-07-31 21:02 ` Stanislav Fomichev
2024-08-02 4:42 ` Kui-Feng Lee
0 siblings, 1 reply; 25+ messages in thread
From: Stanislav Fomichev @ 2024-07-31 21:02 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, geliang,
sinquersw, kuifeng
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.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers.
2024-07-31 21:02 ` Stanislav Fomichev
@ 2024-08-02 4:42 ` Kui-Feng Lee
0 siblings, 0 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-02 4:42 UTC (permalink / raw)
To: Stanislav Fomichev, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, geliang, kuifeng
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH bpf-next v4 4/6] selftests/bpf: Monitor traffic for tc_redirect.
2024-07-31 19:31 [PATCH bpf-next v4 0/6] monitor network traffic for flaky test cases Kui-Feng Lee
` (2 preceding siblings ...)
2024-07-31 19:31 ` [PATCH bpf-next v4 3/6] selftests/bpf: netns_new() and netns_free() helpers Kui-Feng Lee
@ 2024-07-31 19:31 ` Kui-Feng Lee
2024-07-31 19:31 ` [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen Kui-Feng Lee
2024-07-31 19:31 ` [PATCH bpf-next v4 6/6] selftests/bpf: Monitor traffic for select_reuseport Kui-Feng Lee
5 siblings, 0 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-07-31 19:31 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf, geliang
Cc: sinquersw, kuifeng, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen.
2024-07-31 19:31 [PATCH bpf-next v4 0/6] monitor network traffic for flaky test cases Kui-Feng Lee
` (3 preceding siblings ...)
2024-07-31 19:31 ` [PATCH bpf-next v4 4/6] selftests/bpf: Monitor traffic for tc_redirect Kui-Feng Lee
@ 2024-07-31 19:31 ` Kui-Feng Lee
2024-07-31 21:09 ` Stanislav Fomichev
2024-07-31 19:31 ` [PATCH bpf-next v4 6/6] selftests/bpf: Monitor traffic for select_reuseport Kui-Feng Lee
5 siblings, 1 reply; 25+ messages in thread
From: Kui-Feng Lee @ 2024-07-31 19:31 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf, geliang
Cc: sinquersw, kuifeng, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen.
2024-07-31 19:31 ` [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen Kui-Feng Lee
@ 2024-07-31 21:09 ` Stanislav Fomichev
2024-08-02 4:42 ` Kui-Feng Lee
0 siblings, 1 reply; 25+ messages in thread
From: Stanislav Fomichev @ 2024-07-31 21:09 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, geliang,
sinquersw, kuifeng
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".
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen.
2024-07-31 21:09 ` Stanislav Fomichev
@ 2024-08-02 4:42 ` Kui-Feng Lee
0 siblings, 0 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-08-02 4:42 UTC (permalink / raw)
To: Stanislav Fomichev, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, geliang, kuifeng
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!
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH bpf-next v4 6/6] selftests/bpf: Monitor traffic for select_reuseport.
2024-07-31 19:31 [PATCH bpf-next v4 0/6] monitor network traffic for flaky test cases Kui-Feng Lee
` (4 preceding siblings ...)
2024-07-31 19:31 ` [PATCH bpf-next v4 5/6] selftests/bpf: Monitor traffic for sockmap_listen Kui-Feng Lee
@ 2024-07-31 19:31 ` Kui-Feng Lee
5 siblings, 0 replies; 25+ messages in thread
From: Kui-Feng Lee @ 2024-07-31 19:31 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf, geliang
Cc: sinquersw, kuifeng, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread