Skip to content

oom: fix the unsafe usage of badness() in proc_oom_score()

Oleg Nesterov linux-mm lkml 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 Latest1 patch 1 replyIn review

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/1oom: fix the unsafe usage of badness() in proc_oom_score()fs/proc/base.c+3 -21 reply

Reviews trailers per patch

The patch has a Reviewed-by or Acked-by.

PatchDavid Rientjes
1/1A-b

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v1 thread

Cover letter [PATCH 0/1] oom: fix the unsafe usage of badness() in proc_oom_score()
Quoted 7 lines · David Rientjes
Quoted 4 lines · Oleg Nesterov

But. Oh well. David, oom-badness-heuristic-rewrite.patch changed badness() to consult p->signal->oom_score_adj. Until recently this was wrong when it is called from proc_oom_score().

Quoted 7 lines
Quoted 4 lines

This means oom-badness-heuristic-rewrite.patch depends on signals-make-task_struct-signal-immutable-refcountable.patch, or we need the pid_alive() check again.

oom-badness-heuristic-rewrite.patch didn't change anything, Linus' tree currently dereferences p->signal->oom_adj

Yes, I wrongly blaimed oom-badness-heuristic-rewrite.patch, vanilla does the same.

Now this is really bad, and I am resending my patch.

David, Andrew, I understand it (textually) conflicts with oom-badness-heuristic-rewrite.patch, but this bug should be fixed imho before other changes. I hope it will be easy to fixup this chunk

	@@ -447,7 +447,13 @@ static int proc_oom_score(struct task_st

		do_posix_clock_monotonic_gettime(&uptime);
		read_lock(&tasklist_lock);
	-       points = badness(task->group_leader, uptime.tv_sec);
	+       points = oom_badness(task->group_leader,

in that patch.

Quoted 11 lines

Review

Quoted 8 lines
(quoted diff) +2 -1
 	do_posix_clock_monotonic_gettime(&uptime);
 	read_lock(&tasklist_lock);
-	points = oom_badness(task->group_leader,
+	if (pid_alive(task))
+		points = oom_badness(task,
 				global_page_state(NR_INACTIVE_ANON) +
 				global_page_state(NR_ACTIVE_ANON) +
 				global_page_state(NR_INACTIVE_FILE) +

This should be protected by the get_proc_task() on the inode before this function is called from proc_info_read().

No, get_proc_task() shouldn't (and can't) do this. To clarify, get_proc_task() does check the task wasn't unhashed, but nothing can prevent from release_task() after that. Once again, only task_struct itself is protected by get_task_struct(), nothing more.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Read the cover letter in its thread