From da2aeb0c42fee80c02227adb73c0f246bf5f7089 Mon Sep 17 00:00:00 2001 From: Mark Salyzyn Date: Wed, 2 Jan 2019 08:27:22 -0800 Subject: [PATCH] llkd: handle 'adbd shell setsid' to preserve adbd A zombie setsid process occurs when adb shell setsid is issued, however llkd can only detect if it is a result of a kernel livelock by killing the associated parent, which would be adbd; resulting in the adb connection(s) being terminated. Will special case this condition in order to preserve adbd for debugging purposes. We parse & in ro.llk.blacklist.parent as this association, thus adbd&[setsid] covers this special case. Ampersand was selected because it is never part of a process name, however a setprop in the shell requires it to be escaped or quoted; init rc file where this is normally specified does not have issue. getComm() is effectively pure, so hold on to the return value for sake of efficiency. This also reverts commit 599958d1148c1137bee925f4cf08eda7f8050d98 which granted adbd blanket parent immunity from monitoring on userdebug builds. The new logic is a more refined means of preserving the live lock checking associated with adbd and allows the operation to be performed on user builds. POC: date ; adb shell setsid sleep 900 ; date Positive for bug, reports less than 15 minutes, otherwise solved. Test: llkd_unit_test Bug: 120983740 Change-Id: I6442463a48499d925a3a074423a24a1622905559 --- llkd/README.md | 9 +++-- llkd/include/llkd.h | 6 +--- llkd/libllkd.cpp | 74 ++++++++++++++++++++++++++++++++++++---- llkd/tests/llkd_test.cpp | 35 +++++++++++++++++++ 4 files changed, 110 insertions(+), 14 deletions(-) diff --git a/llkd/README.md b/llkd/README.md index 43bb94ae0..191f98819 100644 --- a/llkd/README.md +++ b/llkd/README.md @@ -165,9 +165,14 @@ size of 92. NB: false is a very very very unlikely process to want to blacklist. #### ro.llk.blacklist.parent -default 0,2,adbd (kernel, [kthreadd] and adbd). +default 0,2,adbd&[setsid] (kernel, [kthreadd] and adbd *only for zombie setsid*). Do not watch processes that have this parent. -A parent process can be comm, cmdline or pid reference. +An ampersand (*&*) separator is used to specify that the parent is ignored +only in combination with the target child process. +Ampersand was selected because it is never part of a process name, +however a setprop in the shell requires it to be escaped or quoted; +init rc file where this is normally specified does not have this issue. +A parent or target processes can be specified as comm, cmdline or pid reference. #### ro.llk.blacklist.uid default *empty* or false, comma separated list of uid numbers or names. diff --git a/llkd/include/llkd.h b/llkd/include/llkd.h index 7b7dbf90f..3586ca1b1 100644 --- a/llkd/include/llkd.h +++ b/llkd/include/llkd.h @@ -56,11 +56,7 @@ unsigned llkCheckMilliseconds(void); #define LLK_BLACKLIST_PROCESS_DEFAULT \ "0,1,2,init,[kthreadd],[khungtaskd],lmkd,llkd,watchdogd,[watchdogd],[watchdogd/0]" #define LLK_BLACKLIST_PARENT_PROPERTY "ro.llk.blacklist.parent" -#ifdef __PTRACE_ENABLED__ // defined if userdebug build -#define LLK_BLACKLIST_PARENT_DEFAULT "0,2,[kthreadd],adbd" -#else -#define LLK_BLACKLIST_PARENT_DEFAULT "0,2,[kthreadd]" -#endif +#define LLK_BLACKLIST_PARENT_DEFAULT "0,2,[kthreadd],adbd&[setsid]" #define LLK_BLACKLIST_UID_PROPERTY "ro.llk.blacklist.uid" #define LLK_BLACKLIST_UID_DEFAULT "" #define LLK_BLACKLIST_STACK_PROPERTY "ro.llk.blacklist.process.stack" diff --git a/llkd/libllkd.cpp b/llkd/libllkd.cpp index 3a593ecc1..3c295b506 100644 --- a/llkd/libllkd.cpp +++ b/llkd/libllkd.cpp @@ -108,6 +108,9 @@ std::unordered_set llkBlacklistProcess; // list of parent pids, comm or cmdline names to skip. default: // kernel pid (0), [kthreadd] (2), or ourselves, enforced and implied std::unordered_set llkBlacklistParent; +// list of parent and target processes to skip. default: +// adbd *and* [setsid] +std::unordered_map> llkBlacklistParentAndChild; // list of uids, and uid names, to skip, default nothing std::unordered_set llkBlacklistUid; #ifdef __PTRACE_ENABLED__ @@ -624,6 +627,19 @@ std::string llkFormat(const std::unordered_set& blacklist) { return ret; } +std::string llkFormat( + const std::unordered_map>& blacklist, + bool leading_comma = false) { + std::string ret; + for (const auto& entry : blacklist) { + for (const auto& target : entry.second) { + if (leading_comma || !ret.empty()) ret += ","; + ret += entry.first + "&" + target; + } + } + return ret; +} + // This function parses the properties as a list, incorporating the supplied // default. A leading comma separator means preserve the defaults and add // entries (with an optional leading + sign), or removes entries with a leading @@ -691,6 +707,27 @@ bool llkSkipProc(proc* procp, return false; } +const std::unordered_set& llkSkipName( + const std::string& name, + const std::unordered_map>& blacklist) { + static const std::unordered_set empty; + if (name.empty() || blacklist.empty()) return empty; + auto found = blacklist.find(name); + if (found == blacklist.end()) return empty; + return found->second; +} + +bool llkSkipPproc(proc* pprocp, proc* procp, + const std::unordered_map>& + blacklist = llkBlacklistParentAndChild) { + if (!pprocp || !procp || blacklist.empty()) return false; + if (llkSkipProc(procp, llkSkipName(std::to_string(pprocp->pid), blacklist))) return true; + if (llkSkipProc(procp, llkSkipName(pprocp->getComm(), blacklist))) return true; + if (llkSkipProc(procp, llkSkipName(pprocp->getCmdline(), blacklist))) return true; + return llkSkipProc(procp, + llkSkipName(android::base::Basename(pprocp->getCmdline()), blacklist)); +} + bool llkSkipPid(pid_t pid) { return llkSkipName(std::to_string(pid), llkBlacklistProcess); } @@ -875,7 +912,8 @@ void llkLogConfig(void) { << LLK_BLACKLIST_STACK_PROPERTY "=" << llkFormat(llkBlacklistStack) << "\n" #endif << LLK_BLACKLIST_PROCESS_PROPERTY "=" << llkFormat(llkBlacklistProcess) << "\n" - << LLK_BLACKLIST_PARENT_PROPERTY "=" << llkFormat(llkBlacklistParent) << "\n" + << LLK_BLACKLIST_PARENT_PROPERTY "=" << llkFormat(llkBlacklistParent) + << llkFormat(llkBlacklistParentAndChild, true) << "\n" << LLK_BLACKLIST_UID_PROPERTY "=" << llkFormat(llkBlacklistUid); } @@ -1050,7 +1088,8 @@ milliseconds llkCheck(bool checkRunning) { break; } - if (llkSkipName(procp->getComm())) { + auto process_comm = procp->getComm(); + if (llkSkipName(process_comm)) { continue; } if (llkSkipName(procp->getCmdline())) { @@ -1065,6 +1104,7 @@ milliseconds llkCheck(bool checkRunning) { pprocp = llkTidAlloc(ppid, ppid, 0, "", 0, '?'); } if (pprocp) { + if (llkSkipPproc(pprocp, procp)) break; if (llkSkipProc(pprocp, llkBlacklistParent)) break; } else { if (llkSkipName(std::to_string(ppid), llkBlacklistParent)) break; @@ -1084,7 +1124,7 @@ milliseconds llkCheck(bool checkRunning) { stuck = true; } else if (procp->count != 0ms) { LOG(VERBOSE) << state << ' ' << llkFormat(procp->count) << ' ' << ppid << "->" - << pid << "->" << tid << ' ' << procp->getComm(); + << pid << "->" << tid << ' ' << process_comm; } } if (!stuck) continue; @@ -1092,7 +1132,7 @@ milliseconds llkCheck(bool checkRunning) { if (procp->count >= llkStateTimeoutMs[(state == 'Z') ? llkStateZ : llkStateD]) { if (procp->count != 0ms) { LOG(VERBOSE) << state << ' ' << llkFormat(procp->count) << ' ' << ppid << "->" - << pid << "->" << tid << ' ' << procp->getComm(); + << pid << "->" << tid << ' ' << process_comm; } continue; } @@ -1120,7 +1160,7 @@ milliseconds llkCheck(bool checkRunning) { break; } LOG(WARNING) << "Z " << llkFormat(procp->count) << ' ' << ppid << "->" - << pid << "->" << tid << ' ' << procp->getComm() << " [kill]"; + << pid << "->" << tid << ' ' << process_comm << " [kill]"; if ((llkKillOneProcess(pprocp, procp) >= 0) || (llkKillOneProcess(ppid, procp) >= 0)) { continue; @@ -1137,7 +1177,7 @@ milliseconds llkCheck(bool checkRunning) { // kernel (worse). default: LOG(WARNING) << state << ' ' << llkFormat(procp->count) << ' ' << pid - << "->" << tid << ' ' << procp->getComm() << " [kill]"; + << "->" << tid << ' ' << process_comm << " [kill]"; if ((llkKillOneProcess(llkTidLookup(pid), procp) >= 0) || (llkKillOneProcess(pid, state, tid) >= 0) || (llkKillOneProcess(procp, procp) >= 0) || @@ -1150,7 +1190,7 @@ milliseconds llkCheck(bool checkRunning) { // We are here because we have confirmed kernel live-lock const auto message = state + " "s + llkFormat(procp->count) + " " + std::to_string(ppid) + "->" + std::to_string(pid) + "->" + - std::to_string(tid) + " " + procp->getComm() + " [panic]"; + std::to_string(tid) + " " + process_comm + " [panic]"; llkPanicKernel(dump, tid, (state == 'Z') ? "zombie" : (state == 'D') ? "driver" : "sleeping", message); @@ -1274,6 +1314,26 @@ bool llkInit(const char* threadname) { llkBlacklistParent = llkSplit(LLK_BLACKLIST_PARENT_PROPERTY, std::to_string(kernelPid) + "," + std::to_string(kthreaddPid) + "," LLK_BLACKLIST_PARENT_DEFAULT); + // derive llkBlacklistParentAndChild by moving entries with '&' from above + for (auto it = llkBlacklistParent.begin(); it != llkBlacklistParent.end();) { + auto pos = it->find('&'); + if (pos == std::string::npos) { + ++it; + continue; + } + auto parent = it->substr(0, pos); + auto child = it->substr(pos + 1); + it = llkBlacklistParent.erase(it); + + auto found = llkBlacklistParentAndChild.find(parent); + if (found == llkBlacklistParentAndChild.end()) { + llkBlacklistParentAndChild.emplace(std::make_pair( + std::move(parent), std::unordered_set({std::move(child)}))); + } else { + found->second.emplace(std::move(child)); + } + } + llkBlacklistUid = llkSplit(LLK_BLACKLIST_UID_PROPERTY, LLK_BLACKLIST_UID_DEFAULT); // internal watchdog diff --git a/llkd/tests/llkd_test.cpp b/llkd/tests/llkd_test.cpp index d73893583..96079cc69 100644 --- a/llkd/tests/llkd_test.cpp +++ b/llkd/tests/llkd_test.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -333,3 +334,37 @@ TEST(llkd, sleep) { unlink(stack_pipe_file); } + +// b/120983740 +TEST(llkd, adbd_and_setsid) { + if (checkKill("kernel_panic,sysrq,livelock,zombie")) { + return; + } + const auto period = llkdSleepPeriod('S'); + + // expect llkd.zombie to trigger, but not for adbd&[setsid] + // Create a Persistent Zombie setsid Process + pid_t child_pid = fork(); + ASSERT_LE(0, child_pid); + if (!child_pid) { + prctl(PR_SET_NAME, "adbd"); + auto zombie_pid = fork(); + ASSERT_LE(0, zombie_pid); + if (!zombie_pid) { + prctl(PR_SET_NAME, "setsid"); + sleep(1); + exit(0); + } + sleep(period.count()); + exit(42); + } + + // Reverse of waitForPid, do _not_ expect kill + int wstatus; + ASSERT_LE(0, waitpid(child_pid, &wstatus, 0)); + EXPECT_TRUE(WIFEXITED(wstatus)); + if (WIFEXITED(wstatus)) { + EXPECT_EQ(42, WEXITSTATUS(wstatus)); + } + ASSERT_FALSE(WIFSIGNALED(wstatus)) << "[ INFO ] signo=" << WTERMSIG(wstatus); +}