Ackerley Tng <[email protected]> writes:
> The TEST_EXPECT_SIGBUS macro is not thread-safe as it uses a global
> sigjmp_buf and installs a global SIGBUS signal handler. If multiple threads
> execute the macro concurrently, they will race on installing the signal
> handler and stomp on other threads' jump buffers, leading to incorrect test
> behavior.
>
> Make TEST_EXPECT_SIGBUS thread-safe with the following changes:
>
> Share the KVM tests' global signal handler. sigaction() applies to all
> threads; without sharing a global signal handler, one thread may have
> removed the signal handler that another thread added, hence leading to
> unexpected signals.
>
> The alternative of layering signal handlers was considered, but calling
> sigaction() within TEST_EXPECT_SIGBUS() necessarily creates a race. To
> avoid adding new setup and teardown routines to do sigaction() and keep
> usage of TEST_EXPECT_SIGBUS() simple, share the KVM tests' global signal
> handler.
>
> Opportunistically rename report_unexpected_signal to
> catchall_signal_handler.
>
> To continue to only expect SIGBUS within specific regions of code, use a
> thread-specific variable, expecting_sigbus, to replace installing and
> removing signal handlers.
>
> Make the execution environment for the thread, sigjmp_buf, a
> thread-specific variable.
>
> Signed-off-by: Ackerley Tng <[email protected]>
> ---
> .../testing/selftests/kvm/include/test_util.h | 29 +++++++++----------
> tools/testing/selftests/kvm/lib/kvm_util.c | 18 ++++++++----
> tools/testing/selftests/kvm/lib/test_util.c | 7 -----
> 3 files changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/test_util.h
> b/tools/testing/selftests/kvm/include/test_util.h
> index 2871a4292847..0e4e6f7dab8f 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -80,22 +80,19 @@ do {
> \
> __builtin_unreachable(); \
> } while (0)
>
> -extern sigjmp_buf expect_sigbus_jmpbuf;
> -void expect_sigbus_handler(int signum);
> -
> -#define TEST_EXPECT_SIGBUS(action)
> \
> -do {
> \
> - struct sigaction sa_old, sa_new = {
> \
> - .sa_handler = expect_sigbus_handler,
> \
> - };
> \
> -
> \
> - sigaction(SIGBUS, &sa_new, &sa_old);
> \
> - if (sigsetjmp(expect_sigbus_jmpbuf, 1) == 0) {
> \
> - action;
> \
> - TEST_FAIL("'%s' should have triggered SIGBUS", #action);
> \
> - }
> \
> - sigaction(SIGBUS, &sa_old, NULL);
> \
> -} while (0)
> +extern __thread sigjmp_buf expect_sigbus_jmpbuf;
> +extern __thread bool expecting_sigbus;
> +
> +#define TEST_EXPECT_SIGBUS(action) \
> + do { \
> + expecting_sigbus = true; \
> + if (sigsetjmp(expect_sigbus_jmpbuf, 1) == 0) { \
> + action; \
> + TEST_FAIL("'%s' should have triggered SIGBUS", \
> + #action); \
> + } \
> + expecting_sigbus = false; \
> + } while (0)
>
> size_t parse_size(const char *size);
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c
> b/tools/testing/selftests/kvm/lib/kvm_util.c
> index aec7b24418ab..18ced8bdde36 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2314,13 +2314,20 @@ __weak void kvm_selftest_arch_init(void)
> {
> }
>
> -static void report_unexpected_signal(int signum)
> +__thread sigjmp_buf expect_sigbus_jmpbuf;
> +__thread bool expecting_sigbus;
> +
> +static void catchall_signal_handler(int signum)
> {
> + switch (signum) {
> + case SIGBUS: {
> + if (expecting_sigbus)
Transferring/summarizing an internal comment from Sean upstream:
This assumes that tests are indeed using the catchall_signal_handler as
the global signal handler.
In the next revision, I will make TEST_EXPECT_SIGBUS() assert that the
default signal handler is installed, so that developers get a clear,
explicit failure if/when something goes wrong.
> + siglongjmp(expect_sigbus_jmpbuf, 1);
> +
> + TEST_FAIL("Unexpected SIGBUS (%d)\n", signum);
> + }
> #define KVM_CASE_SIGNUM(sig) \
> case sig: TEST_FAIL("Unexpected " #sig " (%d)\n", signum)
> -
> - switch (signum) {
> - KVM_CASE_SIGNUM(SIGBUS);
> KVM_CASE_SIGNUM(SIGSEGV);
> KVM_CASE_SIGNUM(SIGILL);
> KVM_CASE_SIGNUM(SIGFPE);
> @@ -2332,12 +2339,13 @@ static void report_unexpected_signal(int signum)
> void __attribute((constructor)) kvm_selftest_init(void)
> {
> struct sigaction sig_sa = {
> - .sa_handler = report_unexpected_signal,
> + .sa_handler = catchall_signal_handler,
> };
>
> /* Tell stdout not to buffer its content. */
> setbuf(stdout, NULL);
>
> + expecting_sigbus = false;
> sigaction(SIGBUS, &sig_sa, NULL);
> sigaction(SIGSEGV, &sig_sa, NULL);
> sigaction(SIGILL, &sig_sa, NULL);
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c
> b/tools/testing/selftests/kvm/lib/test_util.c
> index 8a1848586a85..03eb99af9b8d 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -18,13 +18,6 @@
>
> #include "test_util.h"
>
> -sigjmp_buf expect_sigbus_jmpbuf;
> -
> -void __attribute__((used)) expect_sigbus_handler(int signum)
> -{
> - siglongjmp(expect_sigbus_jmpbuf, 1);
> -}
> -
> /*
> * Random number generator that is usable from guest code. This is the
> * Park-Miller LCG using standard constants.
> --
> 2.53.0.rc1.225.gd81095ad13-goog
