[PATCH 0/1] oom: fix the unsafe usage of badness() in proc_oom_score()

[Oleg Nesterov <oleg@redhat.com>]

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.