diff mbox series

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

Message ID 20250304163626.1362031-3-ameryhung@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next,v4,1/3] selftests/bpf: Clean up call sites of stdio_restore() | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format warning Series does not have a cover letter
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 (+0) this patch: 26 (+0)
netdev/cc_maintainers warning 12 maintainers not CCed: jolsa@kernel.org kpsingh@kernel.org linux-kselftest@vger.kernel.org sdf@fomichev.me mykolal@fb.com john.fastabend@gmail.com haoluo@google.com martin.lau@linux.dev eddyz87@gmail.com shuah@kernel.org song@kernel.org yonghong.song@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 success total: 0 errors, 0 warnings, 0 checks, 99 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
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-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-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-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-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-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
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-17 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
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-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
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-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-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
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-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
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-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
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-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
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-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
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-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
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-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
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-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
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-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-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
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-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
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-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18

Commit Message

Amery Hung March 4, 2025, 4:36 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.

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

Fix it by first consolidating stdout assignment into stdio_restore().
stdout will be restored to env.stdout_saved when a test ends or running
in the crash handler and to test_state.stdout_saved otherwise.
Then, protect 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.
stdio_restore() is kept in the crash handler instead of making all print
functions in the crash handler use env.stdout_saved to make it less
error-prone.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 tools/testing/selftests/bpf/test_progs.c | 59 ++++++++++++++++--------
 1 file changed, 39 insertions(+), 20 deletions(-)

Comments

Eduard Zingerman March 5, 2025, 12:12 a.m. UTC | #1 
On Tue, 2025-03-04 at 08:36 -0800, 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.
> 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
> 
> Fix it by first consolidating stdout assignment into stdio_restore().
> stdout will be restored to env.stdout_saved when a test ends or running
> in the crash handler and to test_state.stdout_saved otherwise.
> Then, protect 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.
> stdio_restore() is kept in the crash handler instead of making all print
> functions in the crash handler use env.stdout_saved to make it less
> error-prone.
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---

This patch fixes the error for me.

Tested-by: Eduard Zingerman <eddyz87@gmail.com>

>  tools/testing/selftests/bpf/test_progs.c | 59 ++++++++++++++++--------
>  1 file changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index ab0f2fed3c58..5b89f6ca5a0a 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -88,7 +88,11 @@ 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 bool in_crash_handler(void);
> +
> +static void stdio_restore(void)
>  {
>  #ifdef __GLIBC__
>  	if (verbose() && env.worker_id == -1) {
> @@ -98,34 +102,34 @@ static void stdio_restore_cleanup(void)
>  
>  	fflush(stdout);
>  
> -	if (env.subtest_state) {
> +	pthread_mutex_lock(&stdout_lock);
> +
> +	if (!env.subtest_state || in_crash_handler()) {
> +		if (stdout == env.stdout_saved)
> +			goto out;
> +
> +		fclose(env.test_state->stdout_saved);
> +		env.test_state->stdout_saved = NULL;
> +		stdout = env.stdout_saved;
> +		stderr = env.stderr_saved;
> +	} else {
>  		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;
>  	}
> +out:
> +	pthread_mutex_unlock(&stdout_lock);
>  #endif
>  }

stdio_restore_cleanup() did not reset stderr/stdout when
env.subtest_state was NULL, but this difference does not seem to
matter, stdio_restore_cleanup() was called from:
- test__start_subtest(), where stdio_hijack_init() would override
  stderr/stdout anyways.
- run_one_test(), where it is followed by call to stdio_restore().

I think this change is Ok.

[...]

> @@ -1276,6 +1281,18 @@ void crash_handler(int signum)
>  	backtrace_symbols_fd(bt, sz, STDERR_FILENO);
>  }
>  
> +static bool in_crash_handler(void)
> +{
> +	struct sigaction sigact;
> +
> +	/* sa_handler will be cleared if invoked since crash_handler is
> +	 * registered with SA_RESETHAND
> +	 */
> +	sigaction(SIGSEGV, NULL, &sigact);
> +
> +	return sigact.sa_handler != crash_handler;
> +}
> +

The patch would be simpler w/o this function. I double checked
functions called from crash_handler() and two 'fprintf(stderr, ...)'
there are the only places where stderr/stdout is used instead of
*_saved versions. It is already a prevalent pattern to do
'fprintf(env.stderr_saved, ...)' in this file.
Or pass a flag as in v3?

>  void hexdump(const char *prefix, const void *buf, size_t len)
>  {
>  	for (int i = 0; i < len; i++) {
> @@ -1957,6 +1974,8 @@ int main(int argc, char **argv)

[...]
Martin KaFai Lau March 5, 2025, 1:36 a.m. UTC | #2 
On 3/4/25 8:36 AM, Amery Hung wrote:
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index ab0f2fed3c58..5b89f6ca5a0a 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -88,7 +88,11 @@ 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 bool in_crash_handler(void);
> +
> +static void stdio_restore(void)
>   {
>   #ifdef __GLIBC__
>   	if (verbose() && env.worker_id == -1) {
> @@ -98,34 +102,34 @@ static void stdio_restore_cleanup(void)
>   
>   	fflush(stdout);
>   
> -	if (env.subtest_state) {
> +	pthread_mutex_lock(&stdout_lock);
> +
> +	if (!env.subtest_state || in_crash_handler()) {

Can the stdio restore be done in the crash_handler() itself instead of having a 
special case here and adding another in_crash_handler()?

Theoretically, the crash_handler() only needs to
fflush(stdout /* whatever the current stdout is */) and...

> +		if (stdout == env.stdout_saved)
> +			goto out;
> +
> +		fclose(env.test_state->stdout_saved);
> +		env.test_state->stdout_saved = NULL;
> +		stdout = env.stdout_saved;
> +		stderr = env.stderr_saved;

... restore std{out,err} = env.std{out,err}_saved.

At the crash point, it does not make a big difference to 
fclose(evn.test_state->stdout_saved) or not?

If the crash_handler() does not close the stdout that the traffic monitor might 
potentially be using, then crash_handler() does not need to take mutex, right?

> +	} else {
>   		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;
>   	}
> +out:
> +	pthread_mutex_unlock(&stdout_lock);
>   #endif
>   }
>   

[ ... ]

> +static bool in_crash_handler(void)
> +{
> +	struct sigaction sigact;
> +
> +	/* sa_handler will be cleared if invoked since crash_handler is
> +	 * registered with SA_RESETHAND
> +	 */
> +	sigaction(SIGSEGV, NULL, &sigact);
> +
> +	return sigact.sa_handler != crash_handler;
> +}
> +
Amery Hung March 5, 2025, 4:52 p.m. UTC | #3 
On Tue, Mar 4, 2025 at 5:36 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 3/4/25 8:36 AM, Amery Hung wrote:
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index ab0f2fed3c58..5b89f6ca5a0a 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -88,7 +88,11 @@ 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 bool in_crash_handler(void);
> > +
> > +static void stdio_restore(void)
> >   {
> >   #ifdef __GLIBC__
> >       if (verbose() && env.worker_id == -1) {
> > @@ -98,34 +102,34 @@ static void stdio_restore_cleanup(void)
> >
> >       fflush(stdout);
> >
> > -     if (env.subtest_state) {
> > +     pthread_mutex_lock(&stdout_lock);
> > +
> > +     if (!env.subtest_state || in_crash_handler()) {
>
> Can the stdio restore be done in the crash_handler() itself instead of having a
> special case here and adding another in_crash_handler()?
>
> Theoretically, the crash_handler() only needs to
> fflush(stdout /* whatever the current stdout is */) and...
>
> > +             if (stdout == env.stdout_saved)
> > +                     goto out;
> > +
> > +             fclose(env.test_state->stdout_saved);
> > +             env.test_state->stdout_saved = NULL;
> > +             stdout = env.stdout_saved;
> > +             stderr = env.stderr_saved;
>
> ... restore std{out,err} = env.std{out,err}_saved.
>
> At the crash point, it does not make a big difference to
> fclose(evn.test_state->stdout_saved) or not?
>
> If the crash_handler() does not close the stdout that the traffic monitor might
> potentially be using, then crash_handler() does not need to take mutex, right?
>

You are right. I think it is simpler to not let stdio_restore() handle
the crash case, and just do the following in the crash handler.

fflush(stdout);
stdout = env.stdout_saved;
stderr = env.stderr_saved;


> > +     } else {
> >               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;
> >       }
> > +out:
> > +     pthread_mutex_unlock(&stdout_lock);
> >   #endif
> >   }
> >
>
> [ ... ]
>
> > +static bool in_crash_handler(void)
> > +{
> > +     struct sigaction sigact;
> > +
> > +     /* sa_handler will be cleared if invoked since crash_handler is
> > +      * registered with SA_RESETHAND
> > +      */
> > +     sigaction(SIGSEGV, NULL, &sigact);
> > +
> > +     return sigact.sa_handler != crash_handler;
> > +}
> > +
Amery Hung March 5, 2025, 4:52 p.m. UTC | #4 
On Tue, Mar 4, 2025 at 4:12 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2025-03-04 at 08:36 -0800, 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.
> > 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
> >
> > Fix it by first consolidating stdout assignment into stdio_restore().
> > stdout will be restored to env.stdout_saved when a test ends or running
> > in the crash handler and to test_state.stdout_saved otherwise.
> > Then, protect 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.
> > stdio_restore() is kept in the crash handler instead of making all print
> > functions in the crash handler use env.stdout_saved to make it less
> > error-prone.
> >
> > Signed-off-by: Amery Hung <ameryhung@gmail.com>
> > ---
>
> This patch fixes the error for me.
>
> Tested-by: Eduard Zingerman <eddyz87@gmail.com>
>
> >  tools/testing/selftests/bpf/test_progs.c | 59 ++++++++++++++++--------
> >  1 file changed, 39 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index ab0f2fed3c58..5b89f6ca5a0a 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -88,7 +88,11 @@ 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 bool in_crash_handler(void);
> > +
> > +static void stdio_restore(void)
> >  {
> >  #ifdef __GLIBC__
> >       if (verbose() && env.worker_id == -1) {
> > @@ -98,34 +102,34 @@ static void stdio_restore_cleanup(void)
> >
> >       fflush(stdout);
> >
> > -     if (env.subtest_state) {
> > +     pthread_mutex_lock(&stdout_lock);
> > +
> > +     if (!env.subtest_state || in_crash_handler()) {
> > +             if (stdout == env.stdout_saved)
> > +                     goto out;
> > +
> > +             fclose(env.test_state->stdout_saved);
> > +             env.test_state->stdout_saved = NULL;
> > +             stdout = env.stdout_saved;
> > +             stderr = env.stderr_saved;
> > +     } else {
> >               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;
> >       }
> > +out:
> > +     pthread_mutex_unlock(&stdout_lock);
> >  #endif
> >  }
>
> stdio_restore_cleanup() did not reset stderr/stdout when
> env.subtest_state was NULL, but this difference does not seem to
> matter, stdio_restore_cleanup() was called from:
> - test__start_subtest(), where stdio_hijack_init() would override
>   stderr/stdout anyways.
> - run_one_test(), where it is followed by call to stdio_restore().
>
> I think this change is Ok.
>
> [...]
>
> > @@ -1276,6 +1281,18 @@ void crash_handler(int signum)
> >       backtrace_symbols_fd(bt, sz, STDERR_FILENO);
> >  }
> >
> > +static bool in_crash_handler(void)
> > +{
> > +     struct sigaction sigact;
> > +
> > +     /* sa_handler will be cleared if invoked since crash_handler is
> > +      * registered with SA_RESETHAND
> > +      */
> > +     sigaction(SIGSEGV, NULL, &sigact);
> > +
> > +     return sigact.sa_handler != crash_handler;
> > +}
> > +
>
> The patch would be simpler w/o this function. I double checked
> functions called from crash_handler() and two 'fprintf(stderr, ...)'
> there are the only places where stderr/stdout is used instead of
> *_saved versions. It is already a prevalent pattern to do
> 'fprintf(env.stderr_saved, ...)' in this file.
> Or pass a flag as in v3?

While that is a common pattern, I think restoring stdio there can make
sure people don't accidentally use stdout in the crash handler and
find that it does not work occasionally.

I will remove the in_crash_handler special case handling per Martin's
suggestion.


>
> >  void hexdump(const char *prefix, const void *buf, size_t len)
> >  {
> >       for (int i = 0; i < len; i++) {
> > @@ -1957,6 +1974,8 @@ int main(int argc, char **argv)
>
> [...]
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index ab0f2fed3c58..5b89f6ca5a0a 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -88,7 +88,11 @@  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 bool in_crash_handler(void);
+
+static void stdio_restore(void)
 {
 #ifdef __GLIBC__
 	if (verbose() && env.worker_id == -1) {
@@ -98,34 +102,34 @@  static void stdio_restore_cleanup(void)
 
 	fflush(stdout);
 
-	if (env.subtest_state) {
+	pthread_mutex_lock(&stdout_lock);
+
+	if (!env.subtest_state || in_crash_handler()) {
+		if (stdout == env.stdout_saved)
+			goto out;
+
+		fclose(env.test_state->stdout_saved);
+		env.test_state->stdout_saved = NULL;
+		stdout = env.stdout_saved;
+		stderr = env.stderr_saved;
+	} else {
 		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;
 	}
+out:
+	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 +540,8 @@  void test__end_subtest(void)
 				   test_result(subtest_state->error_cnt,
 					       subtest_state->skipped));
 
-	stdio_restore_cleanup();
+	stdio_restore();
+
 	env.subtest_state = NULL;
 }
 
@@ -1276,6 +1281,18 @@  void crash_handler(int signum)
 	backtrace_symbols_fd(bt, sz, STDERR_FILENO);
 }
 
+static bool in_crash_handler(void)
+{
+	struct sigaction sigact;
+
+	/* sa_handler will be cleared if invoked since crash_handler is
+	 * registered with SA_RESETHAND
+	 */
+	sigaction(SIGSEGV, NULL, &sigact);
+
+	return sigact.sa_handler != crash_handler;
+}
+
 void hexdump(const char *prefix, const void *buf, size_t len)
 {
 	for (int i = 0; i < len; i++) {
@@ -1957,6 +1974,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();