diff mbox series

[bpf-next,v3,1/1] selftests/bpf: Fix dangling stdout seen by traffic monitor thread

Message ID 20250227222336.2236460-1-ameryhung@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,v3,1/1] selftests/bpf: Fix dangling stdout seen by traffic monitor thread | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers warning 12 maintainers not CCed: mykolal@fb.com song@kernel.org kpsingh@kernel.org shuah@kernel.org john.fastabend@gmail.com eddyz87@gmail.com jolsa@kernel.org yonghong.song@linux.dev haoluo@google.com linux-kselftest@vger.kernel.org sdf@fomichev.me martin.lau@linux.dev
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 11 this patch: 11
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel

Commit Message

Amery Hung Feb. 27, 2025, 10:23 p.m. UTC 
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. Fix it by first consolidating stdout assignment into
stdio_restore_cleanup() and then protecting the use/close/reassignment 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.

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

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 tools/testing/selftests/bpf/network_helpers.c |  8 ++++-
 tools/testing/selftests/bpf/network_helpers.h |  6 ++--
 tools/testing/selftests/bpf/test_progs.c      | 29 +++++++++++++------
 3 files changed, 31 insertions(+), 12 deletions(-)

Comments

Martin KaFai Lau Feb. 28, 2025, 11:30 p.m. UTC | #1 
On 2/27/25 2:23 PM, Amery Hung wrote:
> 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. Fix it by first consolidating stdout assignment into
> stdio_restore_cleanup() and then protecting the use/close/reassignment 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.
> 
> 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
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---
>   tools/testing/selftests/bpf/network_helpers.c |  8 ++++-
>   tools/testing/selftests/bpf/network_helpers.h |  6 ++--
>   tools/testing/selftests/bpf/test_progs.c      | 29 +++++++++++++------
>   3 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index 737a952dcf80..5014fd063d67 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -743,6 +743,7 @@ struct tmonitor_ctx {
>   	pcap_t *pcap;
>   	pcap_dumper_t *dumper;
>   	pthread_t thread;
> +	pthread_mutex_t *stdout_lock;
>   	int wake_fd;
>   
>   	volatile bool done;
> @@ -953,6 +954,7 @@ static void *traffic_monitor_thread(void *arg)
>   		ifindex = ntohl(ifindex);
>   		ptype = packet[10];
>   
> +		pthread_mutex_lock(ctx->stdout_lock);
>   		if (proto == ETH_P_IPV6) {
>   			show_ipv6_packet(payload, ifindex, ptype);
>   		} else if (proto == ETH_P_IP) {
> @@ -967,6 +969,7 @@ static void *traffic_monitor_thread(void *arg)
>   			printf("%-7s %-3s Unknown network protocol type 0x%x\n",
>   			       ifname, pkt_type_str(ptype), proto);
>   		}
> +		pthread_mutex_unlock(ctx->stdout_lock);
>   	}
>   
>   	return NULL;
> @@ -1055,7 +1058,8 @@ static void encode_test_name(char *buf, size_t len, const char *test_name, const
>    * in the give network namespace.
>    */
>   struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
> -					   const char *subtest_name)
> +					   const char *subtest_name,
> +					   pthread_mutex_t *stdout_lock)

Thinking out loud here and see if the following will be better than passing a 
pthread_mutex_t.

How about passing a print function pointer instead and this function can do what 
is needed before printf(), i.e. lock mutex here. Something like the 
"libbpf_print_fn_t __libbpf_pr" in libbpf.c.

May be the test_progs.c can set it once by calling tm_set_print (similar to 
libbpf_set_print) instead of passing as an arg during every traffic_monitor_start().

wdyt?

>   {
>   	struct nstoken *nstoken = NULL;
>   	struct tmonitor_ctx *ctx;
> @@ -1109,6 +1113,8 @@ struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_n
>   		goto fail_eventfd;
>   	}
>   
> +	ctx->stdout_lock = stdout_lock;
> +
>   	r = pthread_create(&ctx->thread, NULL, traffic_monitor_thread, ctx);
>   	if (r) {
>   		log_err("Failed to create thread");
> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> index 9f6e05d886c5..b80954eab8d8 100644
> --- a/tools/testing/selftests/bpf/network_helpers.h
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -251,11 +251,13 @@ struct tmonitor_ctx;
>   
>   #ifdef TRAFFIC_MONITOR
>   struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
> -					   const char *subtest_name);
> +					   const char *subtest_name,
> +					   pthread_mutex_t *stdout_lock);
>   void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>   #else
>   static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
> -							 const char *subtest_name)
> +							 const char *subtest_name,
> +							 pthread_mutex_t *stdout_lock)
>   {
>   	return NULL;
>   }
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 0cb759632225..db9ea69e8ba1 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_cleanup(bool restore_default)
>   {
>   #ifdef __GLIBC__
>   	if (verbose() && env.worker_id == -1) {
> @@ -98,15 +100,25 @@ 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;
> -		stdout = env.test_state->stdout_saved;
> -		stderr = env.test_state->stdout_saved;
>   	} else {
>   		fclose(env.test_state->stdout_saved);
>   		env.test_state->stdout_saved = NULL;
>   	}
> +
> +	if (restore_default) {

Why a new "bool restore_default" is needed? Testing env.subtest_state is not enough?

Thanks for debugging this.

> +		stdout = env.stdout_saved;
> +		stderr = env.stderr_saved;
> +	} else if (env.subtest_state) {
> +		stdout = env.test_state->stdout_saved;
> +		stderr = env.test_state->stdout_saved;
> +	}
> +
> +	pthread_mutex_unlock(&stdout_lock);
>   #endif
>   }
>   
> @@ -121,10 +133,7 @@ static void stdio_restore(void)
>   	if (stdout == env.stdout_saved)
>   		return;
>   
> -	stdio_restore_cleanup();
> -
> -	stdout = env.stdout_saved;
> -	stderr = env.stderr_saved;
> +	stdio_restore_cleanup(true);
>   #endif
>   }
>   
> @@ -541,7 +550,8 @@ void test__end_subtest(void)
>   				   test_result(subtest_state->error_cnt,
>   					       subtest_state->skipped));
>   
> -	stdio_restore_cleanup();
> +	stdio_restore_cleanup(false);
> +
>   	env.subtest_state = NULL;
>   }
>   
> @@ -779,7 +789,8 @@ struct netns_obj *netns_new(const char *nsname, bool open)
>   	    (env.subtest_state && env.subtest_state->should_tmon)) {
>   		test_name = env.test->test_name;
>   		subtest_name = env.subtest_state ? env.subtest_state->name : NULL;
> -		netns_obj->tmon = traffic_monitor_start(nsname, test_name, subtest_name);
> +		netns_obj->tmon = traffic_monitor_start(nsname, test_name, subtest_name,
> +							&stdout_lock);
>   		if (!netns_obj->tmon) {
>   			fprintf(stderr, "Failed to start traffic monitor for %s\n", nsname);
>   			goto fail;
Amery Hung March 1, 2025, 12:10 a.m. UTC | #2 
On Fri, Feb 28, 2025 at 3:30 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/27/25 2:23 PM, Amery Hung wrote:
> > 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. Fix it by first consolidating stdout assignment into
> > stdio_restore_cleanup() and then protecting the use/close/reassignment 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.
> >
> > 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
> >
> > Signed-off-by: Amery Hung <ameryhung@gmail.com>
> > ---
> >   tools/testing/selftests/bpf/network_helpers.c |  8 ++++-
> >   tools/testing/selftests/bpf/network_helpers.h |  6 ++--
> >   tools/testing/selftests/bpf/test_progs.c      | 29 +++++++++++++------
> >   3 files changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> > index 737a952dcf80..5014fd063d67 100644
> > --- a/tools/testing/selftests/bpf/network_helpers.c
> > +++ b/tools/testing/selftests/bpf/network_helpers.c
> > @@ -743,6 +743,7 @@ struct tmonitor_ctx {
> >       pcap_t *pcap;
> >       pcap_dumper_t *dumper;
> >       pthread_t thread;
> > +     pthread_mutex_t *stdout_lock;
> >       int wake_fd;
> >
> >       volatile bool done;
> > @@ -953,6 +954,7 @@ static void *traffic_monitor_thread(void *arg)
> >               ifindex = ntohl(ifindex);
> >               ptype = packet[10];
> >
> > +             pthread_mutex_lock(ctx->stdout_lock);
> >               if (proto == ETH_P_IPV6) {
> >                       show_ipv6_packet(payload, ifindex, ptype);
> >               } else if (proto == ETH_P_IP) {
> > @@ -967,6 +969,7 @@ static void *traffic_monitor_thread(void *arg)
> >                       printf("%-7s %-3s Unknown network protocol type 0x%x\n",
> >                              ifname, pkt_type_str(ptype), proto);
> >               }
> > +             pthread_mutex_unlock(ctx->stdout_lock);
> >       }
> >
> >       return NULL;
> > @@ -1055,7 +1058,8 @@ static void encode_test_name(char *buf, size_t len, const char *test_name, const
> >    * in the give network namespace.
> >    */
> >   struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
> > -                                        const char *subtest_name)
> > +                                        const char *subtest_name,
> > +                                        pthread_mutex_t *stdout_lock)
>
> Thinking out loud here and see if the following will be better than passing a
> pthread_mutex_t.
>
> How about passing a print function pointer instead and this function can do what
> is needed before printf(), i.e. lock mutex here. Something like the
> "libbpf_print_fn_t __libbpf_pr" in libbpf.c.
>
> May be the test_progs.c can set it once by calling tm_set_print (similar to
> libbpf_set_print) instead of passing as an arg during every traffic_monitor_start().
>
> wdyt?
>

I think that will be cleaner. Especially explicit
pthread_mutext_lock/unlock() can then be removed from traffic monitor.
I will change to passing a print function pointer from test_progs.c to
traffic monitor.

> >   {
> >       struct nstoken *nstoken = NULL;
> >       struct tmonitor_ctx *ctx;
> > @@ -1109,6 +1113,8 @@ struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_n
> >               goto fail_eventfd;
> >       }
> >
> > +     ctx->stdout_lock = stdout_lock;
> > +
> >       r = pthread_create(&ctx->thread, NULL, traffic_monitor_thread, ctx);
> >       if (r) {
> >               log_err("Failed to create thread");
> > diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> > index 9f6e05d886c5..b80954eab8d8 100644
> > --- a/tools/testing/selftests/bpf/network_helpers.h
> > +++ b/tools/testing/selftests/bpf/network_helpers.h
> > @@ -251,11 +251,13 @@ struct tmonitor_ctx;
> >
> >   #ifdef TRAFFIC_MONITOR
> >   struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
> > -                                        const char *subtest_name);
> > +                                        const char *subtest_name,
> > +                                        pthread_mutex_t *stdout_lock);
> >   void traffic_monitor_stop(struct tmonitor_ctx *ctx);
> >   #else
> >   static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
> > -                                                      const char *subtest_name)
> > +                                                      const char *subtest_name,
> > +                                                      pthread_mutex_t *stdout_lock)
> >   {
> >       return NULL;
> >   }
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 0cb759632225..db9ea69e8ba1 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_cleanup(bool restore_default)
> >   {
> >   #ifdef __GLIBC__
> >       if (verbose() && env.worker_id == -1) {
> > @@ -98,15 +100,25 @@ 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;
> > -             stdout = env.test_state->stdout_saved;
> > -             stderr = env.test_state->stdout_saved;
> >       } else {
> >               fclose(env.test_state->stdout_saved);
> >               env.test_state->stdout_saved = NULL;
> >       }
> > +
> > +     if (restore_default) {
>
> Why a new "bool restore_default" is needed? Testing env.subtest_state is not enough?
>

If a crash happens during a subtest, env.subtest_state will still be
set and stdout and stderr will not be restored to the default ones.

Thanks,
Amery

> Thanks for debugging this.
>
> > +             stdout = env.stdout_saved;
> > +             stderr = env.stderr_saved;
> > +     } else if (env.subtest_state) {
> > +             stdout = env.test_state->stdout_saved;
> > +             stderr = env.test_state->stdout_saved;
> > +     }
> > +
> > +     pthread_mutex_unlock(&stdout_lock);
> >   #endif
> >   }
> >
> > @@ -121,10 +133,7 @@ static void stdio_restore(void)
> >       if (stdout == env.stdout_saved)
> >               return;
> >
> > -     stdio_restore_cleanup();
> > -
> > -     stdout = env.stdout_saved;
> > -     stderr = env.stderr_saved;
> > +     stdio_restore_cleanup(true);
> >   #endif
> >   }
> >
> > @@ -541,7 +550,8 @@ void test__end_subtest(void)
> >                                  test_result(subtest_state->error_cnt,
> >                                              subtest_state->skipped));
> >
> > -     stdio_restore_cleanup();
> > +     stdio_restore_cleanup(false);
> > +
> >       env.subtest_state = NULL;
> >   }
> >
> > @@ -779,7 +789,8 @@ struct netns_obj *netns_new(const char *nsname, bool open)
> >           (env.subtest_state && env.subtest_state->should_tmon)) {
> >               test_name = env.test->test_name;
> >               subtest_name = env.subtest_state ? env.subtest_state->name : NULL;
> > -             netns_obj->tmon = traffic_monitor_start(nsname, test_name, subtest_name);
> > +             netns_obj->tmon = traffic_monitor_start(nsname, test_name, subtest_name,
> > +                                                     &stdout_lock);
> >               if (!netns_obj->tmon) {
> >                       fprintf(stderr, "Failed to start traffic monitor for %s\n", nsname);
> >                       goto fail;
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index 737a952dcf80..5014fd063d67 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -743,6 +743,7 @@  struct tmonitor_ctx {
 	pcap_t *pcap;
 	pcap_dumper_t *dumper;
 	pthread_t thread;
+	pthread_mutex_t *stdout_lock;
 	int wake_fd;
 
 	volatile bool done;
@@ -953,6 +954,7 @@  static void *traffic_monitor_thread(void *arg)
 		ifindex = ntohl(ifindex);
 		ptype = packet[10];
 
+		pthread_mutex_lock(ctx->stdout_lock);
 		if (proto == ETH_P_IPV6) {
 			show_ipv6_packet(payload, ifindex, ptype);
 		} else if (proto == ETH_P_IP) {
@@ -967,6 +969,7 @@  static void *traffic_monitor_thread(void *arg)
 			printf("%-7s %-3s Unknown network protocol type 0x%x\n",
 			       ifname, pkt_type_str(ptype), proto);
 		}
+		pthread_mutex_unlock(ctx->stdout_lock);
 	}
 
 	return NULL;
@@ -1055,7 +1058,8 @@  static void encode_test_name(char *buf, size_t len, const char *test_name, const
  * in the give network namespace.
  */
 struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
-					   const char *subtest_name)
+					   const char *subtest_name,
+					   pthread_mutex_t *stdout_lock)
 {
 	struct nstoken *nstoken = NULL;
 	struct tmonitor_ctx *ctx;
@@ -1109,6 +1113,8 @@  struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_n
 		goto fail_eventfd;
 	}
 
+	ctx->stdout_lock = stdout_lock;
+
 	r = pthread_create(&ctx->thread, NULL, traffic_monitor_thread, ctx);
 	if (r) {
 		log_err("Failed to create thread");
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 9f6e05d886c5..b80954eab8d8 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -251,11 +251,13 @@  struct tmonitor_ctx;
 
 #ifdef TRAFFIC_MONITOR
 struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
-					   const char *subtest_name);
+					   const char *subtest_name,
+					   pthread_mutex_t *stdout_lock);
 void traffic_monitor_stop(struct tmonitor_ctx *ctx);
 #else
 static inline struct tmonitor_ctx *traffic_monitor_start(const char *netns, const char *test_name,
-							 const char *subtest_name)
+							 const char *subtest_name,
+							 pthread_mutex_t *stdout_lock)
 {
 	return NULL;
 }
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 0cb759632225..db9ea69e8ba1 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_cleanup(bool restore_default)
 {
 #ifdef __GLIBC__
 	if (verbose() && env.worker_id == -1) {
@@ -98,15 +100,25 @@  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;
-		stdout = env.test_state->stdout_saved;
-		stderr = env.test_state->stdout_saved;
 	} else {
 		fclose(env.test_state->stdout_saved);
 		env.test_state->stdout_saved = NULL;
 	}
+
+	if (restore_default) {
+		stdout = env.stdout_saved;
+		stderr = env.stderr_saved;
+	} else if (env.subtest_state) {
+		stdout = env.test_state->stdout_saved;
+		stderr = env.test_state->stdout_saved;
+	}
+
+	pthread_mutex_unlock(&stdout_lock);
 #endif
 }
 
@@ -121,10 +133,7 @@  static void stdio_restore(void)
 	if (stdout == env.stdout_saved)
 		return;
 
-	stdio_restore_cleanup();
-
-	stdout = env.stdout_saved;
-	stderr = env.stderr_saved;
+	stdio_restore_cleanup(true);
 #endif
 }
 
@@ -541,7 +550,8 @@  void test__end_subtest(void)
 				   test_result(subtest_state->error_cnt,
 					       subtest_state->skipped));
 
-	stdio_restore_cleanup();
+	stdio_restore_cleanup(false);
+
 	env.subtest_state = NULL;
 }
 
@@ -779,7 +789,8 @@  struct netns_obj *netns_new(const char *nsname, bool open)
 	    (env.subtest_state && env.subtest_state->should_tmon)) {
 		test_name = env.test->test_name;
 		subtest_name = env.subtest_state ? env.subtest_state->name : NULL;
-		netns_obj->tmon = traffic_monitor_start(nsname, test_name, subtest_name);
+		netns_obj->tmon = traffic_monitor_start(nsname, test_name, subtest_name,
+							&stdout_lock);
 		if (!netns_obj->tmon) {
 			fprintf(stderr, "Failed to start traffic monitor for %s\n", nsname);
 			goto fail;