[PATCH 0/3] Kill off show_stack() NULL-implies-current idiom

[PATCH 0/3] Kill off show_stack() NULL-implies-current idiom

[Mark Rutland]

Today, show_stack() accepts a NULL task parameter, which it takes to mean the
current task. However, as noted in tip/x86/asm commit:

  81539169f283329f ("x86/dumpstack: Remove NULL task pointer convention")

... having a NULL task parameter imply current leads to subtle bugs in stack
walking code (so far seen on both 86 and arm64), makes callsites harder to
read, and is unnecessary as all callers have access to current.

As a step towards removing the problematic NULL-implies-current idiom entirely,
these patches ensure that generic code explictly passes current to
show_stack(), rather than relying on arch code to handle NULL.

Thanks,
Mark.

Mark Rutland (3):
  sched: document that show_stack() should not be passed a NULL task
  drivers/tty: Explicitly pass current to show_stack
  lib: dump_stack: explicitly pass current to show_stack

 drivers/tty/sysrq.c   | 2 +-
 include/linux/sched.h | 3 +++
 lib/dump_stack.c      | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH 1/3] sched: document that show_stack() should not be passed a NULL task

[Mark Rutland]

As noted in commit:

  81539169f283329f ("x86/dumpstack: Remove NULL task pointer convention")

... having a NULL task parameter imply current leads to subtle bugs in stack
walking code (so far seen on both 86 and arm64), makes callsites harder to
read, and is unnecessary as all callers have access to current.

As a step towards removing the problematic NULL-implies-current idiom entirely,
document that new code should pass current explicitly.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org
---
 include/linux/sched.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index abb795a..4bc5571 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -367,6 +367,9 @@ extern void show_regs(struct pt_regs *);
  * TASK is a pointer to the task whose backtrace we want to see (or NULL for current
  * task), SP is the stack pointer of the first frame that should be shown in the back
  * trace (or NULL if the entire call-chain of the task should be shown).
+ *
+ * Note: passing a NULL task is deprecated, and new code should pass current
+ * explicitly.
  */
 extern void show_stack(struct task_struct *task, unsigned long *sp);
 
-- 
2.7.4

[PATCH 2/3] drivers/tty: Explicitly pass current to show_stack

[Mark Rutland]

As noted in commit:

  81539169f283329f ("x86/dumpstack: Remove NULL task pointer convention")

... having a NULL task parameter imply current leads to subtle bugs in stack
walking code (so far seen on both 86 and arm64), makes callsites harder to
read, and is unnecessary as all callers have access to current.

As a step towards removing the problematic NULL-implies-current idiom entirely,
have the sysrq code explicitly pass current to show_stack.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/tty/sysrq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 52bbd27..5b1e0ed 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -224,7 +224,7 @@ static void showacpu(void *dummy)
 
 	spin_lock_irqsave(&show_lock, flags);
 	pr_info("CPU%d:\n", smp_processor_id());
-	show_stack(NULL, NULL);
+	show_stack(current, NULL);
 	spin_unlock_irqrestore(&show_lock, flags);
 }
 
-- 
2.7.4

[PATCH 3/3] lib: dump_stack: explicitly pass current to show_stack

[Mark Rutland]

As noted in commit:

  81539169f283329f ("x86/dumpstack: Remove NULL task pointer convention")

... having a NULL task parameter imply current leads to subtle bugs in stack
walking code (so far seen on both 86 and arm64), makes callsites harder to
read, and is unnecessary as all callers have access to current.

As a step towards removing the problematic NULL-implies-current idiom entirely,
have the dump_stack code explicitly pass current to show_stack.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 lib/dump_stack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index c30d07e..7296b63 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -12,7 +12,7 @@
 static void __dump_stack(void)
 {
 	dump_stack_print_info(KERN_DEFAULT);
-	show_stack(NULL, NULL);
+	show_stack(current, NULL);
 }
 
 /**
-- 
2.7.4

Re: [PATCH 0/3] Kill off show_stack() NULL-implies-current idiom

[Josh Poimboeuf]

On Mon, Sep 26, 2016 at 04:16:16PM +0100, Mark Rutland wrote:
> Today, show_stack() accepts a NULL task parameter, which it takes to mean the
> current task. However, as noted in tip/x86/asm commit:
> 
>   81539169f283329f ("x86/dumpstack: Remove NULL task pointer convention")
> 
> ... having a NULL task parameter imply current leads to subtle bugs in stack
> walking code (so far seen on both 86 and arm64), makes callsites harder to
> read, and is unnecessary as all callers have access to current.
> 
> As a step towards removing the problematic NULL-implies-current idiom entirely,
> these patches ensure that generic code explictly passes current to
> show_stack(), rather than relying on arch code to handle NULL.

This is a good step, though it would be really nice to fix this
tree-wide.  Do you have any plans to do so?

Regardless, for the series:

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH 0/3] Kill off show_stack() NULL-implies-current idiom

[Mark Rutland]

On Mon, Sep 26, 2016 at 03:01:36PM -0500, Josh Poimboeuf wrote:
> On Mon, Sep 26, 2016 at 04:16:16PM +0100, Mark Rutland wrote:
> > Today, show_stack() accepts a NULL task parameter, which it takes to mean the
> > current task. However, as noted in tip/x86/asm commit:
> > 
> >   81539169f283329f ("x86/dumpstack: Remove NULL task pointer convention")
> > 
> > ... having a NULL task parameter imply current leads to subtle bugs in stack
> > walking code (so far seen on both 86 and arm64), makes callsites harder to
> > read, and is unnecessary as all callers have access to current.
> > 
> > As a step towards removing the problematic NULL-implies-current idiom entirely,
> > these patches ensure that generic code explictly passes current to
> > show_stack(), rather than relying on arch code to handle NULL.
> 
> This is a good step, though it would be really nice to fix this
> tree-wide.  Do you have any plans to do so?

FWIW, I completely agree, though I have no plans to do so currently.

I'd meant to Cc linux-arch for this series in order to make people aware,
though evidently I did not. I can Cc subsequent arm64 NULL-implies-current
cleanup patches there, if that's helpful?

> Regardless, for the series:
> 
> Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

Cheers!

Thanks,
Mark.

Re: [PATCH 0/3] Kill off show_stack() NULL-implies-current idiom

[Mark Rutland]

On Mon, Sep 26, 2016 at 03:01:36PM -0500, Josh Poimboeuf wrote:
> On Mon, Sep 26, 2016 at 04:16:16PM +0100, Mark Rutland wrote:
> > Today, show_stack() accepts a NULL task parameter, which it takes to mean the
> > current task. However, as noted in tip/x86/asm commit:
> > 
> >   81539169f283329f ("x86/dumpstack: Remove NULL task pointer convention")
> > 
> > ... having a NULL task parameter imply current leads to subtle bugs in stack
> > walking code (so far seen on both 86 and arm64), makes callsites harder to
> > read, and is unnecessary as all callers have access to current.
> > 
> > As a step towards removing the problematic NULL-implies-current idiom entirely,
> > these patches ensure that generic code explictly passes current to
> > show_stack(), rather than relying on arch code to handle NULL.
> 
> This is a good step, though it would be really nice to fix this
> tree-wide.  Do you have any plans to do so?

I started having a go, and in doing so I've realised that as things stand,
these patches are broken, as some architectures don't handle current
explicitly.

Please disregard all patches in this series.

Apologies,
Mark.