LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Kirill Tkhai <ktkhai@parallels.com>,
	linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
	Vladimir Davydov <vdavydov@parallels.com>,
	Kirill Tkhai <tkhai@yandex.ru>, Christoph Lameter <cl@linux.com>
Subject: Re: [PATCH 3/3] introduce task_rcu_dereference()
Date: Wed, 18 May 2016 19:02:18 +0200
Message-ID: <20160518170218.GY3192@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <20141027195446.GD11736@redhat.com>


So I keep coming back to this, and every time I look at it my brain
explodes.

On Mon, Oct 27, 2014 at 08:54:46PM +0100, Oleg Nesterov wrote:
> +struct task_struct *task_rcu_dereference(struct task_struct **ptask)
> +{
> +	struct task_struct *task;
> +	struct sighand_struct *sighand;

I think that needs: ' = NULL', because if

> +
> +	/*
> +	 * We need to verify that release_task() was not called and thus
> +	 * delayed_put_task_struct() can't run and drop the last reference
> +	 * before rcu_read_unlock(). We check task->sighand != NULL, but
> +	 * we can read the already freed and reused memory.
> +	 */
> + retry:
> +	task = rcu_dereference(*ptask);
> +	if (!task)
> +		return NULL;
> +
> +	probe_slab_address(&task->sighand, sighand);

this fails because of DEBUG_PAGE_ALLOC, we might not write to sighand at
all, and

> +	/*
> +	 * Pairs with atomic_dec_and_test(usage) in put_task_struct(task).
> +	 * If this task was already freed we can not miss the preceding
> +	 * update of this pointer.
> +	 */
> +	smp_rmb();
> +	if (unlikely(task != ACCESS_ONCE(*ptask)))
> +		goto retry;
> +
> +	/*
> +	 * Either this is the same task and we can trust sighand != NULL, or
> +	 * its memory was re-instantiated as another instance of task_struct.
> +	 * In the latter case the new task can not go away until another rcu
> +	 * gp pass, so the only problem is that sighand == NULL can be false
> +	 * positive but we can pretend we got this NULL before it was freed.
> +	 */
> +	if (sighand)

this will be inspecting random values on the stack.

> +		return task;
> +
> +	/*
> +	 * We could even eliminate the false positive mentioned above:
> +	 *
> +	 *	probe_slab_address(&task->sighand, sighand);
> +	 *	if (sighand)
> +	 *		goto retry;
> +	 *
> +	 * if sighand != NULL because we read the freed memory we should
> +	 * see the new pointer, otherwise we will likely return this task.
> +	 */
> +	return NULL;
> +}

This thing does more than rcu_dereference; because it also guarantees
that task->usage > 0 for the entire RCU section you do this under.
Because delayed_put_task_struct() will be delayed until at least the
matching rcu_read_unlock().


Should we instead create a primitive like try_get_task_struct() which
returns true if it got a reference? Because that is typically what
people end up doing..

A little like so?

---
 include/linux/sched.h |  2 ++
 kernel/exit.c         | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/sched/fair.c   | 29 +++++++----------------------
 3 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 38526b67e787..b2baa4af04e1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2132,6 +2132,8 @@ static inline void put_task_struct(struct task_struct *t)
 		__put_task_struct(t);
 }
 
+struct task_struct *try_get_task_struct(struct task_struct **ptask);
+
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
 extern void task_cputime(struct task_struct *t,
 			 cputime_t *utime, cputime_t *stime);
diff --git a/kernel/exit.c b/kernel/exit.c
index fd90195667e1..a1670dfd587b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -210,6 +210,52 @@ void release_task(struct task_struct *p)
 		goto repeat;
 }
 
+struct task_struct *try_get_task_struct(struct task_struct **ptask)
+{
+	struct sighand_struct *sighand = NULL;
+	struct task_struct *task = NULL;
+
+	rcu_read_lock();
+	/*
+	 * We need to verify that release_task() was not called and thus
+	 * delayed_put_task_struct() can't run and drop the last reference
+	 * before rcu_read_unlock(). We check task->sighand != NULL,
+	 * but we can read the already freed and reused memory.
+	 */
+retry:
+	task = rcu_dereference(*ptask);
+	if (!task)
+		goto unlock;
+
+	probe_kernel_address(&task->sighand, sighand);
+
+	/*
+	 * Pairs with atomic_dec_and_test() in put_task_struct(). If this task
+	 * was already freed we can not miss the preceding update of this
+	 * pointer.
+	 */
+	if (unlikely(task != READ_ONCE(*ptask)))
+		goto retry;
+
+	/*
+	 * Either this is the same task and we can trust sighand != NULL, or
+	 * its memory was re-instantiated as another instrance of task_struct.
+	 * In the latter case the new task can not go away until another RCU
+	 * GP pass, so the only problem is that sighand == NULL can be a false
+	 * positive, but we can pretend we got this NULL before it was freed.
+	 */
+	if (!sighand) {
+		task = NULL;
+		goto unlock;
+	}
+
+	get_task_struct(task);
+
+unlock:
+	rcu_read_unlock();
+	return task;
+}
+
 /*
  * Determine if a process group is "orphaned", according to the POSIX
  * definition in 2.2.2.52.  Orphaned process groups are not to be affected
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e83db73..1d3a410c481b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1374,30 +1374,15 @@ static void task_numa_compare(struct task_numa_env *env,
 	int dist = env->dist;
 	bool assigned = false;
 
-	rcu_read_lock();
-
-	raw_spin_lock_irq(&dst_rq->lock);
-	cur = dst_rq->curr;
-	/*
-	 * No need to move the exiting task or idle task.
-	 */
-	if ((cur->flags & PF_EXITING) || is_idle_task(cur))
-		cur = NULL;
-	else {
-		/*
-		 * The task_struct must be protected here to protect the
-		 * p->numa_faults access in the task_weight since the
-		 * numa_faults could already be freed in the following path:
-		 * finish_task_switch()
-		 *     --> put_task_struct()
-		 *         --> __put_task_struct()
-		 *             --> task_numa_free()
-		 */
-		get_task_struct(cur);
+	cur = try_get_task_struct(&dst_rq->curr);
+	if (cur) {
+		if ((cur->flags & PF_EXITING) || is_idle_task(cur)) {
+			put_task_struct(cur);
+			cur = NULL;
+		}
 	}
 
-	raw_spin_unlock_irq(&dst_rq->lock);
-
+	rcu_read_lock();
 	/*
 	 * Because we have preemption enabled we can get migrated around and
 	 * end try selecting ourselves (current == env->p) as a swap candidate.

  parent reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-22  7:17 [PATCH v4] sched/numa: fix unsafe get_task_struct() in task_numa_assign() Kirill Tkhai
2014-10-22 21:30 ` introduce task_rcu_dereference? Oleg Nesterov
2014-10-22 22:23   ` Oleg Nesterov
2014-10-23 18:15     ` Oleg Nesterov
2014-10-23  8:10   ` Kirill Tkhai
2014-10-23 18:18     ` Oleg Nesterov
2014-10-24  7:51       ` Kirill Tkhai
2014-10-27 19:53 ` [PATCH 0/3] introduce task_rcu_dereference() Oleg Nesterov
2014-10-27 19:54   ` [PATCH 1/3] probe_kernel_address() can use __probe_kernel_read() Oleg Nesterov
2014-10-27 19:54   ` [PATCH 2/3] introduce probe_slab_address() Oleg Nesterov
2014-10-27 19:21     ` Christoph Lameter
2014-10-28  5:44     ` Kirill Tkhai
2014-10-28  5:48       ` Kirill Tkhai
2014-10-28 15:01       ` Peter Zijlstra
2014-10-28 17:56         ` Kirill Tkhai
2014-10-28 18:00           ` Kirill Tkhai
2014-10-28 19:55           ` Oleg Nesterov
2014-10-28 20:12             ` Oleg Nesterov
2014-10-29  5:10               ` Kirill Tkhai
2014-10-27 19:54   ` [PATCH 3/3] introduce task_rcu_dereference() Oleg Nesterov
2014-10-28  6:22     ` Kirill Tkhai
2016-05-18 17:02     ` Peter Zijlstra [this message]
2016-05-18 18:23       ` Oleg Nesterov
2016-05-18 19:10         ` Peter Zijlstra
2016-05-18 19:57           ` Oleg Nesterov
2016-05-26 11:34             ` Peter Zijlstra
2016-06-03 10:49             ` [tip:sched/core] sched/fair: Use task_rcu_dereference() tip-bot for Oleg Nesterov
2016-06-03 10:48       ` [tip:sched/core] sched/api: Introduce task_rcu_dereference() and try_get_task_struct() tip-bot for Oleg Nesterov
2014-10-28 11:02 ` [tip:sched/core] sched/numa: Fix unsafe get_task_struct() in task_numa_assign() tip-bot for Kirill Tkhai
2014-11-08  3:48 ` [PATCH v4] sched/numa: fix " Sasha Levin
2014-11-09 14:07   ` Kirill Tkhai
2014-11-10 10:03     ` Peter Zijlstra
2014-11-10 15:48       ` Sasha Levin
2014-11-10 16:01         ` Peter Zijlstra
2014-11-16  9:50       ` [tip:sched/urgent] sched/numa: Avoid selecting oneself as swap target tip-bot for Peter Zijlstra
2014-11-10 16:03   ` [PATCH v4] sched/numa: fix unsafe get_task_struct() in task_numa_assign() Peter Zijlstra
2014-11-10 16:09     ` Sasha Levin
2014-11-10 16:16       ` Peter Zijlstra
2014-11-10 16:10     ` Kirill Tkhai
2014-11-10 16:36       ` Kirill Tkhai
2014-11-10 16:44         ` Sasha Levin
2014-11-10 20:01           ` Kirill Tkhai
2014-11-12  9:49             ` Kirill Tkhai
2014-11-15  2:38     ` Sasha Levin
2014-11-18 17:30       ` Sasha Levin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160518170218.GY3192@twins.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=cl@linux.com \
    --cc=ktkhai@parallels.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=oleg@redhat.com \
    --cc=tkhai@yandex.ru \
    --cc=vdavydov@parallels.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git