Re: [PATCH 2/2] mm/linux-next: Fix rcu locking in vm_is_stack

[Oleg Nesterov <oleg@redhat.com>]

On 03/07, Siddhesh Poyarekar wrote:
>
> Take rcu read lock before we do anything at all with the threadgroup
> list. Also use list_first_entry_rcu to safely get the reference to the
> first task in the list.

Sorry, but both patches are absolutely wrong, afaics. This change fixes
nothing, just obfuscates the code.

Probably my explanation was not clear, let me try again.

rcu_read_lock() can not help without the additional checks. By the
time you take it, task->thread_group->next can point to nowhere.

So,

> @@ -3932,18 +3932,20 @@ pid_t vm_is_stack(struct task_struct *task,
>  		return task->pid;
>
>  	if (in_group) {
> -		struct task_struct *t = task;
> +		struct task_struct *t;
>  		rcu_read_lock();
> -		while_each_thread(task, t) {
> +		t = list_first_entry_rcu(&task->thread_group,
> +					 struct task_struct, thread_group);

It is not safe to dereference this pointer.

Once again. You have the task_struct *task. It exits,
but task->thread_group->next still points to another thread T. Now suppose
that T exits too. But task->thread_group->next was not changed, it still
points to T. RCU grace period passes, T is freed.

After that you take rcu_read_lock(), but it is too late! >next points to
the already freed/reused memory. How can list_first_entry_rcu() help?

What you need is something like

		rcu_read_lock();
		if (!pid_alive(task))
			goto done;

		t = task;
		do {
			...
		} while_each(task, t);

	done:
		rcu_read_unlock();


And. Imho it is not good to have the (afaics exactly?) same code in
mm/nommu.c, even with the same names. Why it is not possible to make
a single definition?

Oleg.