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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel]     [Linux USB Development]     [Yosemite News]     [Linux SCSI]

  Powered by Linux