linux-kernel - Re: Pinning down a blocked task to extract diagnostics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 5 Mar 2020 17:40:27 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Peter Zijlstra <peterz@...radead.org>, mingo@...hat.com,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, bsegall@...gle.com, mgorman@...e.de,
        linux-kernel@...r.kernel.org
Subject: Re: Pinning down a blocked task to extract diagnostics

On Thu, Mar 05, 2020 at 07:36:38AM -0800, Paul E. McKenney wrote:
> On Thu, Mar 05, 2020 at 09:28:45AM -0500, Steven Rostedt wrote:
> > On Thu, 5 Mar 2020 06:22:45 -0800
> > "Paul E. McKenney" <paulmck@...nel.org> wrote:
> > 
> > > On Thu, Mar 05, 2020 at 09:13:37AM +0100, Peter Zijlstra wrote:

[ . . . ]

> > > How about if I add something like this, located right by try_to_wake_up()?
> > > 
> > > 	bool try_to_keep_sleeping(struct task_struct *t)
> > > 	{
> > > 		raw_spin_lock_irq(&t->pi_lock);
> > > 		switch (t->state) {
> > > 		case TASK_RUNNING:
> > > 		case TASK_WAKING:
> > > 			raw_spin_unlock_irq(&t->pi_lock);
> > > 			return false;
> > > 
> > > 		default:
> > > 			if (t->on_rq) {
> > 
> > Somehow I think there still needs to be a read barrier before the test to
> > on_rq.
> 
> This is nowhere near a fastpath, so if there is uncertainty it gets
> the smp_rmb().  Or an smp_load_acquire() on t->state.
> 
> > > 				raw_spin_unlock_irq(&t->pi_lock);
> > > 				return false;
> > > 			}
> > > 
> > > 			/* OK to extract consistent diagnostic information. */
> > > 			return true;
> > > 		}
> > > 		/* NOTREACHED */
> > > 	}
> > > 
> > > Then a use might look like this:
> > > 
> > > 	if (try_to_keep_sleeping(t))
> > > 		/* Extract consistent diagnostic information. */
> > > 		raw_spin_unlock_irq(&t->pi_lock);
> > 
> > Perhaps we should have a allow_awake(t) to match it?
> > 
> > 		allow_awake(t);
> > 
> > Where we have:
> > 
> > static inline allow_awake(struct task_struct *t)
> > {
> > 	raw_spin_unlock_irq(&t->pi_lock);
> > }
> 
> Makes sense to me!

So how about like this?

							Thanx, Paul

------------------------------------------------------------------------

commit e2821ae6c6a6adaabc89ccd9babf4375a78e0626
Author: Paul E. McKenney <paulmck@...nel.org>
Date:   Thu Mar 5 16:53:58 2020 -0800

    sched/core: Add functions to prevent sleepers from awakening
    
    In some cases, it is necessary to examine a consistent version of a
    sleeping process's state, in other words, it is necessary to keep
    that process in sleeping state.  This commit therefore provides a
    try_to_keep_sleeping() function that acquires ->pi_lock to prevent
    wakeups from proceeding, returning true if the function is still asleep,
    and otherwise releasing ->pi_lock and returning false.
    
    This commit also provides an allow_awake() function (as suggested by
    by Steven Rostedt) that reverses the effect of a successful call to
    try_to_keep_sleeping(), allowing the process to once again be awakened.
    
    Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
    [ paulmck: Apply feedback from Peter Zijlstra and Steven Rostedt. ]
    Cc: Ingo Molnar <mingo@...hat.com>
    Cc: Peter Zijlstra <peterz@...radead.org>
    Cc: Juri Lelli <juri.lelli@...hat.com>
    Cc: Vincent Guittot <vincent.guittot@...aro.org>
    Cc: Dietmar Eggemann <dietmar.eggemann@....com>
    Cc: Steven Rostedt <rostedt@...dmis.org>
    Cc: Ben Segall <bsegall@...gle.com>
    Cc: Mel Gorman <mgorman@...e.de>

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3283c8d..aefea4a 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1148,4 +1148,7 @@ int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, i
 		(wait)->flags = 0;						\
 	} while (0)
 
+bool try_to_keep_sleeping(struct task_struct *p);
+void allow_awake(struct task_struct *p);
+
 #endif /* _LINUX_WAIT_H */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index fc1dfc0..b665ff7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2654,6 +2654,48 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
 }
 
 /**
+ * try_to_keep_sleeping - Attempt to force task to remain off runqueues
+ * @p: The process to remain asleep.
+ *
+ * Acquires the process's ->pi_lock and checks state.  If the process
+ * is still blocked, returns @true and leave ->pi_lock held, otherwise
+ * releases ->pi_locked and returns @false.
+ */
+bool try_to_keep_sleeping(struct task_struct *p)
+{
+	lockdep_assert_irqs_enabled();
+	raw_spin_lock_irq(&p->pi_lock);
+	switch (p->state) {
+	case TASK_RUNNING:
+	case TASK_WAKING:
+		raw_spin_unlock_irq(&p->pi_lock);
+		return false;
+
+	default:
+		smp_rmb(); /* See comments in try_to_wake_up(). */
+		if (p->on_rq) {
+			raw_spin_unlock_irq(&p->pi_lock);
+			return false;
+		}
+		return true;  /* Process is now stuck in blocked state. */
+	}
+	/* NOTREACHED */
+}
+
+/**
+ * allow_awake - Allow a kept-sleeping process to awaken
+ * @p: Process to be allowed to awaken.
+ *
+ * Given that @p was passed to an earlier call to try_to_keep_sleeping
+ * that returned @true, hence preventing @p from waking up, allow @p
+ * to once again be awakened.
+ */
+void allow_awake(struct task_struct *p)
+{
+	raw_spin_unlock_irq(&p->pi_lock);
+}
+
+/**
  * wake_up_process - Wake up a specific process
  * @p: The process to be woken up.
  *