arch: More wchan fixes

Use asm/unwind.h to implement wchan, since we cannot always rely on STACKTRACE=y.

Reported-by Stephen Rothwell <[email protected]>

Fixes bc9bbb81730e ("x86: Fix get_wchan() to support the ORC unwinder")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Recent patches to get_wchan() made it more robust by only doing the unwind when the task was blocked and serialized against wakeups.

Extract this functionality as a simpler companion to task_call_func() named task_try_func() that really only cares about blocked tasks. Then employ this new function to implement the same robustness for ARCH_STACKWALK based stack_trace_save_tsk().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/wait.h |    1 
 kernel/sched/core.c  |   62 ++++++++++++++++++++++++++++++++++++++++++++-------
 kernel/stacktrace.c  |   13 ++++++----
 3 files changed, 63 insertions(+), 13 deletions(-)

include/linux/wait.h +1 -0

@@ -1162,5 +1162,6 @@ int autoremove_wake_function(struct wait
 
 typedef int (*task_call_f)(struct task_struct *p, void *arg);
 extern int task_call_func(struct task_struct *p, task_call_f func, void *arg);
+extern int task_try_func(struct task_struct *p, task_call_f func, void *arg);
 
 #endif /* _LINUX_WAIT_H */

kernel/sched/core.c +54 -8

@@ -1966,21 +1966,21 @@ bool sched_task_on_rq(struct task_struct
 	return task_on_rq_queued(p);
 }
 
+static int try_get_wchan(struct task_struct *p, void *arg)
+{
+	unsigned long *wchan = arg;
+	*wchan = __get_wchan(p);
+	return 0;
+}
+
 unsigned long get_wchan(struct task_struct *p)
 {
 	unsigned long ip = 0;
-	unsigned int state;
 
 	if (!p || p == current)
 		return 0;
 
-	/* Only get wchan if task is blocked and we can keep it that way. */
-	raw_spin_lock_irq(&p->pi_lock);
-	state = READ_ONCE(p->__state);
-	smp_rmb(); /* see try_to_wake_up() */
-	if (state != TASK_RUNNING && state != TASK_WAKING && !p->on_rq)
-		ip = __get_wchan(p);
-	raw_spin_unlock_irq(&p->pi_lock);
+	task_try_func(p, try_get_wchan, &ip);
 
 	return ip;
 }

@@ -4184,6 +4184,52 @@ int task_call_func(struct task_struct *p
 	return ret;
 }
 
+/*
+ * task_try_func - Invoke a function on task in blocked state
+ * @p: Process for which the function is to be invoked
+ * @func: Function to invoke
+ * @arg: Argument to function
+ *
+ * Fix the task in a blocked state, when possible. And if so, invoke @func on it.
+ *
+ * Returns:
+ *  -EBUSY or whatever @func returns
+ */
+int task_try_func(struct task_struct *p, task_call_f func, void *arg)
+{
+	unsigned long flags;
+	unsigned int state;
+	int ret = -EBUSY;
+
+	raw_spin_lock_irqsave(&p->pi_lock, flags);
+
+	state = READ_ONCE(p->__state);
+
+	/*
+	 * Ensure we load p->on_rq after p->__state, otherwise it would be
+	 * possible to, falsely, observe p->on_rq == 0.
+	 *
+	 * See try_to_wake_up() for a longer comment.
+	 */
+	smp_rmb();
+
+	/*
+	 * Since pi->lock blocks try_to_wake_up(), we don't need rq->lock when
+	 * the task is blocked. Make sure to check @state since ttwu() can drop
+	 * locks at the end, see ttwu_queue_wakelist().
+	 */
+	if (state != TASK_RUNNING && state != TASK_WAKING && !p->on_rq) {
+		/*
+		 * The task is blocked and we're holding off wakeupsr. For any
+		 * of the other task states, see task_call_func().
+		 */
+		ret = func(p, arg);
+	}
+
+	raw_spin_unlock_irqrestore(&p->pi_lock, flags);
+	return ret;
+}
+
 /**
  * wake_up_process - Wake up a specific process
  * @p: The process to be woken up.

kernel/stacktrace.c +7 -1

@@ -123,6 +123,13 @@ unsigned int stack_trace_save(unsigned l
 }
 EXPORT_SYMBOL_GPL(stack_trace_save);
 
+static int try_arch_stack_walk_tsk(struct task_struct *tsk, void *arg)
+{
+	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
+	arch_stack_walk(consume_entry, arg, tsk, NULL);
+	return 0;
+}
+
 /**
  * stack_trace_save_tsk - Save a task stack trace into a storage array
  * @task:	The task to examine

@@ -135,7 +142,6 @@ EXPORT_SYMBOL_GPL(stack_trace_save);
 unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
 				  unsigned int size, unsigned int skipnr)
 {
-	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
 	struct stacktrace_cookie c = {
 		.store	= store,
 		.size	= size,

kernel/stacktrace.c +0 -2

-	arch_stack_walk(consume_entry, &c, tsk, NULL);
-	put_task_stack(tsk);
 	return c.len;
 }
 
 

Quoted 15 lines · Peter Zijlstra

/**
* stack_trace_save_tsk - Save a task stack trace into a storage array
* @task: The task to examine

kernel/stacktrace.c +0 -1

@@ -135,7 +142,6 @@ EXPORT_SYMBOL_GPL(stack_trace_save);
 unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
 				  unsigned int size, unsigned int skipnr)
 {
-	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
 	struct stacktrace_cookie c = {
 		.store	= store,
 		.size	= size,

@@ -143,11 +149,8 @@ unsigned int stack_trace_save_tsk(struct
 		.skip	= skipnr + (current == tsk),
 	};
 

kernel/stacktrace.c unsigned int stack_trace_save_tsk(struct · line 152

Quoted 3 lines

kernel/stacktrace.c +1 -2

-	if (!try_get_task_stack(tsk))
-		return 0;
+	task_try_func(tsk, try_arch_stack_walk_tsk, &c);

Pardon my thin understanding of the scheduler, but I assume this change doesn't mean stack_trace_save_tsk() stops working for "current", right? In trying to answer this for myself, I couldn't convince myself what value current->__state have here. Is it one of TASK_(UN)INTERRUPTIBLE ?

Quoted 16 lines · Kees Cook

Quoted 15 lines · Peter Zijlstra

/**
* stack_trace_save_tsk - Save a task stack trace into a storage array
* @task: The task to examine

kernel/stacktrace.c +0 -1

@@ -135,7 +142,6 @@ EXPORT_SYMBOL_GPL(stack_trace_save);
 unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
 				  unsigned int size, unsigned int skipnr)
 {
-	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
 	struct stacktrace_cookie c = {
 		.store	= store,
 		.size	= size,

@@ -143,11 +149,8 @@ unsigned int stack_trace_save_tsk(struct
 		.skip	= skipnr + (current == tsk),
 	};
 

Quoted 8 lines

kernel/stacktrace.c unsigned int stack_trace_save_tsk(struct · line 152

Quoted 3 lines

kernel/stacktrace.c +1 -2

-	if (!try_get_task_stack(tsk))
-		return 0;
+	task_try_func(tsk, try_arch_stack_walk_tsk, &c);

Pardon my thin understanding of the scheduler, but I assume this change doesn't mean stack_trace_save_tsk() stops working for "current", right? In trying to answer this for myself, I couldn't convince myself what value current->__state have here. Is it one of TASK_(UN)INTERRUPTIBLE ?

current really shouldn't be using stack_trace_save_tsk(), and no you're quite right, it will not work for current, irrespective of ->__state, current will always be ->on_rq.

I started auditing stack_trace_save_tsk() users a few days ago, but didn't look for this particular issue. I suppose I'll have to start over with that.

Quoted 30 lines · Peter Zijlstra

Recent patches to get_wchan() made it more robust by only doing the unwind when the task was blocked and serialized against wakeups.

Extract this functionality as a simpler companion to task_call_func() named task_try_func() that really only cares about blocked tasks. Then employ this new function to implement the same robustness for ARCH_STACKWALK based stack_trace_save_tsk().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/wait.h |    1 
 kernel/sched/core.c  |   62 ++++++++++++++++++++++++++++++++++++++++++++-------
 kernel/stacktrace.c  |   13 ++++++----
 3 files changed, 63 insertions(+), 13 deletions(-)

include/linux/wait.h +1 -0

@@ -1162,5 +1162,6 @@ int autoremove_wake_function(struct wait
 
 typedef int (*task_call_f)(struct task_struct *p, void *arg);
 extern int task_call_func(struct task_struct *p, task_call_f func, void *arg);
+extern int task_try_func(struct task_struct *p, task_call_f func, void *arg);
 
 #endif /* _LINUX_WAIT_H */

kernel/sched/core.c +0 -0

@@ -1966,21 +1966,21 @@ bool sched_task_on_rq(struct task_struct
 	return task_on_rq_queued(p);
 }
 

kernel/sched/core.c bool sched_task_on_rq(struct task_struct · line 1971

Quoted 3 lines

kernel/sched/core.c +3 -0

+static int try_get_wchan(struct task_struct *p, void *arg)
+{
+	unsigned long *wchan = arg;

Quoted 103 lines

kernel/sched/core.c +50 -8

+	return 0;
+}
+
 unsigned long get_wchan(struct task_struct *p)
 {
 	unsigned long ip = 0;
-	unsigned int state;
 
 	if (!p || p == current)
 		return 0;
 
-	/* Only get wchan if task is blocked and we can keep it that way. */
-	raw_spin_lock_irq(&p->pi_lock);
-	state = READ_ONCE(p->__state);
-	smp_rmb(); /* see try_to_wake_up() */
-	if (state != TASK_RUNNING && state != TASK_WAKING && !p->on_rq)
-		ip = __get_wchan(p);
-	raw_spin_unlock_irq(&p->pi_lock);
+	task_try_func(p, try_get_wchan, &ip);
 
 	return ip;
 }

@@ -4184,6 +4184,52 @@ int task_call_func(struct task_struct *p
 	return ret;
 }
 
+/*
+ * task_try_func - Invoke a function on task in blocked state
+ * @p: Process for which the function is to be invoked
+ * @func: Function to invoke
+ * @arg: Argument to function
+ *
+ * Fix the task in a blocked state, when possible. And if so, invoke @func on it.
+ *
+ * Returns:
+ *  -EBUSY or whatever @func returns
+ */
+int task_try_func(struct task_struct *p, task_call_f func, void *arg)
+{
+	unsigned long flags;
+	unsigned int state;
+	int ret = -EBUSY;
+
+	raw_spin_lock_irqsave(&p->pi_lock, flags);
+
+	state = READ_ONCE(p->__state);
+
+	/*
+	 * Ensure we load p->on_rq after p->__state, otherwise it would be
+	 * possible to, falsely, observe p->on_rq == 0.
+	 *
+	 * See try_to_wake_up() for a longer comment.
+	 */
+	smp_rmb();
+
+	/*
+	 * Since pi->lock blocks try_to_wake_up(), we don't need rq->lock when
+	 * the task is blocked. Make sure to check @state since ttwu() can drop
+	 * locks at the end, see ttwu_queue_wakelist().
+	 */
+	if (state != TASK_RUNNING && state != TASK_WAKING && !p->on_rq) {
+		/*
+		 * The task is blocked and we're holding off wakeupsr. For any
+		 * of the other task states, see task_call_func().
+		 */
+		ret = func(p, arg);
+	}
+
+	raw_spin_unlock_irqrestore(&p->pi_lock, flags);
+	return ret;
+}
+
 /**
  * wake_up_process - Wake up a specific process
  * @p: The process to be woken up.

kernel/stacktrace.c +7 -1

@@ -123,6 +123,13 @@ unsigned int stack_trace_save(unsigned l
 }
 EXPORT_SYMBOL_GPL(stack_trace_save);
 
+static int try_arch_stack_walk_tsk(struct task_struct *tsk, void *arg)
+{
+	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
+	arch_stack_walk(consume_entry, arg, tsk, NULL);
+	return 0;
+}
+
 /**
  * stack_trace_save_tsk - Save a task stack trace into a storage array
  * @task:	The task to examine

@@ -135,7 +142,6 @@ EXPORT_SYMBOL_GPL(stack_trace_save);
 unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
 				  unsigned int size, unsigned int skipnr)
 {
-	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
 	struct stacktrace_cookie c = {
 		.store	= store,
 		.size	= size,

@@ -143,11 +149,8 @@ unsigned int stack_trace_save_tsk(struct
 		.skip	= skipnr + (current == tsk),
 	};
 

kernel/stacktrace.c unsigned int stack_trace_save_tsk(struct · line 152

Quoted 3 lines

kernel/stacktrace.c +1 -2

-	if (!try_get_task_stack(tsk))
-		return 0;
+	task_try_func(tsk, try_arch_stack_walk_tsk, &c);

Pardon my thin understanding of the scheduler, but I assume this change doesn't mean stack_trace_save_tsk() stops working for "current", right? In trying to answer this for myself, I couldn't convince myself what value current->__state have here. Is it one of TASK_(UN)INTERRUPTIBLE ?

Quoted 4 lines

Pardon my thin understanding of the scheduler, but I assume this change doesn't mean stack_trace_save_tsk() stops working for "current", right? In trying to answer this for myself, I couldn't convince myself what value current->__state have here. Is it one of TASK_(UN)INTERRUPTIBLE ?

Regardless of that, current->on_rq will be non-zero, so you're right that this causes stack_trace_save_tsk() to not work for current, e.g.

| # cat /proc/self/stack 
| # wc  /proc/self/stack 
|         0         0         0 /proc/self/stack

TBH, I think that (taking a step back from this issue in particular) stack_trace_save_tsk() *shouldn't* work for current, and callers *should* be forced to explicitly handle current separately from blocked tasks.

So we could fix this in the stacktrace code with:

| diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
| index a1cdbf8c3ef8..327af9ff2c55 100644
| --- a/kernel/stacktrace.c
| +++ b/kernel/stacktrace.c
| @@ -149,7 +149,10 @@ unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
|                 .skip   = skipnr + (current == tsk),
|         };
|  
| -       task_try_func(tsk, try_arch_stack_walk_tsk, &c);
| +       if (tsk == current)
| +               try_arch_stack_walk_tsk(tsk, &c);
| +       else
| +               task_try_func(tsk, try_arch_stack_walk_tsk, &c);
|  
|         return c.len;
|  }

... and we could rename task_try_func() to blocked_task_try_func(), and later push the distinction into higher-level callers.

Quoted 12 lines · Mark Rutland

Quoted 4 lines

Pardon my thin understanding of the scheduler, but I assume this change doesn't mean stack_trace_save_tsk() stops working for "current", right? In trying to answer this for myself, I couldn't convince myself what value current->__state have here. Is it one of TASK_(UN)INTERRUPTIBLE ?

Regardless of that, current->on_rq will be non-zero, so you're right that this causes stack_trace_save_tsk() to not work for current, e.g.

| # cat /proc/self/stack 
| # wc  /proc/self/stack 
|         0         0         0 /proc/self/stack

Quoted 3 lines

TBH, I think that (taking a step back from this issue in particular) stack_trace_save_tsk() *shouldn't* work for current, and callers *should* be forced to explicitly handle current separately from blocked tasks.

That..

Quoted 19 lines

So we could fix this in the stacktrace code with:

| diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
| index a1cdbf8c3ef8..327af9ff2c55 100644
| --- a/kernel/stacktrace.c
| +++ b/kernel/stacktrace.c
| @@ -149,7 +149,10 @@ unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
|                 .skip   = skipnr + (current == tsk),
|         };
|  
| -       task_try_func(tsk, try_arch_stack_walk_tsk, &c);
| +       if (tsk == current)
| +               try_arch_stack_walk_tsk(tsk, &c);
| +       else
| +               task_try_func(tsk, try_arch_stack_walk_tsk, &c);
|  
|         return c.len;
|  }

Quoted 2 lines

... and we could rename task_try_func() to blocked_task_try_func(), and later push the distinction into higher-level callers.

I think I favour this fix if we have to. But that's for next week :-)

Quoted 14 lines · Peter Zijlstra

Quoted 12 lines · Mark Rutland

Quoted 4 lines

Pardon my thin understanding of the scheduler, but I assume this change doesn't mean stack_trace_save_tsk() stops working for "current", right? In trying to answer this for myself, I couldn't convince myself what value current->__state have here. Is it one of TASK_(UN)INTERRUPTIBLE ?

Regardless of that, current->on_rq will be non-zero, so you're right that this causes stack_trace_save_tsk() to not work for current, e.g.

| # cat /proc/self/stack 
| # wc  /proc/self/stack 
|         0         0         0 /proc/self/stack

Quoted 5 lines

Quoted 3 lines

TBH, I think that (taking a step back from this issue in particular) stack_trace_save_tsk() *shouldn't* work for current, and callers *should* be forced to explicitly handle current separately from blocked tasks.

That..

So I think I'd prefer the following approach to that (and i'm not currently volunteering for it):

- convert all archs to ARCH_STACKWALK; this gets the semantics out of arch code and into the single kernel/stacktrace.c file.

- bike-shed a new/improved stack_trace_save*() API and implement it *once* in generic code based on arch_stack_walk().

- convert users; delete old etc..

For now, current users of stack_trace_save_tsk() very much expect tsk==current to work.

Quoted 19 lines

Quoted 19 lines

So we could fix this in the stacktrace code with:

| diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
| index a1cdbf8c3ef8..327af9ff2c55 100644
| --- a/kernel/stacktrace.c
| +++ b/kernel/stacktrace.c
| @@ -149,7 +149,10 @@ unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
|                 .skip   = skipnr + (current == tsk),
|         };
|  
| -       task_try_func(tsk, try_arch_stack_walk_tsk, &c);
| +       if (tsk == current)
| +               try_arch_stack_walk_tsk(tsk, &c);
| +       else
| +               task_try_func(tsk, try_arch_stack_walk_tsk, &c);
|  
|         return c.len;
|  }

Quoted 4 lines

Quoted 2 lines

... and we could rename task_try_func() to blocked_task_try_func(), and later push the distinction into higher-level callers.

I think I favour this fix if we have to. But that's for next week :-)

I ended up with the below delta to this patch.

kernel/stacktrace.c +15 -3

@@ -101,7 +101,7 @@ static bool stack_trace_consume_entry_no
 }
 
 /**
- * stack_trace_save - Save a stack trace into a storage array
+ * stack_trace_save - Save a stack trace (of current) into a storage array
  * @store:	Pointer to storage array
  * @size:	Size of the storage array
  * @skipnr:	Number of entries to skip at the start of the stack trace

@@ -132,7 +132,7 @@ static int try_arch_stack_walk_tsk(struc
 
 /**
  * stack_trace_save_tsk - Save a task stack trace into a storage array
- * @task:	The task to examine
+ * @task:	The task to examine (current allowed)
  * @store:	Pointer to storage array
  * @size:	Size of the storage array
  * @skipnr:	Number of entries to skip at the start of the stack trace

@@ -149,13 +149,25 @@ unsigned int stack_trace_save_tsk(struct
 		.skip	= skipnr + (current == tsk),
 	};
 
-	task_try_func(tsk, try_arch_stack_walk_tsk, &c);
+	/*
+	 * If the task doesn't have a stack (e.g., a zombie), the stack is
+	 * empty.
+	 */
+	if (!try_get_task_stack(tsk))
+		return 0;
+
+	if (tsk == current)
+		try_arch_stack_walk_tsk(tsk, &c);
+	else
+		task_try_func(tsk, try_arch_stack_walk_tsk, &c);
+
+	put_task_stack(tsk);

Quoted 14 lines · Peter Zijlstra

Quoted 12 lines · Mark Rutland

Quoted 4 lines

Pardon my thin understanding of the scheduler, but I assume this change doesn't mean stack_trace_save_tsk() stops working for "current", right? In trying to answer this for myself, I couldn't convince myself what value current->__state have here. Is it one of TASK_(UN)INTERRUPTIBLE ?

Regardless of that, current->on_rq will be non-zero, so you're right that this causes stack_trace_save_tsk() to not work for current, e.g.

| # cat /proc/self/stack 
| # wc  /proc/self/stack 
|         0         0         0 /proc/self/stack

Quoted 5 lines

Quoted 3 lines

TBH, I think that (taking a step back from this issue in particular) stack_trace_save_tsk() *shouldn't* work for current, and callers *should* be forced to explicitly handle current separately from blocked tasks.

That..

So I think I'd prefer the following approach to that (and i'm not currently volunteering for it):

- convert all archs to ARCH_STACKWALK; this gets the semantics out of arch code and into the single kernel/stacktrace.c file.

- bike-shed a new/improved stack_trace_save*() API and implement it *once* in generic code based on arch_stack_walk().

- convert users; delete old etc..

For now, current users of stack_trace_save_tsk() very much expect tsk==current to work.

Quoted 19 lines

Quoted 19 lines

So we could fix this in the stacktrace code with:

| diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
| index a1cdbf8c3ef8..327af9ff2c55 100644
| --- a/kernel/stacktrace.c
| +++ b/kernel/stacktrace.c
| @@ -149,7 +149,10 @@ unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
|                 .skip   = skipnr + (current == tsk),
|         };
|  
| -       task_try_func(tsk, try_arch_stack_walk_tsk, &c);
| +       if (tsk == current)
| +               try_arch_stack_walk_tsk(tsk, &c);
| +       else
| +               task_try_func(tsk, try_arch_stack_walk_tsk, &c);
|  
|         return c.len;
|  }

Quoted 4 lines

Quoted 2 lines

... and we could rename task_try_func() to blocked_task_try_func(), and later push the distinction into higher-level callers.

I think I favour this fix if we have to. But that's for next week :-)

I ended up with the below delta to this patch.

kernel/stacktrace.c +16 -4

@@ -101,7 +101,7 @@ static bool stack_trace_consume_entry_no
 }
 
 /**
- * stack_trace_save - Save a stack trace into a storage array
+ * stack_trace_save - Save a stack trace (of current) into a storage array
  * @store:	Pointer to storage array
  * @size:	Size of the storage array
  * @skipnr:	Number of entries to skip at the start of the stack trace

@@ -132,7 +132,7 @@ static int try_arch_stack_walk_tsk(struc
 
 /**
  * stack_trace_save_tsk - Save a task stack trace into a storage array
- * @task:	The task to examine
+ * @task:	The task to examine (current allowed)
  * @store:	Pointer to storage array
  * @size:	Size of the storage array
  * @skipnr:	Number of entries to skip at the start of the stack trace

@@ -149,13 +149,25 @@ unsigned int stack_trace_save_tsk(struct
 		.skip	= skipnr + (current == tsk),
 	};
 
-	task_try_func(tsk, try_arch_stack_walk_tsk, &c);
+	/*
+	 * If the task doesn't have a stack (e.g., a zombie), the stack is
+	 * empty.
+	 */
+	if (!try_get_task_stack(tsk))
+		return 0;
+
+	if (tsk == current)
+		try_arch_stack_walk_tsk(tsk, &c);
+	else
+		task_try_func(tsk, try_arch_stack_walk_tsk, &c);
+
+	put_task_stack(tsk);
 
 	return c.len;
 }
 
 /**
- * stack_trace_save_regs - Save a stack trace based on pt_regs into a storage array
+ * stack_trace_save_regs - Save a stack trace (of current) based on pt_regs into a storage array
  * @regs:	Pointer to pt_regs to examine
  * @store:	Pointer to storage array
  * @size:	Size of the storage array

kernel/stacktrace.c +7 -1

@@ -123,6 +123,13 @@ unsigned int stack_trace_save(unsigned l
 }
 EXPORT_SYMBOL_GPL(stack_trace_save);
 
+static int try_arch_stack_walk_tsk(struct task_struct *tsk, void *arg)
+{
+	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
+	arch_stack_walk(consume_entry, arg, tsk, NULL);
+	return 0;
+}
+
 /**
  * stack_trace_save_tsk - Save a task stack trace into a storage array
  * @task:	The task to examine

@@ -135,7 +142,6 @@ EXPORT_SYMBOL_GPL(stack_trace_save);
 unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
 				  unsigned int size, unsigned int skipnr)
 {
-	stack_trace_consume_fn consume_entry = stack_trace_consume_entry_nosched;
 	struct stacktrace_cookie c = {
 		.store	= store,
 		.size	= size,

kernel/stacktrace.c +1 -2

+	task_try_func(tsk, try_arch_stack_walk_tsk, &c);
 
-	arch_stack_walk(consume_entry, &c, tsk, NULL);
-	put_task_stack(tsk);
 	return c.len;
 }
 

From: Ard Biesheuvel <[email protected]>

Implement the flavor of CONFIG_STACKTRACE that relies mostly on generic code, and only need a small arch_stack_walk() helper that performs the actual frame unwinding.

Note that this removes the SMP check that used to live in __save_stack_trace(), but this is no longer needed now that the generic version of save_stack_trace_tsk() takes care not to walk the call stack of tasks that are live on other CPUs.

Signed-off-by Ard Biesheuvel <[email protected]>

Make arch_stack_walk() available for ARCH_STACKWALK architectures without it being entangled in STACKTRACE.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm/kernel/stacktrace.c   |    2 --
 arch/arm64/kernel/stacktrace.c |    4 ----
 arch/powerpc/kernel/Makefile   |    3 +--
 arch/riscv/kernel/stacktrace.c |    4 ----
 arch/s390/kernel/Makefile      |    3 +--
 arch/x86/kernel/Makefile       |    2 +-
 include/linux/stacktrace.h     |   33 +++++++++++++++++----------------
 7 files changed, 20 insertions(+), 31 deletions(-)

arch/arm/kernel/stacktrace.c +0 -2

@@ -87,7 +87,6 @@ void notrace walk_stackframe(struct stac
 }
 EXPORT_SYMBOL(walk_stackframe);
 
-#ifdef CONFIG_STACKTRACE
 noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 				      void *cookie, struct task_struct *task,
 				      struct pt_regs *regs)

@@ -113,4 +112,3 @@ noinline notrace void arch_stack_walk(st
 			break;
 	}
 }
-#endif

arch/arm64/kernel/stacktrace.c +0 -4

@@ -216,8 +216,6 @@ void show_stack(struct task_struct *tsk,
 	barrier();
 }
 
-#ifdef CONFIG_STACKTRACE
-
 noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 			      void *cookie, struct task_struct *task,
 			      struct pt_regs *regs)

@@ -236,5 +234,3 @@ noinline notrace void arch_stack_walk(st
 
 	walk_stackframe(task, &frame, consume_entry, cookie);
 }
-
-#endif

arch/powerpc/kernel/Makefile +1 -2

@@ -47,7 +47,7 @@ obj-y				:= cputable.o syscalls.o \
 				   udbg.o misc.o io.o misc_$(BITS).o \
 				   of_platform.o prom_parse.o firmware.o \
 				   hw_breakpoint_constraints.o interrupt.o \
-				   kdebugfs.o
+				   kdebugfs.o stacktrace.o
 obj-y				+= ptrace/
 obj-$(CONFIG_PPC64)		+= setup_64.o \
 				   paca.o nvram_64.o note.o

@@ -116,7 +116,6 @@ obj-$(CONFIG_OPTPROBES)		+= optprobes.o
 obj-$(CONFIG_KPROBES_ON_FTRACE)	+= kprobes-ftrace.o
 obj-$(CONFIG_UPROBES)		+= uprobes.o
 obj-$(CONFIG_PPC_UDBG_16550)	+= legacy_serial.o udbg_16550.o
-obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_SWIOTLB)		+= dma-swiotlb.o
 obj-$(CONFIG_ARCH_HAS_DMA_SET_MASK) += dma-mask.o
 

arch/riscv/kernel/stacktrace.c +0 -4

@@ -139,12 +139,8 @@ unsigned long __get_wchan(struct task_st
 	return pc;
 }
 
-#ifdef CONFIG_STACKTRACE
-
 noinline void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 		     struct task_struct *task, struct pt_regs *regs)
 {
 	walk_stackframe(task, regs, consume_entry, cookie);
 }
-
-#endif /* CONFIG_STACKTRACE */

arch/s390/kernel/Makefile +1 -2

@@ -40,7 +40,7 @@ obj-y	+= sysinfo.o lgr.o os_info.o machi
 obj-y	+= runtime_instr.o cache.o fpu.o dumpstack.o guarded_storage.o sthyi.o
 obj-y	+= entry.o reipl.o relocate_kernel.o kdebugfs.o alternative.o
 obj-y	+= nospec-branch.o ipl_vmparm.o machine_kexec_reloc.o unwind_bc.o
-obj-y	+= smp.o text_amode31.o
+obj-y	+= smp.o text_amode31.o stacktrace.o
 
 extra-y				+= head64.o vmlinux.lds
 

@@ -55,7 +55,6 @@ compat-obj-$(CONFIG_AUDIT)	+= compat_aud
 obj-$(CONFIG_COMPAT)		+= compat_linux.o compat_signal.o
 obj-$(CONFIG_COMPAT)		+= $(compat-obj-y)
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
-obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o
 obj-$(CONFIG_KPROBES)		+= kprobes_insn_page.o
 obj-$(CONFIG_FUNCTION_TRACER)	+= mcount.o ftrace.o

arch/x86/kernel/Makefile +1 -1

@@ -84,7 +84,7 @@ obj-$(CONFIG_IA32_EMULATION)	+= tls.o
 obj-y				+= step.o
 obj-$(CONFIG_INTEL_TXT)		+= tboot.o
 obj-$(CONFIG_ISA_DMA_API)	+= i8237.o
-obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
+obj-y				+= stacktrace.o
 obj-y				+= cpu/
 obj-y				+= acpi/
 obj-y				+= reboot.o

include/linux/stacktrace.h +17 -16

@@ -8,21 +8,6 @@
 struct task_struct;
 struct pt_regs;
 
-#ifdef CONFIG_STACKTRACE
-void stack_trace_print(const unsigned long *trace, unsigned int nr_entries,
-		       int spaces);
-int stack_trace_snprint(char *buf, size_t size, const unsigned long *entries,
-			unsigned int nr_entries, int spaces);
-unsigned int stack_trace_save(unsigned long *store, unsigned int size,
-			      unsigned int skipnr);
-unsigned int stack_trace_save_tsk(struct task_struct *task,
-				  unsigned long *store, unsigned int size,
-				  unsigned int skipnr);
-unsigned int stack_trace_save_regs(struct pt_regs *regs, unsigned long *store,
-				   unsigned int size, unsigned int skipnr);
-unsigned int stack_trace_save_user(unsigned long *store, unsigned int size);
-
-/* Internal interfaces. Do not use in generic code */
 #ifdef CONFIG_ARCH_STACKWALK
 
 /**

@@ -75,8 +60,24 @@ int arch_stack_walk_reliable(stack_trace
 
 void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
 			  const struct pt_regs *regs);
+#endif /* CONFIG_ARCH_STACKWALK */
 
-#else /* CONFIG_ARCH_STACKWALK */
+#ifdef CONFIG_STACKTRACE
+void stack_trace_print(const unsigned long *trace, unsigned int nr_entries,
+		       int spaces);
+int stack_trace_snprint(char *buf, size_t size, const unsigned long *entries,
+			unsigned int nr_entries, int spaces);
+unsigned int stack_trace_save(unsigned long *store, unsigned int size,
+			      unsigned int skipnr);
+unsigned int stack_trace_save_tsk(struct task_struct *task,
+				  unsigned long *store, unsigned int size,
+				  unsigned int skipnr);
+unsigned int stack_trace_save_regs(struct pt_regs *regs, unsigned long *store,
+				   unsigned int size, unsigned int skipnr);
+unsigned int stack_trace_save_user(unsigned long *store, unsigned int size);
+
+#ifndef CONFIG_ARCH_STACKWALK
+/* Internal interfaces. Do not use in generic code */
 struct stack_trace {
 	unsigned int nr_entries, max_entries;
 	unsigned long *entries;

Quoted 2 lines · Peter Zijlstra

Make arch_stack_walk() available for ARCH_STACKWALK architectures without it being entangled in STACKTRACE.

Which CONFIG/arch combos did you build test with this? It looks good, but I always expect things like this to end up landing in corner cases. :)

Make arch_stack_walk() available for ARCH_STACKWALK architectures without it being entangled in STACKTRACE.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm/kernel/stacktrace.c   |    2 --
 arch/arm64/kernel/stacktrace.c |    4 ----
 arch/powerpc/kernel/Makefile   |    3 +--
 arch/riscv/kernel/stacktrace.c |    4 ----
 arch/s390/kernel/Makefile      |    3 +--
 arch/x86/kernel/Makefile       |    2 +-
 include/linux/stacktrace.h     |   33 +++++++++++++++++----------------
 7 files changed, 20 insertions(+), 31 deletions(-)

arch/arm/kernel/stacktrace.c +0 -2

@@ -87,7 +87,6 @@ void notrace walk_stackframe(struct stac
 }
 EXPORT_SYMBOL(walk_stackframe);
 
-#ifdef CONFIG_STACKTRACE
 noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 				      void *cookie, struct task_struct *task,
 				      struct pt_regs *regs)

@@ -113,4 +112,3 @@ noinline notrace void arch_stack_walk(st
 			break;
 	}
 }
-#endif

arch/arm64/kernel/stacktrace.c +0 -4

@@ -216,8 +216,6 @@ void show_stack(struct task_struct *tsk,
 	barrier();
 }
 
-#ifdef CONFIG_STACKTRACE
-
 noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 			      void *cookie, struct task_struct *task,
 			      struct pt_regs *regs)

@@ -236,5 +234,3 @@ noinline notrace void arch_stack_walk(st
 
 	walk_stackframe(task, &frame, consume_entry, cookie);
 }
-
-#endif

arch/powerpc/kernel/Makefile +1 -2

@@ -47,7 +47,7 @@ obj-y				:= cputable.o syscalls.o \
 				   udbg.o misc.o io.o misc_$(BITS).o \
 				   of_platform.o prom_parse.o firmware.o \
 				   hw_breakpoint_constraints.o interrupt.o \
-				   kdebugfs.o
+				   kdebugfs.o stacktrace.o
 obj-y				+= ptrace/
 obj-$(CONFIG_PPC64)		+= setup_64.o \
 				   paca.o nvram_64.o note.o

@@ -116,7 +116,6 @@ obj-$(CONFIG_OPTPROBES)		+= optprobes.o
 obj-$(CONFIG_KPROBES_ON_FTRACE)	+= kprobes-ftrace.o
 obj-$(CONFIG_UPROBES)		+= uprobes.o
 obj-$(CONFIG_PPC_UDBG_16550)	+= legacy_serial.o udbg_16550.o
-obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_SWIOTLB)		+= dma-swiotlb.o
 obj-$(CONFIG_ARCH_HAS_DMA_SET_MASK) += dma-mask.o
 

arch/riscv/kernel/stacktrace.c +0 -4

@@ -139,12 +139,8 @@ unsigned long __get_wchan(struct task_st
 	return pc;
 }
 
-#ifdef CONFIG_STACKTRACE
-
 noinline void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 		     struct task_struct *task, struct pt_regs *regs)
 {
 	walk_stackframe(task, regs, consume_entry, cookie);
 }
-
-#endif /* CONFIG_STACKTRACE */

arch/s390/kernel/Makefile +1 -2

@@ -40,7 +40,7 @@ obj-y	+= sysinfo.o lgr.o os_info.o machi
 obj-y	+= runtime_instr.o cache.o fpu.o dumpstack.o guarded_storage.o sthyi.o
 obj-y	+= entry.o reipl.o relocate_kernel.o kdebugfs.o alternative.o
 obj-y	+= nospec-branch.o ipl_vmparm.o machine_kexec_reloc.o unwind_bc.o
-obj-y	+= smp.o text_amode31.o
+obj-y	+= smp.o text_amode31.o stacktrace.o
 
 extra-y				+= head64.o vmlinux.lds
 

@@ -55,7 +55,6 @@ compat-obj-$(CONFIG_AUDIT)	+= compat_aud
 obj-$(CONFIG_COMPAT)		+= compat_linux.o compat_signal.o
 obj-$(CONFIG_COMPAT)		+= $(compat-obj-y)
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
-obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o
 obj-$(CONFIG_KPROBES)		+= kprobes_insn_page.o
 obj-$(CONFIG_FUNCTION_TRACER)	+= mcount.o ftrace.o

arch/x86/kernel/Makefile +1 -1

@@ -84,7 +84,7 @@ obj-$(CONFIG_IA32_EMULATION)	+= tls.o
 obj-y				+= step.o
 obj-$(CONFIG_INTEL_TXT)		+= tboot.o
 obj-$(CONFIG_ISA_DMA_API)	+= i8237.o
-obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
+obj-y				+= stacktrace.o
 obj-y				+= cpu/
 obj-y				+= acpi/
 obj-y				+= reboot.o

include/linux/stacktrace.h +17 -16

@@ -8,21 +8,6 @@
 struct task_struct;
 struct pt_regs;
 
-#ifdef CONFIG_STACKTRACE
-void stack_trace_print(const unsigned long *trace, unsigned int nr_entries,
-		       int spaces);
-int stack_trace_snprint(char *buf, size_t size, const unsigned long *entries,
-			unsigned int nr_entries, int spaces);
-unsigned int stack_trace_save(unsigned long *store, unsigned int size,
-			      unsigned int skipnr);
-unsigned int stack_trace_save_tsk(struct task_struct *task,
-				  unsigned long *store, unsigned int size,
-				  unsigned int skipnr);
-unsigned int stack_trace_save_regs(struct pt_regs *regs, unsigned long *store,
-				   unsigned int size, unsigned int skipnr);
-unsigned int stack_trace_save_user(unsigned long *store, unsigned int size);
-
-/* Internal interfaces. Do not use in generic code */
 #ifdef CONFIG_ARCH_STACKWALK
 
 /**

@@ -75,8 +60,24 @@ int arch_stack_walk_reliable(stack_trace
 
 void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
 			  const struct pt_regs *regs);
+#endif /* CONFIG_ARCH_STACKWALK */
 
-#else /* CONFIG_ARCH_STACKWALK */
+#ifdef CONFIG_STACKTRACE
+void stack_trace_print(const unsigned long *trace, unsigned int nr_entries,
+		       int spaces);
+int stack_trace_snprint(char *buf, size_t size, const unsigned long *entries,
+			unsigned int nr_entries, int spaces);
+unsigned int stack_trace_save(unsigned long *store, unsigned int size,
+			      unsigned int skipnr);
+unsigned int stack_trace_save_tsk(struct task_struct *task,
+				  unsigned long *store, unsigned int size,
+				  unsigned int skipnr);
+unsigned int stack_trace_save_regs(struct pt_regs *regs, unsigned long *store,
+				   unsigned int size, unsigned int skipnr);
+unsigned int stack_trace_save_user(unsigned long *store, unsigned int size);
+
+#ifndef CONFIG_ARCH_STACKWALK
+/* Internal interfaces. Do not use in generic code */
 struct stack_trace {
 	unsigned int nr_entries, max_entries;
 	unsigned long *entries;

Unlike most of the other architectures, PowerPC and ARM64 have __switch_to() as a C function which remains on the stack. Their respective __get_wchan() skips one stack frame unconditionally, without testing is_sched_functions().

Mark them __sched such that we can forgo that special case.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/kernel/process.c   |    4 ++--
 arch/powerpc/kernel/process.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

arch/arm64/kernel/process.c +2 -2

@@ -490,8 +490,8 @@ void update_sctlr_el1(u64 sctlr)
 /*
  * Thread switching.
  */
-__notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
-				struct task_struct *next)
+__notrace_funcgraph __sched
+struct task_struct *__switch_to(struct task_struct *prev, struct task_struct *next)
 {
 	struct task_struct *last;
 

arch/powerpc/kernel/process.c +2 -2

@@ -1201,8 +1201,8 @@ static inline void restore_sprs(struct t
 
 }
 
-struct task_struct *__switch_to(struct task_struct *prev,
-	struct task_struct *new)
+__sched struct task_struct *__switch_to(struct task_struct *prev,
+					struct task_struct *new)
 {
 	struct thread_struct *new_thread, *old_thread;
 	struct task_struct *last;

Unlike most of the other architectures, PowerPC and ARM64 have __switch_to() as a C function which remains on the stack.

Their
respective __get_wchan() skips one stack frame unconditionally, without testing is_sched_functions().

Mark them __sched such that we can forgo that special case.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/kernel/process.c   |    4 ++--
 arch/powerpc/kernel/process.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

arch/arm64/kernel/process.c +0 -0

 {
 	struct task_struct *last;
 

arch/powerpc/kernel/process.c +2 -2

@@ -1201,8 +1201,8 @@ static inline void restore_sprs(struct t
 
 }
 
-struct task_struct *__switch_to(struct task_struct *prev,
-	struct task_struct *new)
+__sched struct task_struct *__switch_to(struct task_struct *prev,
+					struct task_struct *new)
 {
 	struct thread_struct *new_thread, *old_thread;
 	struct task_struct *last;

Hi,

Respin of the rest of the get_wchan() rework [1]. Given the many problems with STACKTRACE this series focuses on the newer ARCH_STACKWALK since that's a smaller set of architectures to review with a better specified interface.

The idea is to piece-wise convert the rest of the architectures to ARCH_STACKWALK, but that can be done at leasure.

PowerPC and ARM64 are a bit funny in that their __switch_to() is in C and ends up on the stack. For now simply mark it __sched to make it work. Mark wanted to maybe do something different, but I think this ought to get us started.

Use ARCH_STACKWALK to implement a generic __get_wchan().

STACKTRACE should be possible, but the various implementations of stack_trace_save_tsk() are not consistent enough for this to work. ARCH_STACKWALK is a smaller set of architectures with a better defined interface.

Since get_wchan() pins the task in a blocked state, it is not necessary to take a reference on the task stack, the task isn't going anywhere.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Use ARCH_STACKWALK to implement a generic __get_wchan().

STACKTRACE should be possible, but the various implementations of stack_trace_save_tsk() are not consistent enough for this to work. ARCH_STACKWALK is a smaller set of architectures with a better defined interface.

Since get_wchan() pins the task in a blocked state, it is not necessary to take a reference on the task stack, the task isn't going anywhere.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm/include/asm/processor.h     |    2 -
 arch/arm/kernel/process.c            |   22 --------------------
 arch/arm64/include/asm/processor.h   |    2 -
 arch/arm64/kernel/process.c          |   26 ------------------------
 arch/powerpc/include/asm/processor.h |    2 -
 arch/powerpc/kernel/process.c        |   37 -----------------------------------
 arch/riscv/include/asm/processor.h   |    3 --
 arch/riscv/kernel/stacktrace.c       |   21 -------------------
 arch/s390/include/asm/processor.h    |    1 
 arch/s390/kernel/process.c           |   29 ---------------------------
 arch/x86/include/asm/processor.h     |    2 -
 arch/x86/kernel/process.c            |   25 -----------------------
 kernel/sched/core.c                  |   24 ++++++++++++++++++++++
 13 files changed, 24 insertions(+), 172 deletions(-)

arch/arm64/kernel/process.c +0 -25

@@ -528,32 +528,6 @@ struct task_struct *__switch_to(struct t
 	return last;
 }
 
-unsigned long __get_wchan(struct task_struct *p)
-{
-	struct stackframe frame;
-	unsigned long stack_page, ret = 0;
-	int count = 0;
-
-	stack_page = (unsigned long)try_get_task_stack(p);
-	if (!stack_page)
-		return 0;
-
-	start_backtrace(&frame, thread_saved_fp(p), thread_saved_pc(p));
-
-	do {
-		if (unwind_frame(p, &frame))
-			goto out;
-		if (!in_sched_functions(frame.pc)) {
-			ret = frame.pc;
-			goto out;
-		}
-	} while (count++ < 16);
-
-out:
-	put_task_stack(p);
-	return ret;
-}
 

kernel/sched/core.c +11 -0

@@ -1966,6 +1966,30 @@ bool sched_task_on_rq(struct task_struct
 	return task_on_rq_queued(p);
 }
 
+#ifdef CONFIG_ARCH_STACKWALK
+
+static bool consume_wchan(void *cookie, unsigned long addr)
+{
+	unsigned long *wchan = cookie;
+
+	if (in_sched_functions(addr))
+		return true;
+
+	*wchan = addr;
+	return false;