* [PATCH bpf-next v5 2/3] selftests/bpf: Allow assigning traffic monitor print function
2025-03-05 18:20 [PATCH bpf-next v5 1/3] selftests/bpf: Clean up call sites of stdio_restore() Amery Hung
@ 2025-03-05 18:20 ` Amery Hung
2025-03-05 18:20 ` [PATCH bpf-next v5 3/3] selftests/bpf: Fix dangling stdout seen by traffic monitor thread Amery Hung
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Amery Hung @ 2025-03-05 18:20 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, alexei.starovoitov, martin.lau, eddyz87,
ameryhung, kernel-team
Allow users to change traffic monitor's print function. If not provided,
traffic monitor will print to stdout by default.
Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
tools/testing/selftests/bpf/network_helpers.c | 69 ++++++++++++++-----
tools/testing/selftests/bpf/network_helpers.h | 8 +++
2 files changed, 58 insertions(+), 19 deletions(-)
diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index 737a952dcf80..11f07c82166b 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -750,6 +750,36 @@ struct tmonitor_ctx {
int pcap_fd;
};
+static int __base_pr(const char *format, va_list args)
+{
+ return vfprintf(stdout, format, args);
+}
+
+static tm_print_fn_t __tm_pr = __base_pr;
+
+tm_print_fn_t traffic_monitor_set_print(tm_print_fn_t fn)
+{
+ tm_print_fn_t old_print_fn;
+
+ old_print_fn = __atomic_exchange_n(&__tm_pr, fn, __ATOMIC_RELAXED);
+
+ return old_print_fn;
+}
+
+void tm_print(const char *format, ...)
+{
+ tm_print_fn_t print_fn;
+ va_list args;
+
+ print_fn = __atomic_load_n(&__tm_pr, __ATOMIC_RELAXED);
+ if (!print_fn)
+ return;
+
+ va_start(args, format);
+ __tm_pr(format, args);
+ va_end(args);
+}
+
/* Is this packet captured with a Ethernet protocol type? */
static bool is_ethernet(const u_char *packet)
{
@@ -767,7 +797,7 @@ static bool is_ethernet(const u_char *packet)
case 770: /* ARPHRD_FRAD */
case 778: /* ARPHDR_IPGRE */
case 803: /* ARPHRD_IEEE80211_RADIOTAP */
- printf("Packet captured: arphdr_type=%d\n", arphdr_type);
+ tm_print("Packet captured: arphdr_type=%d\n", arphdr_type);
return false;
}
return true;
@@ -817,19 +847,19 @@ static void show_transport(const u_char *packet, u16 len, u32 ifindex,
dst_port = ntohs(tcp->dest);
transport_str = "TCP";
} else if (proto == IPPROTO_ICMP) {
- printf("%-7s %-3s IPv4 %s > %s: ICMP, length %d, type %d, code %d\n",
- ifname, pkt_type_str(pkt_type), src_addr, dst_addr, len,
- packet[0], packet[1]);
+ tm_print("%-7s %-3s IPv4 %s > %s: ICMP, length %d, type %d, code %d\n",
+ ifname, pkt_type_str(pkt_type), src_addr, dst_addr, len,
+ packet[0], packet[1]);
return;
} else if (proto == IPPROTO_ICMPV6) {
- printf("%-7s %-3s IPv6 %s > %s: ICMPv6, length %d, type %d, code %d\n",
- ifname, pkt_type_str(pkt_type), src_addr, dst_addr, len,
- packet[0], packet[1]);
+ tm_print("%-7s %-3s IPv6 %s > %s: ICMPv6, length %d, type %d, code %d\n",
+ ifname, pkt_type_str(pkt_type), src_addr, dst_addr, len,
+ packet[0], packet[1]);
return;
} else {
- printf("%-7s %-3s %s %s > %s: protocol %d\n",
- ifname, pkt_type_str(pkt_type), ipv6 ? "IPv6" : "IPv4",
- src_addr, dst_addr, proto);
+ tm_print("%-7s %-3s %s %s > %s: protocol %d\n",
+ ifname, pkt_type_str(pkt_type), ipv6 ? "IPv6" : "IPv4",
+ src_addr, dst_addr, proto);
return;
}
@@ -843,13 +873,13 @@ static void show_transport(const u_char *packet, u16 len, u32 ifindex,
tcp->ack ? ", ACK" : "");
if (ipv6)
- printf("%-7s %-3s IPv6 %s.%d > %s.%d: %s, length %d%s\n",
- ifname, pkt_type_str(pkt_type), src_addr, src_port,
- dst_addr, dst_port, transport_str, len, flags);
+ tm_print("%-7s %-3s IPv6 %s.%d > %s.%d: %s, length %d%s\n",
+ ifname, pkt_type_str(pkt_type), src_addr, src_port,
+ dst_addr, dst_port, transport_str, len, flags);
else
- printf("%-7s %-3s IPv4 %s:%d > %s:%d: %s, length %d%s\n",
- ifname, pkt_type_str(pkt_type), src_addr, src_port,
- dst_addr, dst_port, transport_str, len, flags);
+ tm_print("%-7s %-3s IPv4 %s:%d > %s:%d: %s, length %d%s\n",
+ ifname, pkt_type_str(pkt_type), src_addr, src_port,
+ dst_addr, dst_port, transport_str, len, flags);
}
static void show_ipv6_packet(const u_char *packet, u32 ifindex, u8 pkt_type)
@@ -964,8 +994,8 @@ static void *traffic_monitor_thread(void *arg)
ifname = _ifname;
}
- printf("%-7s %-3s Unknown network protocol type 0x%x\n",
- ifname, pkt_type_str(ptype), proto);
+ tm_print("%-7s %-3s Unknown network protocol type 0x%x\n",
+ ifname, pkt_type_str(ptype), proto);
}
}
@@ -1165,8 +1195,9 @@ void traffic_monitor_stop(struct tmonitor_ctx *ctx)
write(ctx->wake_fd, &w, sizeof(w));
pthread_join(ctx->thread, NULL);
- printf("Packet file: %s\n", strrchr(ctx->pkt_fname, '/') + 1);
+ tm_print("Packet file: %s\n", strrchr(ctx->pkt_fname, '/') + 1);
traffic_monitor_release(ctx);
}
+
#endif /* TRAFFIC_MONITOR */
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 9f6e05d886c5..2d726f8f6327 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -249,10 +249,13 @@ static inline __sum16 build_udp_v6_csum(const struct ipv6hdr *ip6h,
struct tmonitor_ctx;
+typedef int (*tm_print_fn_t)(const char *format, va_list args);
+
#ifdef TRAFFIC_MONITOR
struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
const char *subtest_name);
void traffic_monitor_stop(struct tmonitor_ctx *ctx);
+tm_print_fn_t traffic_monitor_set_print(tm_print_fn_t fn);
#else
static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
const char *subtest_name)
@@ -263,6 +266,11 @@ static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns, cons
static inline void traffic_monitor_stop(struct tmonitor_ctx *ctx)
{
}
+
+static inline tm_print_fn_t traffic_monitor_set_print(tm_print_fn_t fn)
+{
+ return NULL;
+}
#endif
#endif
--
2.47.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH bpf-next v5 3/3] selftests/bpf: Fix dangling stdout seen by traffic monitor thread
2025-03-05 18:20 [PATCH bpf-next v5 1/3] selftests/bpf: Clean up call sites of stdio_restore() Amery Hung
2025-03-05 18:20 ` [PATCH bpf-next v5 2/3] selftests/bpf: Allow assigning traffic monitor print function Amery Hung
@ 2025-03-05 18:20 ` Amery Hung
2025-03-05 21:43 ` Eduard Zingerman
2025-03-05 21:42 ` [PATCH bpf-next v5 1/3] selftests/bpf: Clean up call sites of stdio_restore() Eduard Zingerman
2025-03-06 22:20 ` patchwork-bot+netdevbpf
3 siblings, 1 reply; 6+ messages in thread
From: Amery Hung @ 2025-03-05 18:20 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, alexei.starovoitov, martin.lau, eddyz87,
ameryhung, kernel-team
Traffic monitor thread may see dangling stdout as the main thread closes
and reassigns stdout without protection. This happens when the main thread
finishes one subtest and moves to another one in the same netns_new()
scope.
The issue can be reproduced by running test_progs repeatedly with traffic
monitor enabled:
for ((i=1;i<=100;i++)); do
./test_progs -a flow_dissector_skb* -m '*'
done
For restoring stdout in crash_handler(), since it does not really care
about closing stdout, simlpy flush stdout and restore it to the original
one.
Then, Fix the issue by consolidating stdio_restore_cleanup() and
stdio_restore(), and protecting the use/close/assignment of stdout with
a lock. The locking in the main thread is always performed regradless of
whether traffic monitor is running or not for simplicity. It won't have
any side-effect.
Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
tools/testing/selftests/bpf/test_progs.c | 39 +++++++++++++-----------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index ab0f2fed3c58..d4ec9586b98c 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -88,7 +88,9 @@ static void stdio_hijack(char **log_buf, size_t *log_cnt)
#endif
}
-static void stdio_restore_cleanup(void)
+static pthread_mutex_t stdout_lock = PTHREAD_MUTEX_INITIALIZER;
+
+static void stdio_restore(void)
{
#ifdef __GLIBC__
if (verbose() && env.worker_id == -1) {
@@ -98,6 +100,8 @@ static void stdio_restore_cleanup(void)
fflush(stdout);
+ pthread_mutex_lock(&stdout_lock);
+
if (env.subtest_state) {
fclose(env.subtest_state->stdout_saved);
env.subtest_state->stdout_saved = NULL;
@@ -106,26 +110,21 @@ static void stdio_restore_cleanup(void)
} else {
fclose(env.test_state->stdout_saved);
env.test_state->stdout_saved = NULL;
+ stdout = env.stdout_saved;
+ stderr = env.stderr_saved;
}
+
+ pthread_mutex_unlock(&stdout_lock);
#endif
}
-static void stdio_restore(void)
+static int traffic_monitor_print_fn(const char *format, va_list args)
{
-#ifdef __GLIBC__
- if (verbose() && env.worker_id == -1) {
- /* nothing to do, output to stdout by default */
- return;
- }
-
- if (stdout == env.stdout_saved)
- return;
-
- stdio_restore_cleanup();
+ pthread_mutex_lock(&stdout_lock);
+ vfprintf(stdout, format, args);
+ pthread_mutex_unlock(&stdout_lock);
- stdout = env.stdout_saved;
- stderr = env.stderr_saved;
-#endif
+ return 0;
}
/* Adapted from perf/util/string.c */
@@ -536,7 +535,8 @@ void test__end_subtest(void)
test_result(subtest_state->error_cnt,
subtest_state->skipped));
- stdio_restore_cleanup();
+ stdio_restore();
+
env.subtest_state = NULL;
}
@@ -1265,7 +1265,10 @@ void crash_handler(int signum)
sz = backtrace(bt, ARRAY_SIZE(bt));
- stdio_restore();
+ fflush(stdout);
+ stdout = env.stdout_saved;
+ stderr = env.stderr_saved;
+
if (env.test) {
env.test_state->error_cnt++;
dump_test_log(env.test, env.test_state, true, false, NULL);
@@ -1957,6 +1960,8 @@ int main(int argc, char **argv)
libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
libbpf_set_print(libbpf_print_fn);
+ traffic_monitor_set_print(traffic_monitor_print_fn);
+
srand(time(NULL));
env.jit_enabled = is_jit_enabled();
--
2.47.1
^ permalink raw reply related [flat|nested] 6+ messages in thread