Messages in this thread | | | Date | Thu, 1 Apr 2010 15:13:21 +0200 | From | Oleg Nesterov <> | Subject | [PATCH 0/1] oom: fix the unsafe usage of badness() in proc_oom_score() |
| |
On 04/01, David Rientjes wrote: > > On Wed, 31 Mar 2010, Oleg Nesterov wrote: > > > 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(). > > > > 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.
> > 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.
| |