mm, oom: fix use-after-free in oom_kill_process

On looking further it seems like the process selected to be oom-killed has exited even before reaching read_lock(&tasklist_lock) in oom_kill_process(). More specifically the tsk->usage is 1 which is due to get_task_struct() in oom_evaluate_task() and the put_task_struct within for_each_thread() frees the tsk and for_each_thread() tries to access the tsk. The easiest fix is to do get/put across the for_each_thread() on the selected task.

Now the next question is should we continue with the oom-kill as the previously selected task has exited? However before adding more complexity and heuristics, let's answer why we even look at the children of oom-kill selected task? The select_bad_process() has already selected the worst process in the system/memcg. Due to race, the selected process might not be the worst at the kill time but does that matter matter? The userspace can play with oom_score_adj to prefer children to be killed before the parent. I looked at the history but it seems like this is there before git history.

Hi Shakeel!

Quoted 8 lines

On looking further it seems like the process selected to be oom-killed has exited even before reaching read_lock(&tasklist_lock) in oom_kill_process(). More specifically the tsk->usage is 1 which is due to get_task_struct() in oom_evaluate_task() and the put_task_struct within for_each_thread() frees the tsk and for_each_thread() tries to access the tsk. The easiest fix is to do get/put across the for_each_thread() on the selected task.

Please, feel free to add

Reviewed-by Roman Gushchin <[email protected]>

for this part.

Quoted 10 lines

Now the next question is should we continue with the oom-kill as the previously selected task has exited? However before adding more complexity and heuristics, let's answer why we even look at the children of oom-kill selected task? The select_bad_process() has already selected the worst process in the system/memcg. Due to race, the selected process might not be the worst at the kill time but does that matter matter? The userspace can play with oom_score_adj to prefer children to be killed before the parent. I looked at the history but it seems like this is there before git history.

I'd totally support you in an attempt to remove this logic, unless someone has a good example of its usefulness.

I believe it's a very old hack to select children over parents in case they have the same oom badness (e.g. share most of the memory).

Maybe we can prefer older processes in case of equal oom badness, and it will be enough.

Thanks!

On looking further it seems like the process selected to be oom-killed has exited even before reaching read_lock(&tasklist_lock) in oom_kill_process(). More specifically the tsk->usage is 1 which is due to get_task_struct() in oom_evaluate_task() and the put_task_struct within for_each_thread() frees the tsk and for_each_thread() tries to access the tsk. The easiest fix is to do get/put across the for_each_thread() on the selected task.

mm/oom_kill.c +7 -0

@@ -981,6 +981,13 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	 * still freeing memory.
 	 */
 	read_lock(&tasklist_lock);
+
+	/*
+	 * The task 'p' might have already exited before reaching here. The
+	 * put_task_struct() will free task_struct 'p' while the loop still try
+	 * to access the field of 'p', so, get an extra reference.
+	 */
+	get_task_struct(p);
 	for_each_thread(p, t) {
 		list_for_each_entry(child, &t->children, sibling) {
 			unsigned int child_points;

Quoted 7 lines · Shakeel Butt

On looking further it seems like the process selected to be oom-killed has exited even before reaching read_lock(&tasklist_lock) in oom_kill_process(). More specifically the tsk->usage is 1 which is due to get_task_struct() in oom_evaluate_task() and the put_task_struct within for_each_thread() frees the tsk and for_each_thread() tries to access the tsk. The easiest fix is to do get/put across the for_each_thread() on the selected task.

Good catch. p->usage can become 1 while printk()ing a lot at dump_header().

Quoted 15 lines

mm/oom_kill.c +7 -0

@@ -981,6 +981,13 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
       * still freeing memory.
       */
      read_lock(&tasklist_lock);
+
+     /*
+      * The task 'p' might have already exited before reaching here. The
+      * put_task_struct() will free task_struct 'p' while the loop still try
+      * to access the field of 'p', so, get an extra reference.
+      */
+     get_task_struct(p);
      for_each_thread(p, t) {
              list_for_each_entry(child, &t->children, sibling) {
                      unsigned int child_points;

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 1010

Quoted 4 lines

mm/oom_kill.c +1 -0

@@ -1000,6 +1007,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
                      }
              }
      }
+     put_task_struct(p);

Moving put_task_struct(p) to after read_unlock(&tasklist_lock) will reduce latency of a write_lock(&tasklist_lock) waiter.

Quoted 2 lines

mm/oom_kill.c +0 -0

      read_unlock(&tasklist_lock);
 

Review static void oom_kill_process(struct oom_control *oc, const char *message) · line 1013

Quoted 2 lines

(quoted diff) +0 -0

      /*

By the way, p->usage is already 1 implies that p->mm == NULL due to already completed exit_mm(p). Then, process_shares_mm(child, p->mm) might fail to return true for some of children. Not critical but might lead to unnecessary oom_badness() calls for child selection. Maybe we want to use same logic __oom_kill_process() uses (i.e. bail out if find_task_lock_mm(p) failed)?

On looking further it seems like the process selected to be oom-killed has exited even before reaching read_lock(&tasklist_lock) in oom_kill_process(). More specifically the tsk->usage is 1 which is due to get_task_struct() in oom_evaluate_task() and the put_task_struct within for_each_thread() frees the tsk and for_each_thread() tries to access the tsk. The easiest fix is to do get/put across the for_each_thread() on the selected task.

Now the next question is should we continue with the oom-kill as the previously selected task has exited? However before adding more complexity and heuristics, let's answer why we even look at the children of oom-kill selected task?

The select_bad_process() has already
selected the worst process in the system/memcg. Due to race, the selected process might not be the worst at the kill time but does that matter matter?

The userspace can play with oom_score_adj to prefer
children to be killed before the parent. I looked at the history but it seems like this is there before git history.

Signed-off-by Shakeel Butt <[email protected]>

---
 mm/oom_kill.c | 8 ++++++++
 1 file changed, 8 insertions(+)

mm/oom_kill.c +8 -0

index 0930b4365be7..1a007dae1e8f 100644
@@ -981,6 +981,13 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	 * still freeing memory.
 	 */
 	read_lock(&tasklist_lock);
+
+	/*
+	 * The task 'p' might have already exited before reaching here. The
+	 * put_task_struct() will free task_struct 'p' while the loop still try
+	 * to access the field of 'p', so, get an extra reference.
+	 */
+	get_task_struct(p);
 	for_each_thread(p, t) {
 		list_for_each_entry(child, &t->children, sibling) {
 			unsigned int child_points;

@@ -1000,6 +1007,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 			}
 		}
 	}
+	put_task_struct(p);
 	read_unlock(&tasklist_lock);
 
 	/*

Signature · 1 line

2.20.1.321.g9e740568ce-goog

On Fri 18-01-19 16:50:22, Shakeel Butt wrote:
[...]

Quoted 7 lines

On looking further it seems like the process selected to be oom-killed has exited even before reaching read_lock(&tasklist_lock) in oom_kill_process(). More specifically the tsk->usage is 1 which is due to get_task_struct() in oom_evaluate_task() and the put_task_struct within for_each_thread() frees the tsk and for_each_thread() tries to access the tsk. The easiest fix is to do get/put across the for_each_thread() on the selected task.

Very well spotted! The code seems safe because we are careful to transfer the victim along with reference counting but I've totally missed that the loop itself needs a reference. It seems that this has been broken since the heuristic has been introduced. But I haven't checked it closely. I am still on vacation.

Quoted 4 lines

Now the next question is should we continue with the oom-kill as the previously selected task has exited? However before adding more complexity and heuristics, let's answer why we even look at the children of oom-kill selected task?

The objective was the work protection assuming that children did less work than their parrent. I find this argument a bit questionable because it highly depends a specific workload while it opens doors for problematic behavior at the same time. If you have a fork bomb like workload then it is basically hard to resolve the OOM condition as children have barely any memory so we keep looping killing tasks which will not free up much. So I am all for removing this heuristic.

Quoted 4 lines

The select_bad_process() has already
selected the worst process in the system/memcg. Due to race, the selected process might not be the worst at the kill time but does that matter matter?

No, we don't I believe. The aim of the oom killer is to kill something. We will never be ideal here because this is a land of races.

Quoted 4 lines

The userspace can play with oom_score_adj to prefer
children to be killed before the parent. I looked at the history but it seems like this is there before git history.

Quoted 1 line

Signed-off-by Shakeel Butt <[email protected]>

Fixes 5e9d834a0e0c ("oom: sacrifice child with highest badness score for parent")

Cc stable

Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 31 lines

---
 mm/oom_kill.c | 8 ++++++++
 1 file changed, 8 insertions(+)

mm/oom_kill.c +8 -1

index 0930b4365be7..1a007dae1e8f 100644
@@ -981,6 +981,13 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
       * still freeing memory.
       */
      read_lock(&tasklist_lock);
+
+     /*
+      * The task 'p' might have already exited before reaching here. The
+      * put_task_struct() will free task_struct 'p' while the loop still try
+      * to access the field of 'p', so, get an extra reference.
+      */
+     get_task_struct(p);
      for_each_thread(p, t) {
              list_for_each_entry(child, &t->children, sibling) {
                      unsigned int child_points;

@@ -1000,6 +1007,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
                      }
              }
      }
+     put_task_struct(p);
      read_unlock(&tasklist_lock);
 
      /*
--

Quoted 1 line

2.20.1.321.g9e740568ce-goog

--
Michal Hocko
SUSE Labs

Fixes 5e9d834a0e0c ("oom: sacrifice child with highest badness score for parent")

Cc stable