From d705c2dbcde2561b4aead40d6b977772a760e92d Mon Sep 17 00:00:00 2001 From: Florian Mayer Date: Mon, 12 Sep 2022 18:19:47 -0700 Subject: [PATCH] [MTE] only upgrade to SYNC mode for MTE crashes Bug: 244471804 Test: atest mte_ugprade_test on emulator Change-Id: Ie974cf2dec96267012f1b01b9a40dad86551b1be --- debuggerd/handler/debuggerd_handler.cpp | 12 ++++++++++++ init/Android.bp | 2 ++ init/service.cpp | 17 +++++++++++++---- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index 92e76757d..fa556bbbf 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -623,6 +623,18 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c async_safe_format_log(ANDROID_LOG_ERROR, "libc", "MTE ERROR DETECTED BUT RUNNING IN PERMISSIVE MODE. CONTINUING."); pthread_mutex_unlock(&crash_mutex); + } else if (info->si_signo == SIGSEGV && info->si_code == SEGV_MTEAERR && getppid() == 1) { + // Back channel to init (see system/core/init/service.cpp) to signal that + // this process crashed due to an ASYNC MTE fault and should be considered + // for upgrade to SYNC mode. We are re-using the ART profiler signal, which + // is always handled (ignored in native processes, handled for generating a + // dump in ART processes), so a process will never crash from this signal + // except from here. + // The kernel is not particularly receptive to adding this information: + // https://lore.kernel.org/all/20220909180617.374238-1-fmayer@google.com/, so we work around + // like this. + info->si_signo = BIONIC_SIGNAL_ART_PROFILER; + resend_signal(info); } #endif else { diff --git a/init/Android.bp b/init/Android.bp index dfc90da77..20d622d6b 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -200,6 +200,7 @@ libinit_cc_defaults { "libutils", "libziparchive", ], + header_libs: ["bionic_libc_platform_headers"], bootstrap: true, visibility: [":__subpackages__"], } @@ -529,6 +530,7 @@ cc_library_static { "libcap", ], export_include_dirs: ["test_utils/include"], // for tests + header_libs: ["bionic_libc_platform_headers"], } // Host Verifier diff --git a/init/service.cpp b/init/service.cpp index 4cf409c2d..833473295 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -42,6 +42,10 @@ #include "service_list.h" #include "util.h" +#if defined(__BIONIC__) +#include +#endif + #ifdef INIT_FULL_SOURCES #include #include @@ -323,12 +327,17 @@ void Service::Reap(const siginfo_t& siginfo) { mount_namespace_.has_value() && *mount_namespace_ == NS_DEFAULT; const bool is_process_updatable = use_default_mount_ns && is_apex_updatable; -#ifdef SEGV_MTEAERR +#if defined(__BIONIC__) && defined(SEGV_MTEAERR) // As a precaution, we only upgrade a service once per reboot, to limit // the potential impact. - // TODO(b/244471804): Once we have a kernel API to get sicode, compare it to MTEAERR here. - bool should_upgrade_mte = siginfo.si_code != CLD_EXITED && siginfo.si_status == SIGSEGV && - !upgraded_mte_; + // + // BIONIC_SIGNAL_ART_PROFILER is a magic value used by deuggerd to signal + // that the process crashed with SIGSEGV and SEGV_MTEAERR. This signal will + // never be seen otherwise in a crash, because it always gets handled by the + // profiling signal handlers in bionic. See also + // debuggerd/handler/debuggerd_handler.cpp. + bool should_upgrade_mte = siginfo.si_code != CLD_EXITED && + siginfo.si_status == BIONIC_SIGNAL_ART_PROFILER && !upgraded_mte_; if (should_upgrade_mte) { LOG(INFO) << "Upgrading service " << name_ << " to sync MTE";