Skip to contentSkip to conversation

thread_info cleanups and stack caching

PATCH Superseded Andy Lutomirski 9 participants 41 messages Sep 13, 2016 – Sep 26, 2016 1 message not in the archive

Subsystems (9)
x86 Library code MM core Scheduler +5 more subsystems

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Andy Lutomirski

author maintainer
#0 lore raw

cover 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(-)
Signature · 1 line
2.7.4
author maintainer
#1 lore raw

PATCH 1/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.

---
 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(-)
arch/x86/entry/common.c +2 -2
index 1433f6b4607d..871bbf975d4c 100644
@@ -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) {
arch/x86/include/asm/processor.h +12 -0
index b22fb5a4ff3c..984a7bf17f6a 100644
@@ -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)
arch/x86/include/asm/syscall.h +5 -15
index 4e23dd15c661..e3c95e8e61c5 100644
@@ -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 */
 
arch/x86/include/asm/thread_info.h +4 -19
index 494c4b5ada34..c9dcfe7c7e4b 100644
@@ -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
arch/x86/kernel/asm-offsets.c +0 -1
index db3a0af9b9ec..add5f90b93d4 100644
@@ -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);
arch/x86/kernel/fpu/init.c +0 -1
index 93982aebb398..2f2b8c7ccb85 100644
@@ -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;
arch/x86/kernel/process_64.c +2 -2
index b812cd0d7889..de9acaf2d371 100644
@@ -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);
arch/x86/kernel/ptrace.c +1 -1
index 5b88a1b26fc7..ce94c38cf4d6 100644
@@ -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):
arch/x86/kernel/signal.c +1 -1
index 04cb3212db2d..da20ecb5397a 100644
@@ -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
Signature · 1 line
2.7.4
#2 lore raw

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

x86/asm: Move the thread_info::status field to thread_struct
Commit-ID:  b9d989c7218ac922185d82ad46f3e58b27a4bea9
Gitweb:     http://git.kernel.org/tip/b9d989c7218ac922185d82ad46f3e58b27a4bea9
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Tue, 13 Sep 2016 14:29:21 -0700
Committer:  Ingo Molnar <[email protected]>
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.

---
 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(-)
arch/x86/entry/common.c +2 -2
index 1433f6b..871bbf9 100644
@@ -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) {
arch/x86/include/asm/processor.h +12 -0
index b22fb5a..984a7bf 100644
@@ -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)
arch/x86/include/asm/syscall.h +5 -15
index 4e23dd1..e3c95e8 100644
@@ -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 */
 
arch/x86/include/asm/thread_info.h +4 -19
index 494c4b5..c9dcfe7 100644
@@ -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
arch/x86/kernel/asm-offsets.c +0 -1
index db3a0af..add5f90 100644
@@ -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);
arch/x86/kernel/fpu/init.c +0 -1
index 93982ae..2f2b8c7 100644
@@ -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;
arch/x86/kernel/process_64.c +2 -2
index b812cd0..de9acaf 100644
@@ -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);
arch/x86/kernel/ptrace.c +1 -1
index 5b88a1b..ce94c38 100644
@@ -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):
arch/x86/kernel/signal.c +1 -1
index 04cb321..da20ecb 100644
@@ -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
author maintainer
#3 lore raw

PATCH 2/12 x86/entry: Get rid of pt_regs_to_thread_info()

In reply to Andy Lutomirski

From: Linus Torvalds <[email protected]>

It was a nice optimization while it lasted, but thread_info is moving and this optimization will no longer work.

Quoting Linus:

Oh Gods, Andy. That pt_regs_to_thread_info() thing made me want to do unspeakable acts on a poor innocent wax figure that looked _exactly_ like you.

[changelog written by Andy]
Message-Id: <CA+55aFxvZhBu9U1cqpVm4frv0p5mqu=0TxsSqE-=95ft8HvCVA@mail.gmail.com>

---
 arch/x86/entry/common.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)
arch/x86/entry/common.c +6 -14
index 871bbf975d4c..bdd9cc59d20f 100644
@@ -31,13 +31,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
 
-static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
-{
-	unsigned long top_of_stack =
-		(unsigned long)(regs + 1) + TOP_OF_KERNEL_STACK_PADDING;
-	return (struct thread_info *)(top_of_stack - THREAD_SIZE);
-}
-
 #ifdef CONFIG_CONTEXT_TRACKING
 /* Called on entry from user mode with IRQs off. */
 __visible inline void enter_from_user_mode(void)
@@ -71,7 +64,7 @@ static long syscall_trace_enter(struct pt_regs *regs)
 {
 	u32 arch = in_ia32_syscall() ? AUDIT_ARCH_I386 : AUDIT_ARCH_X86_64;
 
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	unsigned long ret = 0;
 	bool emulated = false;
 	u32 work;
@@ -173,18 +166,17 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 		/* Disable IRQs and retry */
 		local_irq_disable();
 
-		cached_flags = READ_ONCE(pt_regs_to_thread_info(regs)->flags);
+		cached_flags = READ_ONCE(current_thread_info()->flags);
 
 		if (!(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS))
 			break;
-
 	}
 }
 
 /* Called with IRQs disabled. */
 __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	u32 cached_flags;
 
 	if (IS_ENABLED(CONFIG_PROVE_LOCKING) && WARN_ON(!irqs_disabled()))
@@ -247,7 +239,7 @@ static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags)
  */
 __visible inline void syscall_return_slowpath(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	u32 cached_flags = READ_ONCE(ti->flags);
 
 	CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
@@ -270,7 +262,7 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
 #ifdef CONFIG_X86_64
 __visible void do_syscall_64(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	unsigned long nr = regs->orig_ax;
 
 	enter_from_user_mode();
@@ -303,7 +295,7 @@ __visible void do_syscall_64(struct pt_regs *regs)
  */
 static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	unsigned int nr = (unsigned int)regs->orig_ax;
 
 #ifdef CONFIG_IA32_EMULATION
Signature · 1 line
2.7.4
#4 lore raw

Re: [PATCH 02/12] x86/entry: Get rid of pt_regs_to_thread_info()

Quoted 5 lines · Andy Lutomirski

From: Linus Torvalds <[email protected]>

It was a nice optimization while it lasted, but thread_info is moving and this optimization will no longer work.

Quoted 7 lines

Quoting Linus:

Oh Gods, Andy. That pt_regs_to_thread_info() thing made me want to do unspeakable acts on a poor innocent wax figure that looked _exactly_ like you.

[changelog written by Andy]

Linus, since this came from you I presume this is still fine, but I'm adding your Signed-off-by as well to make the SOB chain neat:

Is this ok?

Thanks,

Ingo

#5 lore raw

[tip:x86/asm] x86/entry: Get rid of pt_regs_to_thread_info()

In reply to Andy Lutomirski
x86/entry: Get rid of pt_regs_to_thread_info()
Commit-ID:  97245d00585d82540f4538cf72d92a1e853c7b0e
Gitweb:     http://git.kernel.org/tip/97245d00585d82540f4538cf72d92a1e853c7b0e
Author:     Linus Torvalds <[email protected]>
AuthorDate: Tue, 13 Sep 2016 14:29:22 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 15 Sep 2016 08:25:12 +0200

x86/entry: Get rid of pt_regs_to_thread_info()

It was a nice optimization while it lasted, but thread_info is moving and this optimization will no longer work.

Quoting Linus:

Oh Gods, Andy. That pt_regs_to_thread_info() thing made me want to do unspeakable acts on a poor innocent wax figure that looked _exactly_ like you.

---
 arch/x86/entry/common.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)
arch/x86/entry/common.c +6 -14
index 871bbf9..bdd9cc5 100644
@@ -31,13 +31,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
 
-static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
-{
-	unsigned long top_of_stack =
-		(unsigned long)(regs + 1) + TOP_OF_KERNEL_STACK_PADDING;
-	return (struct thread_info *)(top_of_stack - THREAD_SIZE);
-}
-
 #ifdef CONFIG_CONTEXT_TRACKING
 /* Called on entry from user mode with IRQs off. */
 __visible inline void enter_from_user_mode(void)
@@ -71,7 +64,7 @@ static long syscall_trace_enter(struct pt_regs *regs)
 {
 	u32 arch = in_ia32_syscall() ? AUDIT_ARCH_I386 : AUDIT_ARCH_X86_64;
 
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	unsigned long ret = 0;
 	bool emulated = false;
 	u32 work;
@@ -173,18 +166,17 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 		/* Disable IRQs and retry */
 		local_irq_disable();
 
-		cached_flags = READ_ONCE(pt_regs_to_thread_info(regs)->flags);
+		cached_flags = READ_ONCE(current_thread_info()->flags);
 
 		if (!(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS))
 			break;
-
 	}
 }
 
 /* Called with IRQs disabled. */
 __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	u32 cached_flags;
 
 	if (IS_ENABLED(CONFIG_PROVE_LOCKING) && WARN_ON(!irqs_disabled()))
@@ -247,7 +239,7 @@ static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags)
  */
 __visible inline void syscall_return_slowpath(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	u32 cached_flags = READ_ONCE(ti->flags);
 
 	CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
@@ -270,7 +262,7 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
 #ifdef CONFIG_X86_64
 __visible void do_syscall_64(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	unsigned long nr = regs->orig_ax;
 
 	enter_from_user_mode();
@@ -303,7 +295,7 @@ __visible void do_syscall_64(struct pt_regs *regs)
  */
 static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 {
-	struct thread_info *ti = pt_regs_to_thread_info(regs);
+	struct thread_info *ti = current_thread_info();
 	unsigned int nr = (unsigned int)regs->orig_ax;
 
 #ifdef CONFIG_IA32_EMULATION
author maintainer
#6 lore raw

PATCH 3/12 um: Stop conflating task_struct::stack with thread_info

In reply to Andy Lutomirski

From: Linus Torvalds <[email protected]>

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>

---
 arch/x86/um/ptrace_32.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/um/ptrace_32.c +4 -4
index ebd4dd6ef73b..2a6f781ce9cf 100644
@@ -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));
Signature · 1 line
2.7.4
#7 lore raw

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

Quoted 9 lines · Andy Lutomirski

From: Linus Torvalds <[email protected]>

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>

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

Thanks,

Ingo

#8 lore raw

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

In reply to Andy Lutomirski
um/Stop conflating task_struct::stack with thread_info
Commit-ID:  d896fa20a70c9e596438728561e058a74ed3196b
Gitweb:     http://git.kernel.org/tip/d896fa20a70c9e596438728561e058a74ed3196b
Author:     Linus Torvalds <[email protected]>
AuthorDate: Tue, 13 Sep 2016 14:29:23 -0700
Committer:  Ingo Molnar <[email protected]>
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)". ]

---
 arch/x86/um/ptrace_32.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/um/ptrace_32.c +4 -4
index a7ef7b1..5766ead 100644
@@ -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));
author maintainer
#9 lore raw

PATCH 4/12 sched: Allow putting thread_info into task_struct

In reply to Andy Lutomirski

If an arch opts in by setting CONFIG_THREAD_INFO_IN_TASK_STRUCT, then thread_info is defined as a single 'u32 flags' and is the first entry of task_struct. thread_info::task is removed (it serves no purpose if thread_info is embedded in task_struct), and thread_info::cpu gets its own slot in task_struct.

This is heavily based on a patch written by Linus.

---
 include/linux/init_task.h   |  9 +++++++++
 include/linux/sched.h       | 36 ++++++++++++++++++++++++++++++++++--
 include/linux/thread_info.h | 15 +++++++++++++++
 init/Kconfig                |  7 +++++++
 init/init_task.c            |  7 +++++--
 kernel/sched/sched.h        |  4 ++++
 6 files changed, 74 insertions(+), 4 deletions(-)
include/linux/init_task.h +9 -0
index f8834f820ec2..9c04d44eeb3c 100644
@@ -15,6 +15,8 @@
 #include <net/net_namespace.h>
 #include <linux/sched/rt.h>
 
+#include <asm/thread_info.h>
+
 #ifdef CONFIG_SMP
 # define INIT_PUSHABLE_TASKS(tsk)					\
 	.pushable_tasks = PLIST_NODE_INIT(tsk.pushable_tasks, MAX_PRIO),
@@ -183,12 +185,19 @@ extern struct task_group root_task_group;
 # define INIT_KASAN(tsk)
 #endif
 
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+# define INIT_TASK_TI(tsk) .thread_info = INIT_THREAD_INFO(tsk),
+#else
+# define INIT_TASK_TI(tsk)
+#endif
+
 /*
  *  INIT_TASK is used to set up the first task table, touch at
  * your own risk!. Base=0, limit=0x1fffff (=2MB)
  */
 #define INIT_TASK(tsk)	\
 {									\
+	INIT_TASK_TI(tsk)						\
 	.state		= 0,						\
 	.stack		= init_stack,					\
 	.usage		= ATOMIC_INIT(2),				\
include/linux/sched.h +34 -2
index 20f9f47bcfd0..a287e8b13549 100644
@@ -1458,6 +1458,13 @@ struct tlbflush_unmap_batch {
 };
 
 struct task_struct {
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	/*
+	 * For reasons of header soup (see current_thread_info()), this
+	 * must be the first element of task_struct.
+	 */
+	struct thread_info thread_info;
+#endif
 	volatile long state;	/* -1 unrunnable, 0 runnable, >0 stopped */
 	void *stack;
 	atomic_t usage;
@@ -1467,6 +1474,9 @@ struct task_struct {
 #ifdef CONFIG_SMP
 	struct llist_node wake_entry;
 	int on_cpu;
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	unsigned int cpu;	/* current CPU */
+#endif
 	unsigned int wakee_flips;
 	unsigned long wakee_flip_decay_ts;
 	struct task_struct *last_wakee;
@@ -2588,7 +2598,9 @@ extern void set_curr_task(int cpu, struct task_struct *p);
 void yield(void);
 
 union thread_union {
+#ifndef CONFIG_THREAD_INFO_IN_TASK
 	struct thread_info thread_info;
+#endif
 	unsigned long stack[THREAD_SIZE/sizeof(long)];
 };
 
@@ -3076,10 +3088,26 @@ static inline void threadgroup_change_end(struct task_struct *tsk)
 	cgroup_threadgroup_change_end(tsk);
 }
 
-#ifndef __HAVE_THREAD_FUNCTIONS
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+
+static inline struct thread_info *task_thread_info(struct task_struct *task)
+{
+	return &task->thread_info;
+}
+static inline void *task_stack_page(const struct task_struct *task)
+{
+	return task->stack;
+}
+#define setup_thread_stack(new,old)	do { } while(0)
+static inline unsigned long *end_of_stack(const struct task_struct *task)
+{
+	return task->stack;
+}
+
+#elif !defined(__HAVE_THREAD_FUNCTIONS)
 
 #define task_thread_info(task)	((struct thread_info *)(task)->stack)
-#define task_stack_page(task)	((task)->stack)
+#define task_stack_page(task)	((void *)(task)->stack)
 
 static inline void setup_thread_stack(struct task_struct *p, struct task_struct *org)
 {
@@ -3379,7 +3407,11 @@ static inline void ptrace_signal_wake_up(struct task_struct *t, bool resume)
 
 static inline unsigned int task_cpu(const struct task_struct *p)
 {
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	return p->cpu;
+#else
 	return task_thread_info(p)->cpu;
+#endif
 }
 
 static inline int task_node(const struct task_struct *p)
include/linux/thread_info.h +15 -0
index cbd8990e2e77..cb0ed342d6b1 100644
@@ -13,6 +13,21 @@
 struct timespec;
 struct compat_timespec;
 
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+struct thread_info {
+	u32			flags;		/* low level flags */
+};
+
+#define INIT_THREAD_INFO(tsk)			\
+{						\
+	.flags		= 0,			\
+}
+#endif
+
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+#define current_thread_info() ((struct thread_info *)current)
+#endif
+
 /*
  * System call restart block.
  */
init/Kconfig +7 -0
index cac3f096050d..ec8d43894b02 100644
@@ -26,6 +26,13 @@ config IRQ_WORK
 config BUILDTIME_EXTABLE_SORT
 	bool
 
+config THREAD_INFO_IN_TASK
+	bool
+	help
+	  Select this to move thread_info off the stack into task_struct.  To
+	  make this work, an arch will need to remove all thread_info fields
+	  except flags and fix any runtime bugs.
+
 menu "General setup"
 
 config BROKEN
init/init_task.c +5 -2
index ba0a7f362d9e..11f83be1fa79 100644
@@ -22,5 +22,8 @@ EXPORT_SYMBOL(init_task);
  * Initial thread structure. Alignment of this is handled by a special
  * linker map entry.
  */
-union thread_union init_thread_union __init_task_data =
-	{ INIT_THREAD_INFO(init_task) };
+union thread_union init_thread_union __init_task_data = {
+#ifndef CONFIG_THREAD_INFO_IN_TASK
+	INIT_THREAD_INFO(init_task)
+#endif
+};
kernel/sched/sched.h +4 -0
index c64fc5114004..3655c9625e5b 100644
@@ -1000,7 +1000,11 @@ static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu)
 	 * per-task data have been completed by this moment.
 	 */
 	smp_wmb();
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	p->cpu = cpu;
+#else
 	task_thread_info(p)->cpu = cpu;
+#endif
 	p->wake_cpu = cpu;
 #endif
 }
Signature · 1 line
2.7.4
#10 lore raw

[tip:x86/asm] sched/core: Allow putting thread_info into task_struct

sched/core: Allow putting thread_info into task_struct
Commit-ID:  c65eacbe290b8141554c71b2c94489e73ade8c8d
Gitweb:     http://git.kernel.org/tip/c65eacbe290b8141554c71b2c94489e73ade8c8d
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Tue, 13 Sep 2016 14:29:24 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 15 Sep 2016 08:25:13 +0200

sched/core: Allow putting thread_info into task_struct

If an arch opts in by setting CONFIG_THREAD_INFO_IN_TASK_STRUCT, then thread_info is defined as a single 'u32 flags' and is the first entry of task_struct. thread_info::task is removed (it serves no purpose if thread_info is embedded in task_struct), and thread_info::cpu gets its own slot in task_struct.

This is heavily based on a patch written by Linus.

Originally-from: Linus Torvalds <[email protected]>

---
 include/linux/init_task.h   |  9 +++++++++
 include/linux/sched.h       | 36 ++++++++++++++++++++++++++++++++++--
 include/linux/thread_info.h | 15 +++++++++++++++
 init/Kconfig                |  7 +++++++
 init/init_task.c            |  7 +++++--
 kernel/sched/sched.h        |  4 ++++
 6 files changed, 74 insertions(+), 4 deletions(-)
include/linux/init_task.h +9 -0
index f8834f8..9c04d44 100644
@@ -15,6 +15,8 @@
 #include <net/net_namespace.h>
 #include <linux/sched/rt.h>
 
+#include <asm/thread_info.h>
+
 #ifdef CONFIG_SMP
 # define INIT_PUSHABLE_TASKS(tsk)					\
 	.pushable_tasks = PLIST_NODE_INIT(tsk.pushable_tasks, MAX_PRIO),
@@ -183,12 +185,19 @@ extern struct task_group root_task_group;
 # define INIT_KASAN(tsk)
 #endif
 
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+# define INIT_TASK_TI(tsk) .thread_info = INIT_THREAD_INFO(tsk),
+#else
+# define INIT_TASK_TI(tsk)
+#endif
+
 /*
  *  INIT_TASK is used to set up the first task table, touch at
  * your own risk!. Base=0, limit=0x1fffff (=2MB)
  */
 #define INIT_TASK(tsk)	\
 {									\
+	INIT_TASK_TI(tsk)						\
 	.state		= 0,						\
 	.stack		= init_stack,					\
 	.usage		= ATOMIC_INIT(2),				\
include/linux/sched.h +34 -2
index 20f9f47..a287e8b 100644
@@ -1458,6 +1458,13 @@ struct tlbflush_unmap_batch {
 };
 
 struct task_struct {
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	/*
+	 * For reasons of header soup (see current_thread_info()), this
+	 * must be the first element of task_struct.
+	 */
+	struct thread_info thread_info;
+#endif
 	volatile long state;	/* -1 unrunnable, 0 runnable, >0 stopped */
 	void *stack;
 	atomic_t usage;
@@ -1467,6 +1474,9 @@ struct task_struct {
 #ifdef CONFIG_SMP
 	struct llist_node wake_entry;
 	int on_cpu;
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	unsigned int cpu;	/* current CPU */
+#endif
 	unsigned int wakee_flips;
 	unsigned long wakee_flip_decay_ts;
 	struct task_struct *last_wakee;
@@ -2588,7 +2598,9 @@ extern void set_curr_task(int cpu, struct task_struct *p);
 void yield(void);
 
 union thread_union {
+#ifndef CONFIG_THREAD_INFO_IN_TASK
 	struct thread_info thread_info;
+#endif
 	unsigned long stack[THREAD_SIZE/sizeof(long)];
 };
 
@@ -3076,10 +3088,26 @@ static inline void threadgroup_change_end(struct task_struct *tsk)
 	cgroup_threadgroup_change_end(tsk);
 }
 
-#ifndef __HAVE_THREAD_FUNCTIONS
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+
+static inline struct thread_info *task_thread_info(struct task_struct *task)
+{
+	return &task->thread_info;
+}
+static inline void *task_stack_page(const struct task_struct *task)
+{
+	return task->stack;
+}
+#define setup_thread_stack(new,old)	do { } while(0)
+static inline unsigned long *end_of_stack(const struct task_struct *task)
+{
+	return task->stack;
+}
+
+#elif !defined(__HAVE_THREAD_FUNCTIONS)
 
 #define task_thread_info(task)	((struct thread_info *)(task)->stack)
-#define task_stack_page(task)	((task)->stack)
+#define task_stack_page(task)	((void *)(task)->stack)
 
 static inline void setup_thread_stack(struct task_struct *p, struct task_struct *org)
 {
@@ -3379,7 +3407,11 @@ static inline void ptrace_signal_wake_up(struct task_struct *t, bool resume)
 
 static inline unsigned int task_cpu(const struct task_struct *p)
 {
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	return p->cpu;
+#else
 	return task_thread_info(p)->cpu;
+#endif
 }
 
 static inline int task_node(const struct task_struct *p)
include/linux/thread_info.h +15 -0
index 2b5b10e..e2d0fd8 100644
@@ -13,6 +13,21 @@
 struct timespec;
 struct compat_timespec;
 
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+struct thread_info {
+	u32			flags;		/* low level flags */
+};
+
+#define INIT_THREAD_INFO(tsk)			\
+{						\
+	.flags		= 0,			\
+}
+#endif
+
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+#define current_thread_info() ((struct thread_info *)current)
+#endif
+
 /*
  * System call restart block.
  */
init/Kconfig +7 -0
index cac3f09..ec8d438 100644
@@ -26,6 +26,13 @@ config IRQ_WORK
 config BUILDTIME_EXTABLE_SORT
 	bool
 
+config THREAD_INFO_IN_TASK
+	bool
+	help
+	  Select this to move thread_info off the stack into task_struct.  To
+	  make this work, an arch will need to remove all thread_info fields
+	  except flags and fix any runtime bugs.
+
 menu "General setup"
 
 config BROKEN
init/init_task.c +5 -2
index ba0a7f36..11f83be1 100644
@@ -22,5 +22,8 @@ EXPORT_SYMBOL(init_task);
  * Initial thread structure. Alignment of this is handled by a special
  * linker map entry.
  */
-union thread_union init_thread_union __init_task_data =
-	{ INIT_THREAD_INFO(init_task) };
+union thread_union init_thread_union __init_task_data = {
+#ifndef CONFIG_THREAD_INFO_IN_TASK
+	INIT_THREAD_INFO(init_task)
+#endif
+};
kernel/sched/sched.h +4 -0
index c64fc51..3655c96 100644
@@ -1000,7 +1000,11 @@ static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu)
 	 * per-task data have been completed by this moment.
 	 */
 	smp_wmb();
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	p->cpu = cpu;
+#else
 	task_thread_info(p)->cpu = cpu;
+#endif
 	p->wake_cpu = cpu;
 #endif
 }
author maintainer
#11 lore raw

PATCH 5/12 x86: Move thread_info into task_struct

In reply to Andy Lutomirski

Now that most of the thread_info users have been cleaned up, this is straightforward.

Most of this code was written by Linus.

---
 arch/x86/Kconfig                   |  1 +
 arch/x86/entry/entry_64.S          |  7 ++++--
 arch/x86/include/asm/thread_info.h | 46 --------------------------------------
 arch/x86/kernel/asm-offsets.c      |  4 +---
 arch/x86/kernel/irq_64.c           |  3 +--
 arch/x86/kernel/process.c          |  6 ++---
 6 files changed, 10 insertions(+), 57 deletions(-)
arch/x86/Kconfig +1 -0
index ce8860cccc34..452412cbd6c1 100644
@@ -158,6 +158,7 @@ config X86
 	select SPARSE_IRQ
 	select SRCU
 	select SYSCTL_EXCEPTION_TRACE
+	select THREAD_INFO_IN_TASK
 	select USER_STACKTRACE_SUPPORT
 	select VIRT_TO_BUS
 	select X86_DEV_DMA_OPS			if X86_64
arch/x86/entry/entry_64.S +5 -2
index c0373d667674..0cfc665d6716 100644
@@ -179,7 +179,8 @@ GLOBAL(entry_SYSCALL_64_after_swapgs)
 	 * If we need to do entry work or if we guess we'll need to do
 	 * exit work, go straight to the slow path.
 	 */
-	testl	$_TIF_WORK_SYSCALL_ENTRY|_TIF_ALLWORK_MASK, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
+	movq	PER_CPU_VAR(current_task), %r11
+	testl	$_TIF_WORK_SYSCALL_ENTRY|_TIF_ALLWORK_MASK, TASK_TI_flags(%r11)
 	jnz	entry_SYSCALL64_slow_path
 
 entry_SYSCALL_64_fastpath:
@@ -217,7 +218,8 @@ entry_SYSCALL_64_fastpath:
 	 */
 	DISABLE_INTERRUPTS(CLBR_NONE)
 	TRACE_IRQS_OFF
-	testl	$_TIF_ALLWORK_MASK, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
+	movq	PER_CPU_VAR(current_task), %r11
+	testl	$_TIF_ALLWORK_MASK, TASK_TI_flags(%r11)
 	jnz	1f
 
 	LOCKDEP_SYS_EXIT
@@ -370,6 +372,7 @@ END(ptregs_\func)
 /*
  * %rdi: prev task
  * %rsi: next task
+ * rsi: task we're switching to
  */
 ENTRY(__switch_to_asm)
 	/*
arch/x86/include/asm/thread_info.h +0 -46

64 lines not on this page · raw message

arch/x86/kernel/asm-offsets.c +1 -3
index add5f90b93d4..c62e015b126c 100644
@@ -35,9 +35,7 @@ void common(void) {
 #endif
 
 	BLANK();
-	OFFSET(TI_flags, thread_info, flags);
-
-	BLANK();
+	OFFSET(TASK_TI_flags, task_struct, thread_info.flags);
 	OFFSET(TASK_addr_limit, task_struct, thread.addr_limit);
 
 	BLANK();
arch/x86/kernel/irq_64.c +1 -2
index 4a7903714065..9ebd0b0e73d9 100644
@@ -40,8 +40,7 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 	if (user_mode(regs))
 		return;
 
-	if (regs->sp >= curbase + sizeof(struct thread_info) +
-				  sizeof(struct pt_regs) + STACK_TOP_MARGIN &&
+	if (regs->sp >= curbase + sizeof(struct pt_regs) + STACK_TOP_MARGIN &&
 	    regs->sp <= curbase + THREAD_SIZE)
 		return;
 
arch/x86/kernel/process.c +2 -4
index c1fa790c81cd..0b9ed8ec5226 100644
@@ -549,9 +549,7 @@ unsigned long get_wchan(struct task_struct *p)
 	 * PADDING
 	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
 	 * stack
-	 * ----------- bottom = start + sizeof(thread_info)
-	 * thread_info
-	 * ----------- start
+	 * ----------- bottom = start
 	 *
 	 * The tasks stack pointer points at the location where the
 	 * framepointer is stored. The data on the stack is:
@@ -562,7 +560,7 @@ unsigned long get_wchan(struct task_struct *p)
 	 */
 	top = start + THREAD_SIZE - TOP_OF_KERNEL_STACK_PADDING;
 	top -= 2 * sizeof(unsigned long);
-	bottom = start + sizeof(struct thread_info);
+	bottom = start;
 
 	sp = READ_ONCE(p->thread.sp);
 	if (sp < bottom || sp > top)
Signature · 1 line
2.7.4
#12 lore raw

[tip:x86/asm] x86: Move thread_info into task_struct

x86: Move thread_info into task_struct
Commit-ID:  15f4eae70d365bba26854c90b6002aaabb18c8aa
Gitweb:     http://git.kernel.org/tip/15f4eae70d365bba26854c90b6002aaabb18c8aa
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Tue, 13 Sep 2016 14:29:25 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 15 Sep 2016 08:25:13 +0200

x86: Move thread_info into task_struct

Now that most of the thread_info users have been cleaned up, this is straightforward.

Most of this code was written by Linus.

Originally-from: Linus Torvalds <[email protected]>

---
 arch/x86/Kconfig                   |  1 +
 arch/x86/entry/entry_64.S          |  7 ++++--
 arch/x86/include/asm/thread_info.h | 46 --------------------------------------
 arch/x86/kernel/asm-offsets.c      |  4 +---
 arch/x86/kernel/irq_64.c           |  3 +--
 arch/x86/kernel/process.c          |  6 ++---
 6 files changed, 10 insertions(+), 57 deletions(-)
arch/x86/Kconfig +1 -0
index 4c39728..2a83bc8 100644
@@ -157,6 +157,7 @@ config X86
 	select SPARSE_IRQ
 	select SRCU
 	select SYSCTL_EXCEPTION_TRACE
+	select THREAD_INFO_IN_TASK
 	select USER_STACKTRACE_SUPPORT
 	select VIRT_TO_BUS
 	select X86_DEV_DMA_OPS			if X86_64
arch/x86/entry/entry_64.S +5 -2
index e7fba58..2b46384 100644
@@ -179,7 +179,8 @@ GLOBAL(entry_SYSCALL_64_after_swapgs)
 	 * If we need to do entry work or if we guess we'll need to do
 	 * exit work, go straight to the slow path.
 	 */
-	testl	$_TIF_WORK_SYSCALL_ENTRY|_TIF_ALLWORK_MASK, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
+	movq	PER_CPU_VAR(current_task), %r11
+	testl	$_TIF_WORK_SYSCALL_ENTRY|_TIF_ALLWORK_MASK, TASK_TI_flags(%r11)
 	jnz	entry_SYSCALL64_slow_path
 
 entry_SYSCALL_64_fastpath:
@@ -217,7 +218,8 @@ entry_SYSCALL_64_fastpath:
 	 */
 	DISABLE_INTERRUPTS(CLBR_NONE)
 	TRACE_IRQS_OFF
-	testl	$_TIF_ALLWORK_MASK, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
+	movq	PER_CPU_VAR(current_task), %r11
+	testl	$_TIF_ALLWORK_MASK, TASK_TI_flags(%r11)
 	jnz	1f
 
 	LOCKDEP_SYS_EXIT
@@ -370,6 +372,7 @@ END(ptregs_\func)
 /*
  * %rdi: prev task
  * %rsi: next task
+ * rsi: task we're switching to
  */
 ENTRY(__switch_to_asm)
 	/*
arch/x86/include/asm/thread_info.h +0 -46

64 lines not on this page · raw message

arch/x86/kernel/asm-offsets.c +1 -3
index add5f90..c62e015 100644
@@ -35,9 +35,7 @@ void common(void) {
 #endif
 
 	BLANK();
-	OFFSET(TI_flags, thread_info, flags);
-
-	BLANK();
+	OFFSET(TASK_TI_flags, task_struct, thread_info.flags);
 	OFFSET(TASK_addr_limit, task_struct, thread.addr_limit);
 
 	BLANK();
arch/x86/kernel/irq_64.c +1 -2
index 4a79037..9ebd0b0 100644
@@ -40,8 +40,7 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 	if (user_mode(regs))
 		return;
 
-	if (regs->sp >= curbase + sizeof(struct thread_info) +
-				  sizeof(struct pt_regs) + STACK_TOP_MARGIN &&
+	if (regs->sp >= curbase + sizeof(struct pt_regs) + STACK_TOP_MARGIN &&
 	    regs->sp <= curbase + THREAD_SIZE)
 		return;
 
arch/x86/kernel/process.c +2 -4
index c1fa790..0b9ed8e 100644
@@ -549,9 +549,7 @@ unsigned long get_wchan(struct task_struct *p)
 	 * PADDING
 	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
 	 * stack
-	 * ----------- bottom = start + sizeof(thread_info)
-	 * thread_info
-	 * ----------- start
+	 * ----------- bottom = start
 	 *
 	 * The tasks stack pointer points at the location where the
 	 * framepointer is stored. The data on the stack is:
@@ -562,7 +560,7 @@ unsigned long get_wchan(struct task_struct *p)
 	 */
 	top = start + THREAD_SIZE - TOP_OF_KERNEL_STACK_PADDING;
 	top -= 2 * sizeof(unsigned long);
-	bottom = start + sizeof(struct thread_info);
+	bottom = start;
 
 	sp = READ_ONCE(p->thread.sp);
 	if (sp < bottom || sp > top)
author maintainer
#13 lore raw

PATCH 6/12 sched: Add try_get_task_stack() and put_task_stack()

In reply to Andy Lutomirski

There are a few places in the kernel that access stack memory belonging to a different task. Before we can start freeing task stacks before the task_struct is freed, we need a way for those code paths to pin the stack.

---
 include/linux/sched.h | 16 ++++++++++++++++
 init/Kconfig          |  3 +++
 2 files changed, 19 insertions(+)
include/linux/sched.h +16 -0
index a287e8b13549..a95867267e9f 100644
@@ -3094,11 +3094,19 @@ static inline struct thread_info *task_thread_info(struct task_struct *task)
 {
 	return &task->thread_info;
 }
+
+/*
+ * When accessing the stack of a non-current task that might exit, use
+ * try_get_task_stack() instead.  task_stack_page will return a pointer
+ * that could get freed out from under you.
+ */
 static inline void *task_stack_page(const struct task_struct *task)
 {
 	return task->stack;
 }
+
 #define setup_thread_stack(new,old)	do { } while(0)
+
 static inline unsigned long *end_of_stack(const struct task_struct *task)
 {
 	return task->stack;
@@ -3134,6 +3142,14 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
 }
 
 #endif
+
+static inline void *try_get_task_stack(struct task_struct *tsk)
+{
+	return task_stack_page(tsk);
+}
+
+static inline void put_task_stack(struct task_struct *tsk) {}
+
 #define task_stack_end_corrupted(task) \
 		(*(end_of_stack(task)) != STACK_END_MAGIC)
 
init/Kconfig +3 -0
index ec8d43894b02..3b9a47fe843b 100644
@@ -33,6 +33,9 @@ config THREAD_INFO_IN_TASK
 	  make this work, an arch will need to remove all thread_info fields
 	  except flags and fix any runtime bugs.
 
+	  One subtle change that will be needed is to use try_get_task_stack()
+	  and put_task_stack() in save_thread_stack_tsk() and get_wchan().
+
 menu "General setup"
 
 config BROKEN
Signature · 1 line
2.7.4
author maintainer
#14 lore raw

PATCH 7/12 kthread: to_live_kthread() needs try_get_task_stack()

In reply to Andy Lutomirski

From: Oleg Nesterov <[email protected]>

get_task_struct(tsk) no longer pins tsk->stack so all users of to_live_kthread() should do try_get_task_stack/put_task_stack to protect "struct kthread" which lives on kthread's stack.

TODO: Kill to_live_kthread(), perhaps we can even kill "struct kthread" too, and rework kthread_stop(), it can use task_work_add() to sync with the exiting kernel thread.

Message-Id: <[email protected]>

---
 kernel/kthread.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
kernel/kthread.c +6 -2
index 9ff173dca1ae..4ab4c3766a80 100644
@@ -64,7 +64,7 @@ static inline struct kthread *to_kthread(struct task_struct *k)
 static struct kthread *to_live_kthread(struct task_struct *k)
 {
 	struct completion *vfork = ACCESS_ONCE(k->vfork_done);
-	if (likely(vfork))
+	if (likely(vfork) && try_get_task_stack(k))
 		return __to_kthread(vfork);
 	return NULL;
 }
@@ -425,8 +425,10 @@ void kthread_unpark(struct task_struct *k)
 {
 	struct kthread *kthread = to_live_kthread(k);
 
-	if (kthread)
+	if (kthread) {
 		__kthread_unpark(k, kthread);
+		put_task_stack(k);
+	}
 }
 EXPORT_SYMBOL_GPL(kthread_unpark);
 
@@ -455,6 +457,7 @@ int kthread_park(struct task_struct *k)
 				wait_for_completion(&kthread->parked);
 			}
 		}
+		put_task_stack(k);
 		ret = 0;
 	}
 	return ret;
@@ -490,6 +493,7 @@ int kthread_stop(struct task_struct *k)
 		__kthread_unpark(k, kthread);
 		wake_up_process(k);
 		wait_for_completion(&kthread->exited);
+		put_task_stack(k);
 	}
 	ret = k->exit_code;
 	put_task_struct(k);
Signature · 1 line
2.7.4
author maintainer
#15 lore raw

PATCH 8/12 x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

In reply to Andy Lutomirski

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

---
 arch/x86/kernel/stacktrace.c | 5 +++++
 1 file changed, 5 insertions(+)
arch/x86/kernel/stacktrace.c +5 -0
index 4738f5e0f2ab..b3f32fbe3ba4 100644
@@ -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);
 
Signature · 1 line
2.7.4
#16 lore raw

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

Quoted 4 lines · Andy Lutomirski

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

Do we need a similar patch for show_stack()?

Signature · 1 line
Josh
#17 lore raw

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

Quoted 7 lines · Josh Poimboeuf
Quoted 4 lines · Andy Lutomirski

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

Do we need a similar patch for show_stack()?

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

#18 lore raw

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

Quoted 10 lines · Andy Lutomirski
Quoted 7 lines · Josh Poimboeuf
Quoted 4 lines · Andy Lutomirski

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

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.

Signature · 1 line
Josh
#19 lore raw

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

Quoted 6 lines · Josh Poimboeuf
Quoted 5 lines · Andy Lutomirski
Quoted 4 lines · Josh Poimboeuf
Quoted 3 lines · Andy Lutomirski

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

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

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.

#20 lore raw

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

Quoted 13 lines · Andy Lutomirski
Quoted 12 lines · Josh Poimboeuf
Quoted 10 lines · Andy Lutomirski
Quoted 7 lines · Josh Poimboeuf
Quoted 4 lines · Andy Lutomirski

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

Do we need a similar patch for show_stack()?

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

Quoted 7 lines
Quoted 1 line

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.

Signature · 1 line
Josh
#21 lore raw

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

Quoted 22 lines · Josh Poimboeuf
Quoted 13 lines · Andy Lutomirski
Quoted 12 lines · Josh Poimboeuf
Quoted 10 lines · Andy Lutomirski
Quoted 7 lines · Josh Poimboeuf
Quoted 4 lines · Andy Lutomirski

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

Do we need a similar patch for show_stack()?

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

Quoted 7 lines
Quoted 1 line

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.

Quoted 7 lines

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

#22 lore raw

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

Quoted 7 lines · Andy Lutomirski

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

Quoted 7 lines

- 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/[email protected]

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.

Signature · 1 line
Josh
maintainer
#23 lore raw

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

Quoted 16 lines · Josh Poimboeuf
Quoted 7 lines · Andy Lutomirski

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

Quoted 7 lines

- 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.

Quoted 6 lines

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

https://lkml.kernel.org/r/[email protected]

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.

#24 lore raw

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

Quoted 21 lines · Peter Zijlstra
Quoted 20 lines · Josh Poimboeuf
Quoted 7 lines · Andy Lutomirski

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

Quoted 7 lines

- 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/[email protected]

Quoted 10 lines
Quoted 2 lines

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)?

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
maintainer
#25 lore raw

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

Quoted 6 lines · Andy Lutomirski
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Josh Poimboeuf

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.

Quoted 8 lines
Quoted 5 lines

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.

author maintainer
#26 lore raw

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

Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Andy Lutomirski
Quoted 9 lines · Peter Zijlstra
Quoted 2 lines · Josh Poimboeuf

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.

Quoted 9 lines
Quoted 4 lines
Quoted 1 line

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".

maintainer
#27 lore raw

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

Quoted 4 lines · Andy Lutomirski

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.

#28 lore raw

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

In reply to Andy Lutomirski
Quoted 16 lines · Andy Lutomirski

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

---
 arch/x86/kernel/stacktrace.c | 5 +++++
 1 file changed, 5 insertions(+)
arch/x86/kernel/stacktrace.c +0 -0
index 4738f5e0f2ab..b3f32fbe3ba4 100644
@@ -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)
 {

arch/x86/kernel/stacktrace.c void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace) · line 91

Quoted 10 lines
arch/x86/kernel/stacktrace.c +5 -0
+	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);

So I very much like the first half of the series, the thread_info merge into task_struct (yay!) and I am in the process of applying those bits to -tip.

So the above not (yet) fully correct patch is in preparation to a later change you are doing in this series:

[PATCH 11/12] sched: Free the stack early if CONFIG_THREAD_INFO_IN_TASK

But I am having serious second thoughts about the fact that we now have to sprinke non-obvious try_get_task_stack()/put_task_stack() pairs into debugging code that never really needed anything like that, which pairs are easy to get wrong, easy to miss - and generally if we miss them will it will result in a slightly buggy, very unpleasant, racy, crashy behavior of the kernel.

Can we just ... not do the delayed freeing?

I mean, the cache hotness arguments don't look overly convincing to me: if we just start not using a piece of formerly cache hot memory then those cache lines will eventually decay naturally in whatever LRU scheme the CPU is using and will be reused without much harm done. It's just "another day in CPU land" that happens all the time. Yes, we could reduce the latency of the freeing and thus slightly improve the locality of other bits ... but is it really measurable or noticeable in any fashion. It's like task/thread fork/clone/exit is _that_ much of a fast path.

And then there's also the whole CONFIG_THREAD_INFO_IN_TASK #ifdeffery spreading.

I.e I just don't see the justification for this kind of object life time assymetry and hard to debug fragility.

Am I missing something?

Thanks,

Ingo

A message that is not in the archive (sent off-list or not yet received): CA+55aFxt=HLrELBE=BXUrWdh6LYs4gtu9S=yCruiDffq4HN80w@mail.gmail.com

#30 lore raw

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

Quoted 3 lines · Linus Torvalds

It absolutely is - compared to the vmalloc costs. Andy has quite noticeable slowdowns without the stack reuse - and the stack reuse requires the delayed freeing. With the stack reuse, fork/exit actually sped up.

Ok, great, that's convincing!

I'm still applying it two-phase and I'll wait for v2 of the second half of the series, to have the fix Josh alluded to.

Thanks,

Ingo

author maintainer
#31 lore raw

PATCH 9/12 x86/process: Pin the target stack in get_wchan()

In reply to Andy Lutomirski

This will prevent a crash if get_wchan() runs after the task stack is freed.

---
 arch/x86/kernel/process.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
arch/x86/kernel/process.c +15 -7
index 0b9ed8ec5226..4002b475171c 100644
@@ -532,15 +532,18 @@ unsigned long thread_saved_pc(struct task_struct *tsk)
  */
 unsigned long get_wchan(struct task_struct *p)
 {
-	unsigned long start, bottom, top, sp, fp, ip;
+	unsigned long start, bottom, top, sp, fp, ip, ret = 0;
 	int count = 0;
 
 	if (!p || p == current || p->state == TASK_RUNNING)
 		return 0;
 
+	if (!try_get_task_stack(p))
+		return 0;
+
 	start = (unsigned long)task_stack_page(p);
 	if (!start)
-		return 0;
+		goto out;
 
 	/*
 	 * Layout of the stack page:
@@ -564,16 +567,21 @@ unsigned long get_wchan(struct task_struct *p)
 
 	sp = READ_ONCE(p->thread.sp);
 	if (sp < bottom || sp > top)
-		return 0;
+		goto out;
 
 	fp = READ_ONCE_NOCHECK(((struct inactive_task_frame *)sp)->bp);
 	do {
 		if (fp < bottom || fp > top)
-			return 0;
+			goto out;
 		ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
-		if (!in_sched_functions(ip))
-			return ip;
+		if (!in_sched_functions(ip)) {
+			ret = ip;
+			goto out;
+		}
 		fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
 	} while (count++ < 16 && p->state != TASK_RUNNING);
-	return 0;
+
+out:
+	put_task_stack(p);
+	return ret;
 }
Signature · 1 line
2.7.4
#32 lore raw

Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan()

Quoted 2 lines · Andy Lutomirski

This will prevent a crash if get_wchan() runs after the task stack is freed.

I think I found some more stuff. Have a look at KSTK_EIP() and KSTK_ESP(), I think they read from the saved userspace registers area at the top of the kernel stack?

Used on remote processes in:
vma_is_stack_for_task() (via /proc/$pid/maps)
do_task_stat() (/proc/$pid/stat)

#33 lore raw

Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan()

Quoted 9 lines · Jann Horn
Quoted 2 lines · Andy Lutomirski

This will prevent a crash if get_wchan() runs after the task stack is freed.

I think I found some more stuff. Have a look at KSTK_EIP() and KSTK_ESP(), I think they read from the saved userspace registers area at the top of the kernel stack?

Used on remote processes in:
vma_is_stack_for_task() (via /proc/$pid/maps)

This isn't used in /proc/$pid/maps -- it's only used in /proc/$pid/task/$tid/maps. I wonder if anyone actually cares about it -- it certainly won't work reliably.

I could pin the stack in vma_is_stack_for_task, but it seems potentially better to me to change it to vma_is_stack_for_current() and remove the offending caller in /proc, replacing it with "return 0". Thoughts?

Quoted 1 line

do_task_stat() (/proc/$pid/stat)

Like this:

        mm = get_task_mm(task);
        if (mm) {
                vsize = task_vsize(mm);
                if (permitted) {
                        eip = KSTK_EIP(task);
                        esp = KSTK_ESP(task);
                }
        }

Can we just delete this outright? It seems somewhere between mostly and entirely useless, and it also seems dangerous. Until very recently, on x86_64, this would have been a potential info leak, as SYSCALL followed closely by a hardware interrupt would cause *kernel* values to land in task_pt_regs(). I don't even want to think about what this code does if the task is in vm86 mode. I wouldn't be at all surprised if non-x86 architectures have all kinds of interesting thinks happen if you do this to a task that isn't running normal non-atomic kernel code at the time.

I would advocate for unconditionally returning zeros in these two stat fields.

#34 lore raw

Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan()

Quoted 11 lines · Andy Lutomirski
Quoted 9 lines · Jann Horn
Quoted 2 lines · Andy Lutomirski

This will prevent a crash if get_wchan() runs after the task stack is freed.

I think I found some more stuff. Have a look at KSTK_EIP() and KSTK_ESP(), I think they read from the saved userspace registers area at the top of the kernel stack?

Used on remote processes in:
vma_is_stack_for_task() (via /proc/$pid/maps)

Quoted 8 lines

This isn't used in /proc/$pid/maps -- it's only used in /proc/$pid/task/$tid/maps. I wonder if anyone actually cares about it -- it certainly won't work reliably.

I could pin the stack in vma_is_stack_for_task, but it seems potentially better to me to change it to vma_is_stack_for_current() and remove the offending caller in /proc, replacing it with "return 0". Thoughts?

The history here is strange:

Before March 2012, we used to only consider the "stack" of the mm -- that is, the VMA that the mm actually treats as a stack. Then:

commit b76437579d1344b612cf1851ae610c636cec7db0

Author: Siddhesh Poyarekar <[email protected]>
Date:   Wed Mar 21 16:34:04 2012 -0700

procfs: mark thread stack correctly in proc/<pid>/maps

and we did something extra horrible to try to find out whose stack was where. This got partially reverted by:

commit 65376df582174ffcec9e6471bf5b0dd79ba05e4a

Author: Johannes Weiner <[email protected]>
Date:   Tue Feb 2 16:57:29 2016 -0800

proc: revert /proc/<pid>/maps [stack:TID] annotation

and now we're in the current situation where it's fast but still racy.

Any objection if I finish reverting the patch and restore the pre-2012 behavior? Frankly, I wouldn't mind trying to excise KSTK_EIP and KSTK_ESP from the kernel entirely, to be replaced with current_user_sp() and current_user_ip(). Having those macros around seems likely to make people think they're safe to use.

Quoted 24 lines
Quoted 2 lines

do_task_stat() (/proc/$pid/stat)

Like this:

        mm = get_task_mm(task);
        if (mm) {
                vsize = task_vsize(mm);
                if (permitted) {
                        eip = KSTK_EIP(task);
                        esp = KSTK_ESP(task);
                }
        }

Can we just delete this outright? It seems somewhere between mostly and entirely useless, and it also seems dangerous. Until very recently, on x86_64, this would have been a potential info leak, as SYSCALL followed closely by a hardware interrupt would cause *kernel* values to land in task_pt_regs(). I don't even want to think about what this code does if the task is in vm86 mode. I wouldn't be at all surprised if non-x86 architectures have all kinds of interesting thinks happen if you do this to a task that isn't running normal non-atomic kernel code at the time.

Quoted 1 line

I would advocate for unconditionally returning zeros in these two stat fields.

--Andy

#35 lore raw

Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan()

In reply to Andy Lutomirski
Quoted 11 lines · Andy Lutomirski
Quoted 9 lines · Jann Horn
Quoted 2 lines · Andy Lutomirski

This will prevent a crash if get_wchan() runs after the task stack is freed.

I think I found some more stuff. Have a look at KSTK_EIP() and KSTK_ESP(), I think they read from the saved userspace registers area at the top of the kernel stack?

Used on remote processes in:
vma_is_stack_for_task() (via /proc/$pid/maps)

Quoted 8 lines

This isn't used in /proc/$pid/maps -- it's only used in /proc/$pid/task/$tid/maps. I wonder if anyone actually cares about it -- it certainly won't work reliably.

I could pin the stack in vma_is_stack_for_task, but it seems potentially better to me to change it to vma_is_stack_for_current() and remove the offending caller in /proc, replacing it with "return 0". Thoughts?

I just scrolled through the debian codesearch results for "\[stack\]" - there seem to only be 105 across all of debian's packages, many of them duplicates - and I didn't see any that looked like they used the tid map. So I think this might work.

( https://codesearch.debian.net/search?q=%22%5C%5Bstack%5C%5D%22 )

Quoted 23 lines
Quoted 1 line

do_task_stat() (/proc/$pid/stat)

Like this:

        mm = get_task_mm(task);
        if (mm) {
                vsize = task_vsize(mm);
                if (permitted) {
                        eip = KSTK_EIP(task);
                        esp = KSTK_ESP(task);
                }
        }

Can we just delete this outright? It seems somewhere between mostly and entirely useless, and it also seems dangerous. Until very recently, on x86_64, this would have been a potential info leak, as SYSCALL followed closely by a hardware interrupt would cause *kernel* values to land in task_pt_regs(). I don't even want to think about what this code does if the task is in vm86 mode. I wouldn't be at all surprised if non-x86 architectures have all kinds of interesting thinks happen if you do this to a task that isn't running normal non-atomic kernel code at the time.

Quoted 1 line

I would advocate for unconditionally returning zeros in these two stat fields.

I'd like that a lot.

I guess the two things that might theoretically use it are ptrace users and (very theoretically) sampling profiling stuff or so?

In gdb, the only code I can find that reads this is in gdb/linux-nat.c, but it's behind an "#ifdef 0":

  #if 0   /* Don't know how architecture-dependent the rest is...
             Anyway the signal bitmap info is available from "status".  */
            if (fscanf (procfile, "%lu ", &ltmp) > 0)     /* FIXME arch?  */
              printf_filtered (_("Kernel stack pointer: 0x%lx\n"), ltmp);
            if (fscanf (procfile, "%lu ", &ltmp) > 0)     /* FIXME arch?  */
              printf_filtered (_("Kernel instr pointer: 0x%lx\n"), ltmp);

[...]

strace and ltrace don't seem to be using it.

#36 lore raw

Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan()

Quoted 68 lines · Jann Horn
Quoted 11 lines · Andy Lutomirski
Quoted 9 lines · Jann Horn
Quoted 2 lines · Andy Lutomirski

This will prevent a crash if get_wchan() runs after the task stack is freed.

I think I found some more stuff. Have a look at KSTK_EIP() and KSTK_ESP(), I think they read from the saved userspace registers area at the top of the kernel stack?

Used on remote processes in:
vma_is_stack_for_task() (via /proc/$pid/maps)

Quoted 8 lines

This isn't used in /proc/$pid/maps -- it's only used in /proc/$pid/task/$tid/maps. I wonder if anyone actually cares about it -- it certainly won't work reliably.

I could pin the stack in vma_is_stack_for_task, but it seems potentially better to me to change it to vma_is_stack_for_current() and remove the offending caller in /proc, replacing it with "return 0". Thoughts?

I just scrolled through the debian codesearch results for "\[stack\]" - there seem to only be 105 across all of debian's packages, many of them duplicates - and I didn't see any that looked like they used the tid map. So I think this might work.

( https://codesearch.debian.net/search?q=%22%5C%5Bstack%5C%5D%22 )

Quoted 23 lines
Quoted 1 line

do_task_stat() (/proc/$pid/stat)

Like this:

        mm = get_task_mm(task);
        if (mm) {
                vsize = task_vsize(mm);
                if (permitted) {
                        eip = KSTK_EIP(task);
                        esp = KSTK_ESP(task);
                }
        }

Can we just delete this outright? It seems somewhere between mostly and entirely useless, and it also seems dangerous. Until very recently, on x86_64, this would have been a potential info leak, as SYSCALL followed closely by a hardware interrupt would cause *kernel* values to land in task_pt_regs(). I don't even want to think about what this code does if the task is in vm86 mode. I wouldn't be at all surprised if non-x86 architectures have all kinds of interesting thinks happen if you do this to a task that isn't running normal non-atomic kernel code at the time.

Quoted 1 line

I would advocate for unconditionally returning zeros in these two stat fields.

I'd like that a lot.

I guess the two things that might theoretically use it are ptrace users and (very theoretically) sampling profiling stuff or so?

In gdb, the only code I can find that reads this is in gdb/linux-nat.c, but it's behind an "#ifdef 0":

  #if 0   /* Don't know how architecture-dependent the rest is...
             Anyway the signal bitmap info is available from "status".  */
            if (fscanf (procfile, "%lu ", &ltmp) > 0)     /* FIXME arch?  */
              printf_filtered (_("Kernel stack pointer: 0x%lx\n"), ltmp);
            if (fscanf (procfile, "%lu ", &ltmp) > 0)     /* FIXME arch?  */
              printf_filtered (_("Kernel instr pointer: 0x%lx\n"), ltmp);
Quoted 3 lines

[...]

strace and ltrace don't seem to be using it.

Does CRIU use this? I wouldn't expect so, since they're using ptrace, IIUC, to freeze/restore.

-Kees

Signature · 2 lines
Kees Cook
Nexus Security
#37 lore raw

Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan()

Quoted 71 lines · Kees Cook
Quoted 70 lines · Jann Horn
Quoted 11 lines · Andy Lutomirski
Quoted 9 lines · Jann Horn
Quoted 2 lines · Andy Lutomirski

This will prevent a crash if get_wchan() runs after the task stack is freed.

I think I found some more stuff. Have a look at KSTK_EIP() and KSTK_ESP(), I think they read from the saved userspace registers area at the top of the kernel stack?

Used on remote processes in:
vma_is_stack_for_task() (via /proc/$pid/maps)

Quoted 8 lines

This isn't used in /proc/$pid/maps -- it's only used in /proc/$pid/task/$tid/maps. I wonder if anyone actually cares about it -- it certainly won't work reliably.

I could pin the stack in vma_is_stack_for_task, but it seems potentially better to me to change it to vma_is_stack_for_current() and remove the offending caller in /proc, replacing it with "return 0". Thoughts?

I just scrolled through the debian codesearch results for "\[stack\]" - there seem to only be 105 across all of debian's packages, many of them duplicates - and I didn't see any that looked like they used the tid map. So I think this might work.

( https://codesearch.debian.net/search?q=%22%5C%5Bstack%5C%5D%22 )

Quoted 23 lines
Quoted 1 line

do_task_stat() (/proc/$pid/stat)

Like this:

        mm = get_task_mm(task);
        if (mm) {
                vsize = task_vsize(mm);
                if (permitted) {
                        eip = KSTK_EIP(task);
                        esp = KSTK_ESP(task);
                }
        }

Can we just delete this outright? It seems somewhere between mostly and entirely useless, and it also seems dangerous. Until very recently, on x86_64, this would have been a potential info leak, as SYSCALL followed closely by a hardware interrupt would cause *kernel* values to land in task_pt_regs(). I don't even want to think about what this code does if the task is in vm86 mode. I wouldn't be at all surprised if non-x86 architectures have all kinds of interesting thinks happen if you do this to a task that isn't running normal non-atomic kernel code at the time.

Quoted 1 line

I would advocate for unconditionally returning zeros in these two stat fields.

I'd like that a lot.

I guess the two things that might theoretically use it are ptrace users and (very theoretically) sampling profiling stuff or so?

In gdb, the only code I can find that reads this is in gdb/linux-nat.c, but it's behind an "#ifdef 0":

  #if 0   /* Don't know how architecture-dependent the rest is...
             Anyway the signal bitmap info is available from "status".  */
            if (fscanf (procfile, "%lu ", &ltmp) > 0)     /* FIXME arch?  */
              printf_filtered (_("Kernel stack pointer: 0x%lx\n"), ltmp);
            if (fscanf (procfile, "%lu ", &ltmp) > 0)     /* FIXME arch?  */
              printf_filtered (_("Kernel instr pointer: 0x%lx\n"), ltmp);

[...]

Quoted 4 lines
Quoted 1 line

strace and ltrace don't seem to be using it.

Does CRIU use this? I wouldn't expect so, since they're using ptrace, IIUC, to freeze/restore.

As far as I can tell:

parse_pid_stat() parses them into a struct proc_pid_stat as "esp" and "eip", but those struct members are never used (like, probably, most other members of that struct).

child_opened_proc.c just opens /proc/%d/stat and then closes it again immediately.

So in summary: I don't think so.

#38 lore raw

Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan()

Quoted 9 lines · Jann Horn
Quoted 2 lines · Kees Cook

Does CRIU use this? I wouldn't expect so, since they're using ptrace, IIUC, to freeze/restore.

As far as I can tell:

parse_pid_stat() parses them into a struct proc_pid_stat as "esp" and "eip", but those struct members are never used (like, probably, most other members of that struct).

Yes, that's my reading of it too.

Quoted 2 lines

child_opened_proc.c just opens /proc/%d/stat and then closes it again immediately.

This is just a test for ordering of things that are restored, and it could use any file in /proc, stat was just convenient.

Quoted 1 line

So in summary: I don't think so.

Yep, agreed.

Tycho

author maintainer
#39 lore raw

PATCH 10/12 lib/syscall: Pin the task stack in collect_syscall()

In reply to Andy Lutomirski

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

---
 lib/syscall.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
lib/syscall.c +13 -2
index e30e03932480..63239e097b13 100644
@@ -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;
 }
 
Signature · 1 line
2.7.4
author maintainer
#40 lore raw

PATCH 11/12 sched: Free the stack early if CONFIG_THREAD_INFO_IN_TASK

In reply to Andy Lutomirski

We currently keep every task's stack around until the task_struct itself is freed. This means that we keep the stack allocation alive for longer than necessary and that, under load, we free stacks in big batches whenever RCU drops the last task reference. Neither of these is good for reuse of cache-hot memory, and freeing in batches prevents us from usefully caching small numbers of vmalloced stacks.

On architectures that have thread_info on the stack, we can't easily change this, but on architectures that set THREAD_INFO_IN_TASK, we can free it as soon as the task is dead.

Cc Oleg Nesterov <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 include/linux/init_task.h |  4 +++-
 include/linux/sched.h     | 14 ++++++++++++++
 kernel/fork.c             | 35 ++++++++++++++++++++++++++++++++++-
 kernel/sched/core.c       |  4 ++++
 4 files changed, 55 insertions(+), 2 deletions(-)
include/linux/init_task.h +3 -1
index 9c04d44eeb3c..325f649d77ff 100644
@@ -186,7 +186,9 @@ extern struct task_group root_task_group;
 #endif
 
 #ifdef CONFIG_THREAD_INFO_IN_TASK
-# define INIT_TASK_TI(tsk) .thread_info = INIT_THREAD_INFO(tsk),
+# define INIT_TASK_TI(tsk)			\
+	.thread_info = INIT_THREAD_INFO(tsk),	\
+	.stack_refcount = ATOMIC_INIT(1),
 #else
 # define INIT_TASK_TI(tsk)
 #endif
include/linux/sched.h +14 -0
index a95867267e9f..abb795afc823 100644
@@ -1936,6 +1936,10 @@ struct task_struct {
 #ifdef CONFIG_VMAP_STACK
 	struct vm_struct *stack_vm_area;
 #endif
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	/* A live task holds one reference. */
+	atomic_t stack_refcount;
+#endif
 /* CPU-specific state of this task */
 	struct thread_struct thread;
 /*
@@ -3143,12 +3147,22 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
 
 #endif
 
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+static inline void *try_get_task_stack(struct task_struct *tsk)
+{
+	return atomic_inc_not_zero(&tsk->stack_refcount) ?
+		task_stack_page(tsk) : NULL;
+}
+
+extern void put_task_stack(struct task_struct *tsk);
+#else
 static inline void *try_get_task_stack(struct task_struct *tsk)
 {
 	return task_stack_page(tsk);
 }
 
 static inline void put_task_stack(struct task_struct *tsk) {}
+#endif
 
 #define task_stack_end_corrupted(task) \
 		(*(end_of_stack(task)) != STACK_END_MAGIC)
kernel/fork.c +34 -1
index 9b85f6b2cdcd..e0fd5446df58 100644
@@ -269,11 +269,40 @@ static void account_kernel_stack(struct task_struct *tsk, int account)
 	}
 }
 
-void free_task(struct task_struct *tsk)
+static void release_task_stack(struct task_struct *tsk)
 {
 	account_kernel_stack(tsk, -1);
 	arch_release_thread_stack(tsk->stack);
 	free_thread_stack(tsk);
+	tsk->stack = NULL;
+#ifdef CONFIG_VMAP_STACK
+	tsk->stack_vm_area = NULL;
+#endif
+}
+
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+void put_task_stack(struct task_struct *tsk)
+{
+	if (atomic_dec_and_test(&tsk->stack_refcount))
+		release_task_stack(tsk);
+}
+#endif
+
+void free_task(struct task_struct *tsk)
+{
+#ifndef CONFIG_THREAD_INFO_IN_TASK
+	/*
+	 * The task is finally done with both the stack and thread_info,
+	 * so free both.
+	 */
+	release_task_stack(tsk);
+#else
+	/*
+	 * If the task had a separate stack allocation, it should be gone
+	 * by now.
+	 */
+	WARN_ON_ONCE(atomic_read(&tsk->stack_refcount) != 0);
+#endif
 	rt_mutex_debug_task_free(tsk);
 	ftrace_graph_exit_task(tsk);
 	put_seccomp_filter(tsk);
@@ -411,6 +440,9 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 #ifdef CONFIG_VMAP_STACK
 	tsk->stack_vm_area = stack_vm_area;
 #endif
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	atomic_set(&tsk->stack_refcount, 1);
+#endif
 
 	if (err)
 		goto free_stack;
@@ -1750,6 +1782,7 @@ bad_fork_cleanup_count:
 	atomic_dec(&p->cred->user->processes);
 	exit_creds(p);
 bad_fork_free:
+	put_task_stack(p);
 	free_task(p);
 fork_out:
 	return ERR_PTR(retval);
kernel/sched/core.c +4 -0
index 3d91b63dd2f6..d61bfae91093 100644
@@ -2750,6 +2750,10 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 		 * task and put them back on the free list.
 		 */
 		kprobe_flush_task(prev);
+
+		/* Task is done with its stack. */
+		put_task_stack(prev);
+
 		put_task_struct(prev);
 	}
 
Signature · 1 line
2.7.4
author maintainer
#41 lore raw

PATCH 12/12 fork: Cache two thread stacks per cpu if CONFIG_VMAP_STACK is set

In reply to Andy Lutomirski

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.

---
 kernel/fork.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 9 deletions(-)
kernel/fork.c +53 -9
index e0fd5446df58..128ef350ce53 100644
@@ -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;
Signature · 1 line
2.7.4