From ab4c6d86e04054ff75b383c3b04aa125e00d4b35 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Wed, 6 Jul 2022 12:03:35 -0700 Subject: [PATCH 1/4] lmkd: set normal scheduling policy for reaper threads Reaper threads can take considerable time to free target's memory running at RT priority that they inherit from LMKD. This might prevent other critical tasks from being scheduled while reaper threads are active. Explicitly set reaper threads to run under normal scheduling policy but set their nice value to ANDROID_PRIORITY_HIGHEST. Bug: 237180716 Signed-off-by: Suren Baghdasaryan Change-Id: Idad817e698ae1d5ac6cee5aa3281c69c7e0d257f --- reaper.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/reaper.cpp b/reaper.cpp index 2c9e737..8603793 100644 --- a/reaper.cpp +++ b/reaper.cpp @@ -64,6 +64,10 @@ static void* reaper_main(void* param) { ALOGE("Failed to assign cpuset to the reaper thread"); } + if (setpriority(PRIO_PROCESS, tid, ANDROID_PRIORITY_HIGHEST)) { + ALOGW("Unable to raise priority of the reaper thread (%d): errno=%d", tid, errno); + } + for (;;) { target = reaper->dequeue_request(); @@ -112,6 +116,9 @@ bool Reaper::is_reaping_supported() { bool Reaper::init(int comm_fd) { char name[16]; + struct sched_param param = { + .sched_priority = 0, + }; if (thread_cnt_ > 0) { // init should not be called multiple times @@ -124,6 +131,10 @@ bool Reaper::init(int comm_fd) { ALOGE("pthread_create failed: %s", strerror(errno)); continue; } + // set normal scheduling policy for the reaper thread + if (pthread_setschedparam(thread_pool_[thread_cnt_], SCHED_OTHER, ¶m)) { + ALOGW("set SCHED_FIFO failed %s", strerror(errno)); + } snprintf(name, sizeof(name), "lmkd_reaper%d", thread_cnt_); if (pthread_setname_np(thread_pool_[thread_cnt_], name)) { ALOGW("pthread_setname_np failed: %s", strerror(errno)); From a0b25851c7094dbc83a490088ce80cef3746a3bf Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 7 Jul 2022 08:39:49 -0700 Subject: [PATCH 2/4] lmkd: Fix the text of the warning when pthread_setschedparam fails Fix the text of the warning when pthread_setschedparam fails to set SCHED_OTHER scheduling policy for a reaper thread. Bug: 237180716 Signed-off-by: Suren Baghdasaryan Change-Id: I1b04713d145f4f3c243f16f932c753f5b06d48e6 --- reaper.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reaper.cpp b/reaper.cpp index 8603793..6e4ee6d 100644 --- a/reaper.cpp +++ b/reaper.cpp @@ -133,7 +133,7 @@ bool Reaper::init(int comm_fd) { } // set normal scheduling policy for the reaper thread if (pthread_setschedparam(thread_pool_[thread_cnt_], SCHED_OTHER, ¶m)) { - ALOGW("set SCHED_FIFO failed %s", strerror(errno)); + ALOGW("set SCHED_OTHER failed %s", strerror(errno)); } snprintf(name, sizeof(name), "lmkd_reaper%d", thread_cnt_); if (pthread_setname_np(thread_pool_[thread_cnt_], name)) { From 26c9de4e727473b6037347cf794f5a7236f39092 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Tue, 23 Aug 2022 13:25:56 -0700 Subject: [PATCH 3/4] lmkd: Remove process record after it is killed by lmkd watchdog After lmkd watchdog kills a process, its record should be removed. Bug: 243567425 Signed-off-by: Suren Baghdasaryan Change-Id: I70bb2a432df8088ebc9865fbc36b065738248d19 --- lmkd.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lmkd.cpp b/lmkd.cpp index 741e891..5821e8f 100644 --- a/lmkd.cpp +++ b/lmkd.cpp @@ -2153,6 +2153,8 @@ static void watchdog_callback() { if (reaper.kill({ target.pidfd, target.pid, target.uid }, true) == 0) { ALOGW("lmkd watchdog killed process %d, oom_score_adj %d", target.pid, oom_score); killinfo_log(&target, 0, 0, 0, NULL, NULL, NULL, NULL, NULL); + // WARNING: do not use target after pid_remove() + pid_remove(target.pid); break; } prev_pid = target.pid; From a3802f16cf68d37ca89b4d0b8cf4776b64082995 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Tue, 27 Sep 2022 14:30:34 -0700 Subject: [PATCH 4/4] lmkd: Fix UAF caused by calling pid_remove() from the watchdog thread pid_remove() is not a thread-safe function and can be called only from the main thread. Calling it from the watchdog_callback() executed in the context of the watchdog thread can cause a use-after-free failure if the same record is being used by the main thread. Fix this issue by marking the process record as invalid instead of destroying it. Later on invalid records will be cleared in kill_one_process() called from the context of the main thread. Fixes: f8727745f917 ("lmkd: Remove process record after it is killed by lmkd watchdog") Bug: 248448498 Signed-off-by: Suren Baghdasaryan Change-Id: I0c7776aea1518c17f0a29904a44b7fe8f33980ca --- lmkd.cpp | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/lmkd.cpp b/lmkd.cpp index 5821e8f..6405187 100644 --- a/lmkd.cpp +++ b/lmkd.cpp @@ -511,6 +511,7 @@ struct proc { uid_t uid; int oomadj; pid_t reg_pid; /* PID of the process that registered this record */ + bool valid; struct proc *pidhash_next; }; @@ -942,6 +943,7 @@ static void proc_insert(struct proc *procp) { proc_slot(procp); } +// Can be called only from the main thread. static int pid_remove(int pid) { int hval = pid_hashfn(pid); struct proc *procp; @@ -971,6 +973,15 @@ static int pid_remove(int pid) { return 0; } +static void pid_invalidate(int pid) { + std::shared_lock lock(adjslot_list_lock); + struct proc *procp = pid_lookup(pid); + + if (procp) { + procp->valid = false; + } +} + /* * Write a string to a file. * Returns false if the file does not exist. @@ -1221,6 +1232,7 @@ static void cmd_procprio(LMKD_CTRL_PACKET packet, int field_count, struct ucred procp->uid = params.uid; procp->reg_pid = cred->pid; procp->oomadj = params.oomadj; + procp->valid = true; proc_insert(procp); } else { if (!claim_record(procp, cred->pid)) { @@ -2088,7 +2100,7 @@ static struct proc *proc_adj_prev(int oomadj, int pid) { return NULL; } -// When called from a non-main thread, adjslot_list_lock read lock should be taken. +// Can be called only from the main thread. static struct proc *proc_get_heaviest(int oomadj) { struct adjslot_list *head = &procadjslot_list[ADJTOSLOT(oomadj)]; struct adjslot_list *curr = head->next; @@ -2150,11 +2162,11 @@ static void watchdog_callback() { continue; } - if (reaper.kill({ target.pidfd, target.pid, target.uid }, true) == 0) { + if (target.valid && reaper.kill({ target.pidfd, target.pid, target.uid }, true) == 0) { ALOGW("lmkd watchdog killed process %d, oom_score_adj %d", target.pid, oom_score); killinfo_log(&target, 0, 0, 0, NULL, NULL, NULL, NULL, NULL); - // WARNING: do not use target after pid_remove() - pid_remove(target.pid); + // Can't call pid_remove() from non-main thread, therefore just invalidate the record + pid_invalidate(target.pid); break; } prev_pid = target.pid; @@ -2292,7 +2304,7 @@ static int kill_one_process(struct proc* procp, int min_oom_score, struct kill_i char buf[PAGE_SIZE]; char desc[LINE_MAX]; - if (!read_proc_status(pid, buf, sizeof(buf))) { + if (!procp->valid || !read_proc_status(pid, buf, sizeof(buf))) { goto out; } if (!parse_status_tag(buf, PROC_STATUS_TGID_FIELD, &tgid)) {