Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1482782 > unrolled thread

[PATCH 00/12] thread_info cleanups and stack caching

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/12] thread_info cleanups and stack caching Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:30 +0200
    [PATCH 03/12] um: Stop conflating task_struct::stack with thread_info Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:30 +0200
      Re: [PATCH 03/12] um: Stop conflating task_struct::stack with  thread_info Ingo Molnar <mingo@kernel.org> - 2016-09-15 08:30 +0200
      [tip:x86/asm] um/Stop conflating task_struct::stack with  thread_info tip-bot for Linus Torvalds <tipbot@zytor.com> - 2016-09-15 12:50 +0200
    [PATCH 12/12] fork: Cache two thread stacks per cpu if CONFIG_VMAP_STACK is set Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
    [PATCH 01/12] x86/asm: Move 'status' from struct thread_info to struct thread_struct Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
      [tip:x86/asm] x86/asm: Move the thread_info::status field to  thread_struct tip-bot for Andy Lutomirski <tipbot@zytor.com> - 2016-09-15 12:50 +0200
    [PATCH 10/12] lib/syscall: Pin the task stack in collect_syscall() Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
    [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk() Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
      Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-09-14 17:00 +0200
        Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk() Andy Lutomirski <luto@amacapital.net> - 2016-09-14 20:30 +0200
          Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-09-14 20:40 +0200
            Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk() Andy Lutomirski <luto@amacapital.net> - 2016-09-15 20:10 +0200
              Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-09-15 20:40 +0200
                Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk() Andy Lutomirski <luto@amacapital.net> - 2016-09-15 20:50 +0200
                  Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-09-15 21:20 +0200
                    Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Peter Zijlstra <peterz@infradead.org> - 2016-09-16 09:50 +0200
                      Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk() Andy Lutomirski <luto@amacapital.net> - 2016-09-16 17:20 +0200
                        Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk() Andy Lutomirski <luto@kernel.org> - 2016-09-16 17:40 +0200
                          Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Peter Zijlstra <peterz@infradead.org> - 2016-09-16 18:40 +0200
                        Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Peter Zijlstra <peterz@infradead.org> - 2016-09-16 17:40 +0200
      Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Ingo Molnar <mingo@kernel.org> - 2016-09-15 08:40 +0200
        Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in  save_stack_trace_tsk() Ingo Molnar <mingo@kernel.org> - 2016-09-15 11:30 +0200
    [PATCH 09/12] x86/process: Pin the target stack in get_wchan() Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
      Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan() Jann Horn <jann@thejh.net> - 2016-09-17 04:10 +0200
        Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2016-09-23 00:50 +0200
          Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2016-09-23 01:00 +0200
          Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan() Jann Horn <jann@thejh.net> - 2016-09-23 09:50 +0200
    [PATCH 06/12] sched: Add try_get_task_stack() and put_task_stack() Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
    [PATCH 02/12] x86/entry: Get rid of pt_regs_to_thread_info() Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
      Re: [PATCH 02/12] x86/entry: Get rid of pt_regs_to_thread_info() Ingo Molnar <mingo@kernel.org> - 2016-09-15 08:30 +0200
      [tip:x86/asm] x86/entry: Get rid of pt_regs_to_thread_info() tip-bot for Linus Torvalds <tipbot@zytor.com> - 2016-09-15 12:50 +0200
    [PATCH 05/12] x86: Move thread_info into task_struct Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
      [tip:x86/asm] x86: Move thread_info into task_struct tip-bot for Andy Lutomirski <tipbot@zytor.com> - 2016-09-15 12:50 +0200
    [PATCH 04/12] sched: Allow putting thread_info into task_struct Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
      [tip:x86/asm] sched/core: Allow putting thread_info into  task_struct tip-bot for Andy Lutomirski <tipbot@zytor.com> - 2016-09-15 12:50 +0200
    [PATCH 11/12] sched: Free the stack early if CONFIG_THREAD_INFO_IN_TASK Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200
    [PATCH 07/12] kthread: to_live_kthread() needs try_get_task_stack() Andy Lutomirski <luto@kernel.org> - 2016-09-13 23:40 +0200

Page 1 of 2  [1] 2  Next page →

#1482782 — [PATCH 00/12] thread_info cleanups and stack caching

[Sorry this is late.  I apparently never hit enter on the git
 send-email command.  This is what I meant to send, except that I
 folded in the collect_syscall() fix and redid the rebase (which
 was uneventful).]

This series extensively cleans up thread_info.  thread_info has been
partially redundant with thread_struct for a long time -- both are
places for arch code to add additional per-task variables.
thread_struct is much cleaner: it's always in task_struct, and
there's nothing particularly magical about it.  So this series moves
x86's status field from thread_info to thread_struct and to remove
x86's dependence on thread_info's position on the stack.  Then it
opts x86 into a new config option THREAD_INFO_IN_TASK to get rid of
arch-specific thread_info entirely and simply embed a defanged
thread_info (containing only flags) and 'int cpu' into task_struct.

Once thread_info stops being magical, there's another benefit: we
can free the thread stack as soon as the task is dead (without
waiting for RCU) and then, if vmapped stacks are in use, cache the
entire stack for reuse on the same cpu.

This seems to be an overall speedup of about 0.5-1 µs per
pthread_create/join compared to the old CONFIG_VMAP_STACK=n baseline
in a simple test -- a percpu cache of vmalloced stacks appears to be
a bit faster than a high-order stack allocation, at least when the
cache hits.  (I expect that workloads with a low cache hit rate are
likely to be dominated by other effects anyway.)

Changes from before:
 - A bunch of the series is already in 4.8-rc.
 - Added the get_wchan() and collect_syscall() patches.
 - Rebased.

Andy Lutomirski (9):
  x86/asm: Move 'status' from struct thread_info to struct thread_struct
  sched: Allow putting thread_info into task_struct
  x86: Move thread_info into task_struct
  sched: Add try_get_task_stack() and put_task_stack()
  x86/dumpstack: Pin the target stack in save_stack_trace_tsk()
  x86/process: Pin the target stack in get_wchan()
  lib/syscall: Pin the task stack in collect_syscall()
  sched: Free the stack early if CONFIG_THREAD_INFO_IN_TASK
  fork: Cache two thread stacks per cpu if CONFIG_VMAP_STACK is set

Linus Torvalds (2):
  x86/entry: Get rid of pt_regs_to_thread_info()
  um: Stop conflating task_struct::stack with thread_info

Oleg Nesterov (1):
  kthread: to_live_kthread() needs try_get_task_stack()

 arch/x86/Kconfig                   |  1 +
 arch/x86/entry/common.c            | 24 ++++------
 arch/x86/entry/entry_64.S          |  7 ++-
 arch/x86/include/asm/processor.h   | 12 +++++
 arch/x86/include/asm/syscall.h     | 20 ++------
 arch/x86/include/asm/thread_info.h | 69 ++-------------------------
 arch/x86/kernel/asm-offsets.c      |  5 +-
 arch/x86/kernel/fpu/init.c         |  1 -
 arch/x86/kernel/irq_64.c           |  3 +-
 arch/x86/kernel/process.c          | 28 ++++++-----
 arch/x86/kernel/process_64.c       |  4 +-
 arch/x86/kernel/ptrace.c           |  2 +-
 arch/x86/kernel/signal.c           |  2 +-
 arch/x86/kernel/stacktrace.c       |  5 ++
 arch/x86/um/ptrace_32.c            |  8 ++--
 include/linux/init_task.h          | 11 +++++
 include/linux/sched.h              | 66 +++++++++++++++++++++++++-
 include/linux/thread_info.h        | 15 ++++++
 init/Kconfig                       | 10 ++++
 init/init_task.c                   |  7 ++-
 kernel/fork.c                      | 97 ++++++++++++++++++++++++++++++++++----
 kernel/kthread.c                   |  8 +++-
 kernel/sched/core.c                |  4 ++
 kernel/sched/sched.h               |  4 ++
 lib/syscall.c                      | 15 +++++-
 25 files changed, 286 insertions(+), 142 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1482783 — [PATCH 03/12] um: Stop conflating task_struct::stack with thread_info

From: Linus Torvalds <torvalds@linux-foundation.org>

thread_info may move in the future, so use the accessors.

Andy Lutomirski wrote this changelog message and changed
"task_thread_info(child)->cpu" to "task_cpu(child)".

Message-Id: <CA+55aFxvZhBu9U1cqpVm4frv0p5mqu=0TxsSqE-=95ft8HvCVA@mail.gmail.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/um/ptrace_32.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/um/ptrace_32.c b/arch/x86/um/ptrace_32.c
index ebd4dd6ef73b..2a6f781ce9cf 100644
--- a/arch/x86/um/ptrace_32.c
+++ b/arch/x86/um/ptrace_32.c
@@ -191,7 +191,7 @@ int peek_user(struct task_struct *child, long addr, long data)
 
 static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int err, n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int err, n, cpu = task_cpu(child);
 	struct user_i387_struct fpregs;
 
 	err = save_i387_registers(userspace_pid[cpu],
@@ -208,7 +208,7 @@ static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 
 static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int n, cpu = task_cpu(child);
 	struct user_i387_struct fpregs;
 
 	n = copy_from_user(&fpregs, buf, sizeof(fpregs));
@@ -221,7 +221,7 @@ static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 
 static int get_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *child)
 {
-	int err, n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int err, n, cpu = task_cpu(child);
 	struct user_fxsr_struct fpregs;
 
 	err = save_fpx_registers(userspace_pid[cpu], (unsigned long *) &fpregs);
@@ -237,7 +237,7 @@ static int get_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *
 
 static int set_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *child)
 {
-	int n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int n, cpu = task_cpu(child);
 	struct user_fxsr_struct fpregs;
 
 	n = copy_from_user(&fpregs, buf, sizeof(fpregs));
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1483865 — Re: [PATCH 03/12] um: Stop conflating task_struct::stack with thread_info

* Andy Lutomirski <luto@kernel.org> wrote:

> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> thread_info may move in the future, so use the accessors.
> 
> Andy Lutomirski wrote this changelog message and changed
> "task_thread_info(child)->cpu" to "task_cpu(child)".
> 
> Message-Id: <CA+55aFxvZhBu9U1cqpVm4frv0p5mqu=0TxsSqE-=95ft8HvCVA@mail.gmail.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

Like the previous patch I'm adding Linus's primary author SOB:

  Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1483971 — [tip:x86/asm] um/Stop conflating task_struct::stack with thread_info

Commit-ID:  d896fa20a70c9e596438728561e058a74ed3196b
Gitweb:     http://git.kernel.org/tip/d896fa20a70c9e596438728561e058a74ed3196b
Author:     Linus Torvalds <torvalds@linux-foundation.org>
AuthorDate: Tue, 13 Sep 2016 14:29:23 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 15 Sep 2016 08:25:12 +0200

um/Stop conflating task_struct::stack with thread_info

thread_info may move in the future, so use the accessors.

[ Andy Lutomirski wrote this changelog message and changed
  "task_thread_info(child)->cpu" to "task_cpu(child)". ]

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jann Horn <jann@thejh.net>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/3439705d9838940cc82733a7335fa8c654c37db8.1473801993.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/um/ptrace_32.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/um/ptrace_32.c b/arch/x86/um/ptrace_32.c
index a7ef7b1..5766ead 100644
--- a/arch/x86/um/ptrace_32.c
+++ b/arch/x86/um/ptrace_32.c
@@ -194,7 +194,7 @@ int peek_user(struct task_struct *child, long addr, long data)
 
 static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int err, n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int err, n, cpu = task_cpu(child);
 	struct user_i387_struct fpregs;
 
 	err = save_i387_registers(userspace_pid[cpu],
@@ -211,7 +211,7 @@ static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 
 static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int n, cpu = task_cpu(child);
 	struct user_i387_struct fpregs;
 
 	n = copy_from_user(&fpregs, buf, sizeof(fpregs));
@@ -224,7 +224,7 @@ static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 
 static int get_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *child)
 {
-	int err, n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int err, n, cpu = task_cpu(child);
 	struct user_fxsr_struct fpregs;
 
 	err = save_fpx_registers(userspace_pid[cpu], (unsigned long *) &fpregs);
@@ -240,7 +240,7 @@ static int get_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *
 
 static int set_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *child)
 {
-	int n, cpu = ((struct thread_info *) child->stack)->cpu;
+	int n, cpu = task_cpu(child);
 	struct user_fxsr_struct fpregs;
 
 	n = copy_from_user(&fpregs, buf, sizeof(fpregs));

[toc] | [prev] | [next] | [standalone]

#1482786 — [PATCH 12/12] fork: Cache two thread stacks per cpu if CONFIG_VMAP_STACK is set

vmalloc is a bit slow, and pounding vmalloc/vfree will eventually
force a global TLB flush.

To reduce pressure on them, if CONFIG_VMAP_STACK, cache two thread
stacks per cpu.  This will let us quickly allocate a hopefully
cache-hot, TLB-hot stack under heavy forking workloads (shell script
style).

On my silly pthread_create benchmark, it saves about 2 µs per
pthread_create+join with CONFIG_VMAP_STACK=y.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 kernel/fork.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 9 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index e0fd5446df58..128ef350ce53 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -159,15 +159,41 @@ void __weak arch_release_thread_stack(unsigned long *stack)
  * kmemcache based allocator.
  */
 # if THREAD_SIZE >= PAGE_SIZE || defined(CONFIG_VMAP_STACK)
+
+#ifdef CONFIG_VMAP_STACK
+/*
+ * vmalloc is a bit slow, and calling vfree enough times will force a TLB
+ * flush.  Try to minimize the number of calls by caching stacks.
+ */
+#define NR_CACHED_STACKS 2
+static DEFINE_PER_CPU(struct vm_struct *, cached_stacks[NR_CACHED_STACKS]);
+#endif
+
 static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
 {
 #ifdef CONFIG_VMAP_STACK
-	void *stack = __vmalloc_node_range(THREAD_SIZE, THREAD_SIZE,
-					   VMALLOC_START, VMALLOC_END,
-					   THREADINFO_GFP | __GFP_HIGHMEM,
-					   PAGE_KERNEL,
-					   0, node,
-					   __builtin_return_address(0));
+	void *stack;
+	int i;
+
+	local_irq_disable();
+	for (i = 0; i < NR_CACHED_STACKS; i++) {
+		struct vm_struct *s = this_cpu_read(cached_stacks[i]);
+
+		if (!s)
+			continue;
+		this_cpu_write(cached_stacks[i], NULL);
+
+		tsk->stack_vm_area = s;
+		local_irq_enable();
+		return s->addr;
+	}
+	local_irq_enable();
+
+	stack = __vmalloc_node_range(THREAD_SIZE, THREAD_SIZE,
+				     VMALLOC_START, VMALLOC_END,
+				     THREADINFO_GFP | __GFP_HIGHMEM,
+				     PAGE_KERNEL,
+				     0, node, __builtin_return_address(0));
 
 	/*
 	 * We can't call find_vm_area() in interrupt context, and
@@ -187,10 +213,28 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
 
 static inline void free_thread_stack(struct task_struct *tsk)
 {
-	if (task_stack_vm_area(tsk))
+#ifdef CONFIG_VMAP_STACK
+	if (task_stack_vm_area(tsk)) {
+		unsigned long flags;
+		int i;
+
+		local_irq_save(flags);
+		for (i = 0; i < NR_CACHED_STACKS; i++) {
+			if (this_cpu_read(cached_stacks[i]))
+				continue;
+
+			this_cpu_write(cached_stacks[i], tsk->stack_vm_area);
+			local_irq_restore(flags);
+			return;
+		}
+		local_irq_restore(flags);
+
 		vfree(tsk->stack);
-	else
-		__free_pages(virt_to_page(tsk->stack), THREAD_SIZE_ORDER);
+		return;
+	}
+#endif
+
+	__free_pages(virt_to_page(tsk->stack), THREAD_SIZE_ORDER);
 }
 # else
 static struct kmem_cache *thread_stack_cache;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1482787 — [PATCH 01/12] x86/asm: Move 'status' from struct thread_info to struct thread_struct

Becuase sched.h and thread_info.h are a tangled mess, I turned
in_compat_syscall into a macro.  If we had current_thread_struct()
or similar and we could use it from thread_info.h, then this would
be a bit cleaner.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/entry/common.c            |  4 ++--
 arch/x86/include/asm/processor.h   | 12 ++++++++++++
 arch/x86/include/asm/syscall.h     | 20 +++++---------------
 arch/x86/include/asm/thread_info.h | 23 ++++-------------------
 arch/x86/kernel/asm-offsets.c      |  1 -
 arch/x86/kernel/fpu/init.c         |  1 -
 arch/x86/kernel/process_64.c       |  4 ++--
 arch/x86/kernel/ptrace.c           |  2 +-
 arch/x86/kernel/signal.c           |  2 +-
 9 files changed, 27 insertions(+), 42 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 1433f6b4607d..871bbf975d4c 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -209,7 +209,7 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 	 * special case only applies after poking regs and before the
 	 * very next return to user mode.
 	 */
-	ti->status &= ~(TS_COMPAT|TS_I386_REGS_POKED);
+	current->thread.status &= ~(TS_COMPAT|TS_I386_REGS_POKED);
 #endif
 
 	user_enter_irqoff();
@@ -307,7 +307,7 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 	unsigned int nr = (unsigned int)regs->orig_ax;
 
 #ifdef CONFIG_IA32_EMULATION
-	ti->status |= TS_COMPAT;
+	current->thread.status |= TS_COMPAT;
 #endif
 
 	if (READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY) {
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index b22fb5a4ff3c..984a7bf17f6a 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -389,6 +389,9 @@ struct thread_struct {
 	unsigned short		fsindex;
 	unsigned short		gsindex;
 #endif
+
+	u32			status;		/* thread synchronous flags */
+
 #ifdef CONFIG_X86_64
 	unsigned long		fsbase;
 	unsigned long		gsbase;
@@ -435,6 +438,15 @@ struct thread_struct {
 };
 
 /*
+ * Thread-synchronous status.
+ *
+ * This is different from the flags in that nobody else
+ * ever touches our thread-synchronous status, so we don't
+ * have to worry about atomic accesses.
+ */
+#define TS_COMPAT		0x0002	/* 32bit syscall active (64BIT)*/
+
+/*
  * Set IOPL bits in EFLAGS from given mask
  */
 static inline void native_set_iopl_mask(unsigned mask)
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 4e23dd15c661..e3c95e8e61c5 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -60,7 +60,7 @@ static inline long syscall_get_error(struct task_struct *task,
 	 * TS_COMPAT is set for 32-bit syscall entries and then
 	 * remains set until we return to user mode.
 	 */
-	if (task_thread_info(task)->status & (TS_COMPAT|TS_I386_REGS_POKED))
+	if (task->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
 		/*
 		 * Sign-extend the value so (int)-EFOO becomes (long)-EFOO
 		 * and will match correctly in comparisons.
@@ -116,7 +116,7 @@ static inline void syscall_get_arguments(struct task_struct *task,
 					 unsigned long *args)
 {
 # ifdef CONFIG_IA32_EMULATION
-	if (task_thread_info(task)->status & TS_COMPAT)
+	if (task->thread.status & TS_COMPAT)
 		switch (i) {
 		case 0:
 			if (!n--) break;
@@ -177,7 +177,7 @@ static inline void syscall_set_arguments(struct task_struct *task,
 					 const unsigned long *args)
 {
 # ifdef CONFIG_IA32_EMULATION
-	if (task_thread_info(task)->status & TS_COMPAT)
+	if (task->thread.status & TS_COMPAT)
 		switch (i) {
 		case 0:
 			if (!n--) break;
@@ -234,18 +234,8 @@ static inline void syscall_set_arguments(struct task_struct *task,
 
 static inline int syscall_get_arch(void)
 {
-#ifdef CONFIG_IA32_EMULATION
-	/*
-	 * TS_COMPAT is set for 32-bit syscall entry and then
-	 * remains set until we return to user mode.
-	 *
-	 * x32 tasks should be considered AUDIT_ARCH_X86_64.
-	 */
-	if (task_thread_info(current)->status & TS_COMPAT)
-		return AUDIT_ARCH_I386;
-#endif
-	/* Both x32 and x86_64 are considered "64-bit". */
-	return AUDIT_ARCH_X86_64;
+	/* x32 tasks should be considered AUDIT_ARCH_X86_64. */
+	return in_ia32_syscall() ? AUDIT_ARCH_I386 : AUDIT_ARCH_X86_64;
 }
 #endif	/* CONFIG_X86_32 */
 
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 494c4b5ada34..c9dcfe7c7e4b 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -55,7 +55,6 @@ struct task_struct;
 struct thread_info {
 	struct task_struct	*task;		/* main task structure */
 	__u32			flags;		/* low level flags */
-	__u32			status;		/* thread synchronous flags */
 	__u32			cpu;		/* current CPU */
 };
 
@@ -253,31 +252,17 @@ static inline int arch_within_stack_frames(const void * const stack,
 
 #endif
 
-/*
- * Thread-synchronous status.
- *
- * This is different from the flags in that nobody else
- * ever touches our thread-synchronous status, so we don't
- * have to worry about atomic accesses.
- */
-#define TS_COMPAT		0x0002	/* 32bit syscall active (64BIT)*/
 #ifdef CONFIG_COMPAT
 #define TS_I386_REGS_POKED	0x0004	/* regs poked by 32-bit ptracer */
 #endif
-
 #ifndef __ASSEMBLY__
 
-static inline bool in_ia32_syscall(void)
-{
 #ifdef CONFIG_X86_32
-	return true;
-#endif
-#ifdef CONFIG_IA32_EMULATION
-	if (current_thread_info()->status & TS_COMPAT)
-		return true;
+#define in_ia32_syscall() true
+#else
+#define in_ia32_syscall() (IS_ENABLED(CONFIG_IA32_EMULATION) && \
+			   current->thread.status & TS_COMPAT)
 #endif
-	return false;
-}
 
 /*
  * Force syscall return via IRET by making it look as if there was
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index db3a0af9b9ec..add5f90b93d4 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -36,7 +36,6 @@ void common(void) {
 
 	BLANK();
 	OFFSET(TI_flags, thread_info, flags);
-	OFFSET(TI_status, thread_info, status);
 
 	BLANK();
 	OFFSET(TASK_addr_limit, task_struct, thread.addr_limit);
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 93982aebb398..2f2b8c7ccb85 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -317,7 +317,6 @@ static void __init fpu__init_system_ctx_switch(void)
 	on_boot_cpu = 0;
 
 	WARN_ON_FPU(current->thread.fpu.fpstate_active);
-	current_thread_info()->status = 0;
 
 	if (boot_cpu_has(X86_FEATURE_XSAVEOPT) && eagerfpu != DISABLE)
 		eagerfpu = ENABLE;
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index b812cd0d7889..de9acaf2d371 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -510,7 +510,7 @@ void set_personality_ia32(bool x32)
 		current->personality &= ~READ_IMPLIES_EXEC;
 		/* in_compat_syscall() uses the presence of the x32
 		   syscall bit flag to determine compat status */
-		current_thread_info()->status &= ~TS_COMPAT;
+		current->thread.status &= ~TS_COMPAT;
 	} else {
 		set_thread_flag(TIF_IA32);
 		clear_thread_flag(TIF_X32);
@@ -518,7 +518,7 @@ void set_personality_ia32(bool x32)
 			current->mm->context.ia32_compat = TIF_IA32;
 		current->personality |= force_personality32;
 		/* Prepare the first "return" to user space */
-		current_thread_info()->status |= TS_COMPAT;
+		current->thread.status |= TS_COMPAT;
 	}
 }
 EXPORT_SYMBOL_GPL(set_personality_ia32);
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 5b88a1b26fc7..ce94c38cf4d6 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -934,7 +934,7 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
 		 */
 		regs->orig_ax = value;
 		if (syscall_get_nr(child, regs) >= 0)
-			task_thread_info(child)->status |= TS_I386_REGS_POKED;
+			child->thread.status |= TS_I386_REGS_POKED;
 		break;
 
 	case offsetof(struct user32, regs.eflags):
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 04cb3212db2d..da20ecb5397a 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -783,7 +783,7 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 	 * than the tracee.
 	 */
 #ifdef CONFIG_IA32_EMULATION
-	if (current_thread_info()->status & (TS_COMPAT|TS_I386_REGS_POKED))
+	if (current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
 		return __NR_ia32_restart_syscall;
 #endif
 #ifdef CONFIG_X86_X32_ABI
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1483980 — [tip:x86/asm] x86/asm: Move the thread_info::status field to thread_struct

Commit-ID:  b9d989c7218ac922185d82ad46f3e58b27a4bea9
Gitweb:     http://git.kernel.org/tip/b9d989c7218ac922185d82ad46f3e58b27a4bea9
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Tue, 13 Sep 2016 14:29:21 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 15 Sep 2016 08:25:12 +0200

x86/asm: Move the thread_info::status field to thread_struct

Because sched.h and thread_info.h are a tangled mess, I turned
in_compat_syscall() into a macro.  If we had current_thread_struct()
or similar and we could use it from thread_info.h, then this would
be a bit cleaner.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jann Horn <jann@thejh.net>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/ccc8a1b2f41f9c264a41f771bb4a6539a642ad72.1473801993.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/common.c            |  4 ++--
 arch/x86/include/asm/processor.h   | 12 ++++++++++++
 arch/x86/include/asm/syscall.h     | 20 +++++---------------
 arch/x86/include/asm/thread_info.h | 23 ++++-------------------
 arch/x86/kernel/asm-offsets.c      |  1 -
 arch/x86/kernel/fpu/init.c         |  1 -
 arch/x86/kernel/process_64.c       |  4 ++--
 arch/x86/kernel/ptrace.c           |  2 +-
 arch/x86/kernel/signal.c           |  2 +-
 9 files changed, 27 insertions(+), 42 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 1433f6b..871bbf9 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -209,7 +209,7 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 	 * special case only applies after poking regs and before the
 	 * very next return to user mode.
 	 */
-	ti->status &= ~(TS_COMPAT|TS_I386_REGS_POKED);
+	current->thread.status &= ~(TS_COMPAT|TS_I386_REGS_POKED);
 #endif
 
 	user_enter_irqoff();
@@ -307,7 +307,7 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 	unsigned int nr = (unsigned int)regs->orig_ax;
 
 #ifdef CONFIG_IA32_EMULATION
-	ti->status |= TS_COMPAT;
+	current->thread.status |= TS_COMPAT;
 #endif
 
 	if (READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY) {
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index b22fb5a..984a7bf 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -389,6 +389,9 @@ struct thread_struct {
 	unsigned short		fsindex;
 	unsigned short		gsindex;
 #endif
+
+	u32			status;		/* thread synchronous flags */
+
 #ifdef CONFIG_X86_64
 	unsigned long		fsbase;
 	unsigned long		gsbase;
@@ -435,6 +438,15 @@ struct thread_struct {
 };
 
 /*
+ * Thread-synchronous status.
+ *
+ * This is different from the flags in that nobody else
+ * ever touches our thread-synchronous status, so we don't
+ * have to worry about atomic accesses.
+ */
+#define TS_COMPAT		0x0002	/* 32bit syscall active (64BIT)*/
+
+/*
  * Set IOPL bits in EFLAGS from given mask
  */
 static inline void native_set_iopl_mask(unsigned mask)
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 4e23dd1..e3c95e8 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -60,7 +60,7 @@ static inline long syscall_get_error(struct task_struct *task,
 	 * TS_COMPAT is set for 32-bit syscall entries and then
 	 * remains set until we return to user mode.
 	 */
-	if (task_thread_info(task)->status & (TS_COMPAT|TS_I386_REGS_POKED))
+	if (task->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
 		/*
 		 * Sign-extend the value so (int)-EFOO becomes (long)-EFOO
 		 * and will match correctly in comparisons.
@@ -116,7 +116,7 @@ static inline void syscall_get_arguments(struct task_struct *task,
 					 unsigned long *args)
 {
 # ifdef CONFIG_IA32_EMULATION
-	if (task_thread_info(task)->status & TS_COMPAT)
+	if (task->thread.status & TS_COMPAT)
 		switch (i) {
 		case 0:
 			if (!n--) break;
@@ -177,7 +177,7 @@ static inline void syscall_set_arguments(struct task_struct *task,
 					 const unsigned long *args)
 {
 # ifdef CONFIG_IA32_EMULATION
-	if (task_thread_info(task)->status & TS_COMPAT)
+	if (task->thread.status & TS_COMPAT)
 		switch (i) {
 		case 0:
 			if (!n--) break;
@@ -234,18 +234,8 @@ static inline void syscall_set_arguments(struct task_struct *task,
 
 static inline int syscall_get_arch(void)
 {
-#ifdef CONFIG_IA32_EMULATION
-	/*
-	 * TS_COMPAT is set for 32-bit syscall entry and then
-	 * remains set until we return to user mode.
-	 *
-	 * x32 tasks should be considered AUDIT_ARCH_X86_64.
-	 */
-	if (task_thread_info(current)->status & TS_COMPAT)
-		return AUDIT_ARCH_I386;
-#endif
-	/* Both x32 and x86_64 are considered "64-bit". */
-	return AUDIT_ARCH_X86_64;
+	/* x32 tasks should be considered AUDIT_ARCH_X86_64. */
+	return in_ia32_syscall() ? AUDIT_ARCH_I386 : AUDIT_ARCH_X86_64;
 }
 #endif	/* CONFIG_X86_32 */
 
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 494c4b5..c9dcfe7 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -55,7 +55,6 @@ struct task_struct;
 struct thread_info {
 	struct task_struct	*task;		/* main task structure */
 	__u32			flags;		/* low level flags */
-	__u32			status;		/* thread synchronous flags */
 	__u32			cpu;		/* current CPU */
 };
 
@@ -253,31 +252,17 @@ static inline int arch_within_stack_frames(const void * const stack,
 
 #endif
 
-/*
- * Thread-synchronous status.
- *
- * This is different from the flags in that nobody else
- * ever touches our thread-synchronous status, so we don't
- * have to worry about atomic accesses.
- */
-#define TS_COMPAT		0x0002	/* 32bit syscall active (64BIT)*/
 #ifdef CONFIG_COMPAT
 #define TS_I386_REGS_POKED	0x0004	/* regs poked by 32-bit ptracer */
 #endif
-
 #ifndef __ASSEMBLY__
 
-static inline bool in_ia32_syscall(void)
-{
 #ifdef CONFIG_X86_32
-	return true;
-#endif
-#ifdef CONFIG_IA32_EMULATION
-	if (current_thread_info()->status & TS_COMPAT)
-		return true;
+#define in_ia32_syscall() true
+#else
+#define in_ia32_syscall() (IS_ENABLED(CONFIG_IA32_EMULATION) && \
+			   current->thread.status & TS_COMPAT)
 #endif
-	return false;
-}
 
 /*
  * Force syscall return via IRET by making it look as if there was
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index db3a0af..add5f90 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -36,7 +36,6 @@ void common(void) {
 
 	BLANK();
 	OFFSET(TI_flags, thread_info, flags);
-	OFFSET(TI_status, thread_info, status);
 
 	BLANK();
 	OFFSET(TASK_addr_limit, task_struct, thread.addr_limit);
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 93982ae..2f2b8c7 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -317,7 +317,6 @@ static void __init fpu__init_system_ctx_switch(void)
 	on_boot_cpu = 0;
 
 	WARN_ON_FPU(current->thread.fpu.fpstate_active);
-	current_thread_info()->status = 0;
 
 	if (boot_cpu_has(X86_FEATURE_XSAVEOPT) && eagerfpu != DISABLE)
 		eagerfpu = ENABLE;
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index b812cd0..de9acaf 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -510,7 +510,7 @@ void set_personality_ia32(bool x32)
 		current->personality &= ~READ_IMPLIES_EXEC;
 		/* in_compat_syscall() uses the presence of the x32
 		   syscall bit flag to determine compat status */
-		current_thread_info()->status &= ~TS_COMPAT;
+		current->thread.status &= ~TS_COMPAT;
 	} else {
 		set_thread_flag(TIF_IA32);
 		clear_thread_flag(TIF_X32);
@@ -518,7 +518,7 @@ void set_personality_ia32(bool x32)
 			current->mm->context.ia32_compat = TIF_IA32;
 		current->personality |= force_personality32;
 		/* Prepare the first "return" to user space */
-		current_thread_info()->status |= TS_COMPAT;
+		current->thread.status |= TS_COMPAT;
 	}
 }
 EXPORT_SYMBOL_GPL(set_personality_ia32);
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 5b88a1b..ce94c38 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -934,7 +934,7 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
 		 */
 		regs->orig_ax = value;
 		if (syscall_get_nr(child, regs) >= 0)
-			task_thread_info(child)->status |= TS_I386_REGS_POKED;
+			child->thread.status |= TS_I386_REGS_POKED;
 		break;
 
 	case offsetof(struct user32, regs.eflags):
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 04cb321..da20ecb 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -783,7 +783,7 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 	 * than the tracee.
 	 */
 #ifdef CONFIG_IA32_EMULATION
-	if (current_thread_info()->status & (TS_COMPAT|TS_I386_REGS_POKED))
+	if (current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
 		return __NR_ia32_restart_syscall;
 #endif
 #ifdef CONFIG_X86_X32_ABI

[toc] | [prev] | [next] | [standalone]

#1482788 — [PATCH 10/12] lib/syscall: Pin the task stack in collect_syscall()

This will avoid a potential read-after-free if collect_syscall()
(e.g. /proc/PID/syscall) is called on an exiting task.

Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 lib/syscall.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/syscall.c b/lib/syscall.c
index e30e03932480..63239e097b13 100644
--- a/lib/syscall.c
+++ b/lib/syscall.c
@@ -7,9 +7,19 @@ static int collect_syscall(struct task_struct *target, long *callno,
 			   unsigned long args[6], unsigned int maxargs,
 			   unsigned long *sp, unsigned long *pc)
 {
-	struct pt_regs *regs = task_pt_regs(target);
-	if (unlikely(!regs))
+	struct pt_regs *regs;
+
+	if (!try_get_task_stack(target)) {
+		/* Task has no stack, so the task isn't in a syscall. */
+		*callno = -1;
+		return 0;
+	}
+
+	regs = task_pt_regs(target);
+	if (unlikely(!regs)) {
+		put_task_stack(target);
 		return -EAGAIN;
+	}
 
 	*sp = user_stack_pointer(regs);
 	*pc = instruction_pointer(regs);
@@ -18,6 +28,7 @@ static int collect_syscall(struct task_struct *target, long *callno,
 	if (*callno != -1L && maxargs > 0)
 		syscall_get_arguments(target, regs, 0, maxargs, args);
 
+	put_task_stack(target);
 	return 0;
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1482789 — [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

This will prevent a crash if the target task dies before or while
dumping its stack once we start freeing task stacks early.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/stacktrace.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 4738f5e0f2ab..b3f32fbe3ba4 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -79,9 +79,14 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 
 void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 {
+	if (!try_get_task_stack(tsk))
+		return;
+
 	dump_trace(tsk, NULL, NULL, 0, &save_stack_ops_nosched, trace);
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
+
+	put_task_stack(tsk);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1483414 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Tue, Sep 13, 2016 at 02:29:28PM -0700, Andy Lutomirski wrote:
> This will prevent a crash if the target task dies before or while
> dumping its stack once we start freeing task stacks early.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

Do we need a similar patch for show_stack()?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1483529 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Wed, Sep 14, 2016 at 7:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Tue, Sep 13, 2016 at 02:29:28PM -0700, Andy Lutomirski wrote:
>> This will prevent a crash if the target task dies before or while
>> dumping its stack once we start freeing task stacks early.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>
> Do we need a similar patch for show_stack()?

Probably.  Shouldn't it go in show_stack_log_lvl() instead, though?

[toc] | [prev] | [next] | [standalone]

#1483533 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Wed, Sep 14, 2016 at 11:22:00AM -0700, Andy Lutomirski wrote:
> On Wed, Sep 14, 2016 at 7:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Tue, Sep 13, 2016 at 02:29:28PM -0700, Andy Lutomirski wrote:
> >> This will prevent a crash if the target task dies before or while
> >> dumping its stack once we start freeing task stacks early.
> >>
> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >
> > Do we need a similar patch for show_stack()?
> 
> Probably.  Shouldn't it go in show_stack_log_lvl() instead, though?

Yeah, that would probably be better.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1484446 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Wed, Sep 14, 2016 at 11:35 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Wed, Sep 14, 2016 at 11:22:00AM -0700, Andy Lutomirski wrote:
>> On Wed, Sep 14, 2016 at 7:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Tue, Sep 13, 2016 at 02:29:28PM -0700, Andy Lutomirski wrote:
>> >> This will prevent a crash if the target task dies before or while
>> >> dumping its stack once we start freeing task stacks early.
>> >>
>> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> >
>> > Do we need a similar patch for show_stack()?
>>
>> Probably.  Shouldn't it go in show_stack_log_lvl() instead, though?
>
> Yeah, that would probably be better.

This code is a colossal mess.  I really hope that, some day, we can
clarify which entry points are used only in dumpstack*.c and which are
used elsewhere.  Creating an arch/x86/kernel/dumpstack.h or just
merging the three files and removing all the intermediate crap from
the headers could help a lot.

[toc] | [prev] | [next] | [standalone]

#1484453 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Thu, Sep 15, 2016 at 11:04:47AM -0700, Andy Lutomirski wrote:
> On Wed, Sep 14, 2016 at 11:35 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Wed, Sep 14, 2016 at 11:22:00AM -0700, Andy Lutomirski wrote:
> >> On Wed, Sep 14, 2016 at 7:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > On Tue, Sep 13, 2016 at 02:29:28PM -0700, Andy Lutomirski wrote:
> >> >> This will prevent a crash if the target task dies before or while
> >> >> dumping its stack once we start freeing task stacks early.
> >> >>
> >> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >> >
> >> > Do we need a similar patch for show_stack()?
> >>
> >> Probably.  Shouldn't it go in show_stack_log_lvl() instead, though?
> >
> > Yeah, that would probably be better.
> 
> This code is a colossal mess.  I really hope that, some day, we can
> clarify which entry points are used only in dumpstack*.c and which are
> used elsewhere.  Creating an arch/x86/kernel/dumpstack.h or just
> merging the three files and removing all the intermediate crap from
> the headers could help a lot.

Agreed, it's a mess, though it's improving with some of my changes.
dumpstack_32.c and dumpstack_64.c will be shrinking, with dump_trace()
getting removed.  And they'll be shrinking even more with Linus's
suggestion to remove show_stack_log_lvl().  Then maybe we can look at
merging those two files into dumpstack.c with an #ifdef to separate the
subarch differences.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1484460 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Thu, Sep 15, 2016 at 11:37 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Sep 15, 2016 at 11:04:47AM -0700, Andy Lutomirski wrote:
>> On Wed, Sep 14, 2016 at 11:35 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Wed, Sep 14, 2016 at 11:22:00AM -0700, Andy Lutomirski wrote:
>> >> On Wed, Sep 14, 2016 at 7:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> >> > On Tue, Sep 13, 2016 at 02:29:28PM -0700, Andy Lutomirski wrote:
>> >> >> This will prevent a crash if the target task dies before or while
>> >> >> dumping its stack once we start freeing task stacks early.
>> >> >>
>> >> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> >> >
>> >> > Do we need a similar patch for show_stack()?
>> >>
>> >> Probably.  Shouldn't it go in show_stack_log_lvl() instead, though?
>> >
>> > Yeah, that would probably be better.
>>
>> This code is a colossal mess.  I really hope that, some day, we can
>> clarify which entry points are used only in dumpstack*.c and which are
>> used elsewhere.  Creating an arch/x86/kernel/dumpstack.h or just
>> merging the three files and removing all the intermediate crap from
>> the headers could help a lot.
>
> Agreed, it's a mess, though it's improving with some of my changes.
> dumpstack_32.c and dumpstack_64.c will be shrinking, with dump_trace()
> getting removed.  And they'll be shrinking even more with Linus's
> suggestion to remove show_stack_log_lvl().  Then maybe we can look at
> merging those two files into dumpstack.c with an #ifdef to separate the
> subarch differences.
>

I also wouldn't mind trying to do something to prevent ever dumping
the stack of an actively running task.  It's definitely safe to dump:

 - current

 - any task that's stopped via ptrace, etc

 - any task on the current CPU if running atomically enough that the
task can't migrate (which probably covers the nasty NMI cases, I hope)

What's *not* safe AFAIK is /proc/PID/stack.  I don't know if we can
somehow fix that short of actually sending an interrupt or NMI to
freeze the task if it's running.  I'm also not sure it's worth
worrying about it.

--Andy

[toc] | [prev] | [next] | [standalone]

#1484481 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Thu, Sep 15, 2016 at 11:41:25AM -0700, Andy Lutomirski wrote:
> I also wouldn't mind trying to do something to prevent ever dumping
> the stack of an actively running task.  It's definitely safe to dump:
> 
>  - current
> 
>  - any task that's stopped via ptrace, etc
> 
>  - any task on the current CPU if running atomically enough that the
> task can't migrate (which probably covers the nasty NMI cases, I hope)
> 
> What's *not* safe AFAIK is /proc/PID/stack.  I don't know if we can
> somehow fix that short of actually sending an interrupt or NMI to
> freeze the task if it's running.  I'm also not sure it's worth
> worrying about it.

Yeah, I proposed a fix for /proc/PID/stack a while back:

  https://lkml.kernel.org/r/cover.1424109806.git.jpoimboe@redhat.com

My idea was to use task_rq_lock() to lock the runqueue and then check
tsk->on_cpu.  I think Peter wasn't too keen on it.

We'll need something similar for the livepatch consistency model.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1484747 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Thu, Sep 15, 2016 at 02:19:38PM -0500, Josh Poimboeuf wrote:
> On Thu, Sep 15, 2016 at 11:41:25AM -0700, Andy Lutomirski wrote:
> > I also wouldn't mind trying to do something to prevent ever dumping
> > the stack of an actively running task.  It's definitely safe to dump:
> > 
> >  - current
> > 
> >  - any task that's stopped via ptrace, etc
> > 
> >  - any task on the current CPU if running atomically enough that the
> > task can't migrate (which probably covers the nasty NMI cases, I hope)
> > 
> > What's *not* safe AFAIK is /proc/PID/stack.  I don't know if we can
> > somehow fix that short of actually sending an interrupt or NMI to
> > freeze the task if it's running.  I'm also not sure it's worth
> > worrying about it.
> 
> Yeah, I proposed a fix for /proc/PID/stack a while back:
> 
>   https://lkml.kernel.org/r/cover.1424109806.git.jpoimboe@redhat.com
> 
> My idea was to use task_rq_lock() to lock the runqueue and then check
> tsk->on_cpu.  I think Peter wasn't too keen on it.

That basically allows a DoS on the scheduler, since a user can run tasks
on every cpu (through sys_sched_setaffinity()). Then doing while (1) cat
/proc/$PID/stack would saturate the rq->lock on every CPU.

The more tasks the merrier.

[toc] | [prev] | [next] | [standalone]

#1485116 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Fri, Sep 16, 2016 at 12:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Sep 15, 2016 at 02:19:38PM -0500, Josh Poimboeuf wrote:
>> On Thu, Sep 15, 2016 at 11:41:25AM -0700, Andy Lutomirski wrote:
>> > I also wouldn't mind trying to do something to prevent ever dumping
>> > the stack of an actively running task.  It's definitely safe to dump:
>> >
>> >  - current
>> >
>> >  - any task that's stopped via ptrace, etc
>> >
>> >  - any task on the current CPU if running atomically enough that the
>> > task can't migrate (which probably covers the nasty NMI cases, I hope)
>> >
>> > What's *not* safe AFAIK is /proc/PID/stack.  I don't know if we can
>> > somehow fix that short of actually sending an interrupt or NMI to
>> > freeze the task if it's running.  I'm also not sure it's worth
>> > worrying about it.
>>
>> Yeah, I proposed a fix for /proc/PID/stack a while back:
>>
>>   https://lkml.kernel.org/r/cover.1424109806.git.jpoimboe@redhat.com
>>
>> My idea was to use task_rq_lock() to lock the runqueue and then check
>> tsk->on_cpu.  I think Peter wasn't too keen on it.
>
> That basically allows a DoS on the scheduler, since a user can run tasks
> on every cpu (through sys_sched_setaffinity()). Then doing while (1) cat
> /proc/$PID/stack would saturate the rq->lock on every CPU.
>
> The more tasks the merrier.
>
>

Is this worse than it would be if this code used preempt_disable()
(which I think it did until very recently)?

-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1485125 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Fri, Sep 16, 2016 at 8:31 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Sep 16, 2016 at 08:12:40AM -0700, Andy Lutomirski wrote:
>> On Fri, Sep 16, 2016 at 12:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Thu, Sep 15, 2016 at 02:19:38PM -0500, Josh Poimboeuf wrote:
>
>> >> My idea was to use task_rq_lock() to lock the runqueue and then check
>> >> tsk->on_cpu.  I think Peter wasn't too keen on it.
>> >
>> > That basically allows a DoS on the scheduler, since a user can run tasks
>> > on every cpu (through sys_sched_setaffinity()). Then doing while (1) cat
>> > /proc/$PID/stack would saturate the rq->lock on every CPU.
>> >
>> > The more tasks the merrier.
>>
>> Is this worse than it would be if this code used preempt_disable()
>> (which I think it did until very recently)?
>
> Much worse, since the proposed task_rq_lock() not only disables
> preemption, it also disables IRQs and takes 2 locks. And hogging the
> rq->lock affects other tasks their ability to schedule.
>

Fair enough.

I'm not sure I care quite enough about /proc/PID/stack to personally
dig through the scheduler and find a way to cleanly say "please don't
run this task for a little while".

[toc] | [prev] | [next] | [standalone]

#1485168 — Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

On Fri, Sep 16, 2016 at 08:32:44AM -0700, Andy Lutomirski wrote:
> 
> I'm not sure I care quite enough about /proc/PID/stack to personally
> dig through the scheduler and find a way to cleanly say "please don't
> run this task for a little while".

The 'best' we can do is prod the task awake and then have it take itself
out, which is what SIGSTOP and ptrace() do. It is also rather ugly, due
to avoiding overhead on regular hot paths.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web