Skip to contentSkip to conversation

Baby steps toward cleaning up KERNEL_STACK_OFFSET

RFC Superseded Andy Lutomirski 4 participants 17 messages Feb 26, 2015 – Mar 4, 2015

Subsystems (2)
x86 Xen hypervisor x86

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Andy Lutomirski

author
#0 lore raw

cover Baby steps toward cleaning up KERNEL_STACK_OFFSET

Denys is right that KERNEL_STACK_OFFSET is a mess. Let's start fixing it.

This removes all C code that *reads* kernel_stack. It also fixes the KERNEL_STACK_OFFSET abomination in ia32_sysenter_target.

It does not fix the KERNEL_STACK_OFFSET abomination in GET_THREAD_INFO and THREAD_INFO. I think that should be its own patch.

It also doesn't change the two syscall targets. To fix them, we should make a decision. Either we should make KERNEL_STACK_OFFSET have the correct nonzero value to save an instruction or we should get rid of kernel_stack entirely.

Andy Lutomirski (3):
x86: Add this_cpu_sp0() to read sp0 for the current cpu
x86: Switch all C consumers of kernel_stack to this_cpu_sp0 x86, asm: Change the 32-bit sysenter code to use sp0

 arch/x86/ia32/ia32entry.S          | 3 +--
 arch/x86/include/asm/processor.h   | 5 +++++
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/asm-offsets_64.c   | 1 +
 arch/x86/kernel/traps.c            | 2 +-
 arch/x86/lguest/boot.c             | 1 +
 arch/x86/xen/enlighten.c           | 1 +
 7 files changed, 11 insertions(+), 5 deletions(-)
Signature · 1 line
2.1.0
author
#1 lore raw

RFC 1/3 x86: Add this_cpu_sp0() to read sp0 for the current cpu

We currently store references to the top of the kernel stack in multiple places: kernel_stack (with an offset) and init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware and is a clean canonical way to find the top of the stack. Add an accessor so we can start using it.

This needs minor paravirt tweaks to ensure that On native, sp0 defines the top of the kernel stack. On Xen and lguest, the hypervisor tracks it, but we want to start reading sp0 in the kernel. Fixing this is simple: just update our local copy of sp0 as well as the hypervisor's copy on task switches.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/processor.h | 5 +++++
 arch/x86/lguest/boot.c           | 1 +
 arch/x86/xen/enlighten.c         | 1 +
 3 files changed, 7 insertions(+)
arch/x86/include/asm/processor.h +5 -0
index a092a0cce0b7..477b1ff4a6c9 100644
@@ -564,6 +564,11 @@ static inline void native_swapgs(void)
 #endif
 }
 
+static inline unsigned long this_cpu_sp0(void)
+{
+	return this_cpu_read_stable(init_tss.x86_tss.sp0);
+}
+
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
 #else
arch/x86/lguest/boot.c +1 -0
index c1c1544b8485..77a609d1898d 100644
@@ -1053,6 +1053,7 @@ static void lguest_load_sp0(struct tss_struct *tss,
 {
 	lazy_hcall3(LHCALL_SET_STACK, __KERNEL_DS | 0x1, thread->sp0,
 		   THREAD_SIZE / PAGE_SIZE);
+	tss->x86_tss.sp0 = sp0;
 }
 
 /* Let's just say, I wouldn't do debugging under a Guest. */
arch/x86/xen/enlighten.c +1 -0
index 78a881b7fc41..d8195ca15346 100644
@@ -912,6 +912,7 @@ static void xen_load_sp0(struct tss_struct *tss,
 	mcs = xen_mc_entry(0);
 	MULTI_stack_switch(mcs.mc, __KERNEL_DS, thread->sp0);
 	xen_mc_issue(PARAVIRT_LAZY_CPU);
+	tss->x86_tss.sp0 = thread->sp0;
 }
 
 static void xen_set_iopl_mask(unsigned mask)
Signature · 1 line
2.1.0
#2 lore raw

Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu

Quoted 4 lines · Andy Lutomirski

We currently store references to the top of the kernel stack in multiple places: kernel_stack (with an offset) and init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware and is a clean canonical way to find the top of the stack. Add an

Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as there's nothing 'init' about it anymore - we load it during CPU init and then manually maintain its contents. A better name would be 'current_tss' - referring to both the current CPU and the current task?

Quoted 1 line

This needs minor paravirt tweaks to ensure that On native, sp0

nit: s/On/on/

Quoted 4 lines

defines the top of the kernel stack. On Xen and lguest, the hypervisor tracks it, but we want to start reading sp0 in the kernel. Fixing this is simple: just update our local copy of sp0 as well as the hypervisor's copy on task switches.

Thanks,

Ingo

author
#3 lore raw

Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu

Quoted 8 lines · Ingo Molnar
Quoted 4 lines · Andy Lutomirski

We currently store references to the top of the kernel stack in multiple places: kernel_stack (with an offset) and init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware and is a clean canonical way to find the top of the stack. Add an

Quoted 5 lines

Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as there's nothing 'init' about it anymore - we load it during CPU init and then manually maintain its contents. A better name would be 'current_tss' - referring to both the current CPU and the current task?

Hmm. That seems a little odd to me, since we never change the TSS pointer. It's certainly better than init_tss, though. I'll add a followup to rename it.

Quoted 9 lines
Quoted 2 lines

This needs minor paravirt tweaks to ensure that On native, sp0

nit: s/On/on/

Quoted 4 lines

defines the top of the kernel stack. On Xen and lguest, the hypervisor tracks it, but we want to start reading sp0 in the kernel. Fixing this is simple: just update our local copy of sp0 as well as the hypervisor's copy on task switches.

Thanks,
Andy

#4 lore raw

Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu

Quoted 15 lines · Andy Lutomirski
Quoted 8 lines · Ingo Molnar
Quoted 4 lines · Andy Lutomirski

We currently store references to the top of the kernel stack in multiple places: kernel_stack (with an offset) and init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware and is a clean canonical way to find the top of the stack. Add an

Quoted 5 lines

Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as there's nothing 'init' about it anymore - we load it during CPU init and then manually maintain its contents. A better name would be 'current_tss' - referring to both the current CPU and the current task?

Quoted 3 lines

Hmm. That seems a little odd to me, since we never change the TSS pointer. It's certainly better than init_tss, though. I'll add a followup to rename it.

Alternatively we could use 'cpu_tss': the CPU's current TSS.

Thanks,

Ingo

author
#5 lore raw

Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu

Quoted 18 lines · Ingo Molnar
Quoted 15 lines · Andy Lutomirski
Quoted 13 lines · Ingo Molnar
Quoted 4 lines · Andy Lutomirski

We currently store references to the top of the kernel stack in multiple places: kernel_stack (with an offset) and init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware and is a clean canonical way to find the top of the stack. Add an

Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as there's nothing 'init' about it anymore - we load it during CPU init and then manually maintain its contents. A better name would be 'current_tss' - referring to both the current CPU and the current task?

Quoted 5 lines
Quoted 3 lines

Hmm. That seems a little odd to me, since we never change the TSS pointer. It's certainly better than init_tss, though. I'll add a followup to rename it.

Alternatively we could use 'cpu_tss': the CPU's current TSS.

I painted my bikeshed "singleton_tss", since cpu_tss seemed redundant for something that's already per cpu. If you prefer your bikeshed color, let me know.

--Andy

Quoted 4 lines

Thanks,

Ingo

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

Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu

Quoted 3 lines · Andy Lutomirski

I painted my bikeshed "singleton_tss", since cpu_tss seemed redundant for something that's already per cpu. If you prefer your bikeshed color, let me know.

I want "pink_pony_tss".

:-P

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
#7 lore raw

Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu

In reply to Andy Lutomirski
Quoted 23 lines · Andy Lutomirski
Quoted 22 lines · Ingo Molnar
Quoted 18 lines · Andy Lutomirski
Quoted 8 lines · Ingo Molnar
Quoted 4 lines · Andy Lutomirski

We currently store references to the top of the kernel stack in multiple places: kernel_stack (with an offset) and init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware and is a clean canonical way to find the top of the stack. Add an

Quoted 5 lines

Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as there's nothing 'init' about it anymore - we load it during CPU init and then manually maintain its contents. A better name would be 'current_tss' - referring to both the current CPU and the current task?

Hmm. That seems a little odd to me, since we never change the TSS pointer. It's certainly better than init_tss, though. I'll add a followup to rename it.

Quoted 5 lines
Quoted 1 line

Alternatively we could use 'cpu_tss': the CPU's current TSS.

I painted my bikeshed "singleton_tss", since cpu_tss seemed redundant for something that's already per cpu. If you prefer your bikeshed color, let me know.

Yeah, name it *anything* but 'singleton'!!

/me gets sick of design patterns ;-)

Thanks,

Ingo

author
#8 lore raw

Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu

Quoted 28 lines · Ingo Molnar
Quoted 25 lines · Andy Lutomirski
Quoted 23 lines · Ingo Molnar
Quoted 15 lines · Andy Lutomirski
Quoted 8 lines · Ingo Molnar
Quoted 4 lines · Andy Lutomirski

We currently store references to the top of the kernel stack in multiple places: kernel_stack (with an offset) and init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware and is a clean canonical way to find the top of the stack. Add an

Quoted 5 lines

Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as there's nothing 'init' about it anymore - we load it during CPU init and then manually maintain its contents. A better name would be 'current_tss' - referring to both the current CPU and the current task?

Quoted 3 lines

Hmm. That seems a little odd to me, since we never change the TSS pointer. It's certainly better than init_tss, though. I'll add a followup to rename it.

Alternatively we could use 'cpu_tss': the CPU's current TSS.

Quoted 7 lines
Quoted 3 lines

I painted my bikeshed "singleton_tss", since cpu_tss seemed redundant for something that's already per cpu. If you prefer your bikeshed color, let me know.

Yeah, name it *anything* but 'singleton'!!

/me gets sick of design patterns ;-)

My tss factory laughs at your silly ideas :)

cpu_tss it is.

--Andy

Quoted 4 lines

Thanks,

Ingo

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
author
#9 lore raw

RFC 2/3 x86: Switch all C consumers of kernel_stack to this_cpu_sp0

In reply to Andy Lutomirski

This will make modifying the semantics of kernel_stack easier.

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc Rusty Russell <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/thread_info.h +1 -2
index e82e95abc92b..92549053d86d 100644
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
 	struct thread_info *ti;
-	ti = (void *)(this_cpu_read_stable(kernel_stack) +
-		      KERNEL_STACK_OFFSET - THREAD_SIZE);
+	ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
 	return ti;
 }
arch/x86/kernel/traps.c +1 -1
index c74f2f5652da..d287ea779728 100644
@@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
 	 * will catch asm bugs and any attempt to use ist_preempt_enable
 	 * from double_fault.
 	 */
-	BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+	BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
 		& ~(THREAD_SIZE - 1)) != 0);
 
 	preempt_count_sub(HARDIRQ_OFFSET);
Signature · 1 line
2.1.0
author
#10 lore raw

RFC 3/3 x86, asm: Change the 32-bit sysenter code to use sp0

The ia32 sysenter code loaded the top of the kernel stack into rsp by loading kernel_stack and then adjusting it. It can be simplified to just read sp0 directly.

This requires the addition of a new asm-offsets entry for sp0.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/ia32/ia32entry.S        | 3 +--
 arch/x86/kernel/asm-offsets_64.c | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/ia32/ia32entry.S +1 -2
index ed9746340363..719db63b35c4 100644
@@ -113,8 +113,7 @@ ENTRY(ia32_sysenter_target)
 	CFI_DEF_CFA	rsp,0
 	CFI_REGISTER	rsp,rbp
 	SWAPGS_UNSAFE_STACK
-	movq	PER_CPU_VAR(kernel_stack), %rsp
-	addq	$(KERNEL_STACK_OFFSET),%rsp
+	movq	PER_CPU_VAR(init_tss + TSS_sp0), %rsp
 	/*
 	 * No need to follow this irqs on/off section: the syscall
 	 * disabled irqs, here we enable it straight after entry:
arch/x86/kernel/asm-offsets_64.c +1 -0
index fdcbb4d27c9f..5ce6f2da8763 100644
@@ -81,6 +81,7 @@ int main(void)
 #undef ENTRY
 
 	OFFSET(TSS_ist, tss_struct, x86_tss.ist);
+	OFFSET(TSS_sp0, tss_struct, x86_tss.sp0);
 	BLANK();
 
 	DEFINE(__NR_syscall_max, sizeof(syscalls_64) - 1);
Signature · 1 line
2.1.0
#11 lore raw

Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0

In reply to Andy Lutomirski
Quoted 30 lines · Andy Lutomirski

This will make modifying the semantics of kernel_stack easier.

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc Rusty Russell <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/thread_info.h +1 -2
index e82e95abc92b..92549053d86d 100644
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
 	struct thread_info *ti;
-	ti = (void *)(this_cpu_read_stable(kernel_stack) +
-		      KERNEL_STACK_OFFSET - THREAD_SIZE);
+	ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
 	return ti;
 }
arch/x86/kernel/traps.c +0 -0
index c74f2f5652da..d287ea779728 100644

arch/x86/kernel/traps.c void ist_begin_non_atomic(struct pt_regs *regs) · line 178

Quoted 6 lines
arch/x86/kernel/traps.c +1 -1
@@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
 	 * will catch asm bugs and any attempt to use ist_preempt_enable
 	 * from double_fault.
 	 */
-	BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+	BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
 		& ~(THREAD_SIZE - 1)) != 0);

While we are at it, I propose a more readable version of this check:

BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);

Yes, I am aware that it is not equivalent to the existing condition
- it uses the fact that this_cpu_sp0(), previous check wasn't making that assumption. But that assumption is true, so shouldn't be a problem.

author
#12 lore raw

Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0

Quoted 41 lines · Denys Vlasenko
Quoted 30 lines · Andy Lutomirski

This will make modifying the semantics of kernel_stack easier.

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc Rusty Russell <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/thread_info.h +1 -2
index e82e95abc92b..92549053d86d 100644
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
      struct thread_info *ti;
-     ti = (void *)(this_cpu_read_stable(kernel_stack) +
-                   KERNEL_STACK_OFFSET - THREAD_SIZE);
+     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
      return ti;
 }
arch/x86/kernel/traps.c +0 -0
index c74f2f5652da..d287ea779728 100644

arch/x86/kernel/traps.c void ist_begin_non_atomic(struct pt_regs *regs) · line 178

Quoted 6 lines
arch/x86/kernel/traps.c +1 -1
@@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
       * will catch asm bugs and any attempt to use ist_preempt_enable
       * from double_fault.
       */
-     BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+     BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
              & ~(THREAD_SIZE - 1)) != 0);

While we are at it, I propose a more readable version of this check:

Quoted 6 lines

BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);

Yes, I am aware that it is not equivalent to the existing condition
- it uses the fact that this_cpu_sp0(), previous check wasn't making that assumption. But that assumption is true, so shouldn't be a problem.

You're missing an absolute value in here, though. This isn't a check for overflow; it's a check that we aren't on an IST or other per cpu stack.

--Andy

#13 lore raw

Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0

Quoted 42 lines · Andy Lutomirski
Quoted 41 lines · Denys Vlasenko
Quoted 30 lines · Andy Lutomirski

This will make modifying the semantics of kernel_stack easier.

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc Rusty Russell <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/thread_info.h +1 -2
index e82e95abc92b..92549053d86d 100644
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
      struct thread_info *ti;
-     ti = (void *)(this_cpu_read_stable(kernel_stack) +
-                   KERNEL_STACK_OFFSET - THREAD_SIZE);
+     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
      return ti;
 }
arch/x86/kernel/traps.c +0 -0
index c74f2f5652da..d287ea779728 100644

arch/x86/kernel/traps.c void ist_begin_non_atomic(struct pt_regs *regs) · line 178

Quoted 6 lines
arch/x86/kernel/traps.c +1 -1
@@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
       * will catch asm bugs and any attempt to use ist_preempt_enable
       * from double_fault.
       */
-     BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+     BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
              & ~(THREAD_SIZE - 1)) != 0);

While we are at it, I propose a more readable version of this check:

Quoted 4 lines
Quoted 4 lines

BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);

Yes, I am aware that it is not equivalent to the existing condition
- it uses the fact that this_cpu_sp0(), previous check

             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Oops...
I meant to say "...the fact that this_cpu_sp0() points to the very top of the stack, previous check ..."

Quoted 6 lines
Quoted 2 lines

wasn't making that assumption. But that assumption is true, so shouldn't be a problem.

You're missing an absolute value in here, though. This isn't a check for overflow; it's a check that we aren't on an IST or other per cpu stack.

Yes, that's exactly what the condition checks for. It reads

"is current stack pointer below task's kernel stack by no more than THREAD_SIZE?"

which is only possible if we are on task's kernel stack:

If current_stack_pointer() is elsewhere, it is either
(a) much smaller than this_cpu_sp0(), and BUG_ON condition obviously triggers; or
(b) it is somewhere above this_cpu_sp0(), in which case subtraction overflows and condition triggers too.

How would you write this condition so that it's easily readable? Evidently, my version isn't as readable sa I hoped...

author
#14 lore raw

Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0

Quoted 66 lines · Denys Vlasenko
Quoted 42 lines · Andy Lutomirski
Quoted 41 lines · Denys Vlasenko
Quoted 30 lines · Andy Lutomirski

This will make modifying the semantics of kernel_stack easier.

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc Rusty Russell <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/thread_info.h +1 -2
index e82e95abc92b..92549053d86d 100644
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
      struct thread_info *ti;
-     ti = (void *)(this_cpu_read_stable(kernel_stack) +
-                   KERNEL_STACK_OFFSET - THREAD_SIZE);
+     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
      return ti;
 }
arch/x86/kernel/traps.c +0 -0
index c74f2f5652da..d287ea779728 100644

arch/x86/kernel/traps.c void ist_begin_non_atomic(struct pt_regs *regs) · line 178

Quoted 6 lines
arch/x86/kernel/traps.c +1 -1
@@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
       * will catch asm bugs and any attempt to use ist_preempt_enable
       * from double_fault.
       */
-     BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+     BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
              & ~(THREAD_SIZE - 1)) != 0);

While we are at it, I propose a more readable version of this check:

Quoted 4 lines
Quoted 4 lines

BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);

Yes, I am aware that it is not equivalent to the existing condition
- it uses the fact that this_cpu_sp0(), previous check

             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Oops...
I meant to say "...the fact that this_cpu_sp0() points to the very top of the stack, previous check ..."

Quoted 6 lines
Quoted 2 lines

wasn't making that assumption. But that assumption is true, so shouldn't be a problem.

You're missing an absolute value in here, though. This isn't a check for overflow; it's a check that we aren't on an IST or other per cpu stack.

Yes, that's exactly what the condition checks for. It reads

"is current stack pointer below task's kernel stack by no more than THREAD_SIZE?"

Quoted 8 lines

which is only possible if we are on task's kernel stack:

If current_stack_pointer() is elsewhere, it is either
(a) much smaller than this_cpu_sp0(), and BUG_ON condition obviously triggers; or
(b) it is somewhere above this_cpu_sp0(), in which case subtraction overflows and condition triggers too.

Right, it's unsigned. I like yours better if I add a comment. It also avoids relying on the stack being aligned to its full size.

--Andy

Quoted 2 lines

How would you write this condition so that it's easily readable? Evidently, my version isn't as readable sa I hoped...

#15 lore raw

Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0

In reply to Andy Lutomirski
Quoted 16 lines · Andy Lutomirski

This will make modifying the semantics of kernel_stack easier.

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc Rusty Russell <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/thread_info.h +0 -0
index e82e95abc92b..92549053d86d 100644

arch/x86/include/asm/thread_info.h DECLARE_PER_CPU(unsigned long, kernel_stack); · line 168

Quoted 8 lines
arch/x86/include/asm/thread_info.h +1 -2
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
 	struct thread_info *ti;
-	ti = (void *)(this_cpu_read_stable(kernel_stack) +
-		      KERNEL_STACK_OFFSET - THREAD_SIZE);
+	ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
 	return ti;
 }

This makes modpost unhappy, it says these modules need init_tss symbol to be visible to them:

Building modules, stage 2.
MODPOST 2556 modules
ERROR: "init_tss" [sound/usb/usx2y/snd-usb-usx2y.ko] undefined!
ERROR: "init_tss" [sound/pci/hda/snd-hda-codec.ko] undefined!
ERROR: "init_tss" [sound/pci/emu10k1/snd-emu10k1.ko] undefined!
ERROR: "init_tss" [sound/core/snd-pcm.ko] undefined!
ERROR: "init_tss" [sound/core/snd-hwdep.ko] undefined!
ERROR: "init_tss" [sound/core/seq/snd-seq.ko] undefined!
ERROR: "init_tss" [sound/core/oss/snd-pcm-oss.ko] undefined!
ERROR: "init_tss" [net/sunrpc/sunrpc.ko] undefined!
ERROR: "init_tss" [net/sctp/sctp.ko] undefined!
ERROR: "init_tss" [net/rds/rds_tcp.ko] undefined!
ERROR: "init_tss" [net/core/pktgen.ko] undefined!
ERROR: "init_tss" [net/batman-adv/batman-adv.ko] undefined!
ERROR: "init_tss" [net/9p/9pnet.ko] undefined!
ERROR: "init_tss" [fs/reiserfs/reiserfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/dlmfs/ocfs2_dlmfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/cluster/ocfs2_nodemanager.ko] undefined!
ERROR: "init_tss" [fs/nfsd/nfsd.ko] undefined!
ERROR: "init_tss" [fs/fscache/fscache.ko] undefined!
ERROR: "init_tss" [fs/fat/fat.ko] undefined!
ERROR: "init_tss" [fs/ecryptfs/ecryptfs.ko] undefined!
ERROR: "init_tss" [fs/cachefiles/cachefiles.ko] undefined!
ERROR: "init_tss" [fs/btrfs/btrfs.ko] undefined!
ERROR: "init_tss" [fs/9p/9p.ko] undefined!
ERROR: "init_tss" [drivers/xen/xen-privcmd.ko] undefined!
ERROR: "init_tss" [drivers/vhost/vhost.ko] undefined!
ERROR: "init_tss" [drivers/tty/n_hdlc.ko] undefined!
ERROR: "init_tss" [drivers/target/target_core_file.ko] undefined!
ERROR: "init_tss" [drivers/scsi/pmcraid.ko] undefined!
ERROR: "init_tss" [drivers/scsi/lpfc/lpfc.ko] undefined!
ERROR: "init_tss" [drivers/parport/parport.ko] undefined!
ERROR: "init_tss" [drivers/net/ethernet/broadcom/tg3.ko] undefined!
ERROR: "init_tss" [drivers/net/bonding/bonding.ko] undefined!
ERROR: "init_tss" [drivers/mtd/mtd.ko] undefined!
ERROR: "init_tss" [drivers/misc/vmw_vmci/vmw_vmci.ko] undefined!
ERROR: "init_tss" [drivers/media/v4l2-core/videodev.ko] undefined!
ERROR: "init_tss" [drivers/media/usb/uvc/uvcvideo.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/saa7134/saa7134.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/ivtv/ivtvfb.ko] undefined!
ERROR: "init_tss" [drivers/media/i2c/vpx3220.ko] undefined!
ERROR: "init_tss" [drivers/md/bcache/bcache.ko] undefined!
ERROR: "init_tss" [drivers/isdn/i4l/isdn.ko] undefined!
ERROR: "init_tss" [drivers/input/misc/uinput.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/qib/ib_qib.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/ipath/ib_ipath.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/core/ib_uverbs.ko] undefined!
ERROR: "init_tss" [drivers/hid/uhid.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/radeon/radeon.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/qxl/qxl.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/i915/i915.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "init_tss" [drivers/firewire/firewire-core.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm_tis.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "init_tss" [drivers/char/pcmcia/cm4000_cs.ko] undefined!
ERROR: "init_tss" [drivers/char/lp.ko] undefined!
ERROR: "init_tss" [drivers/char/ipmi/ipmi_devintf.ko] undefined!
ERROR: "init_tss" [drivers/block/drbd/drbd.ko] undefined!
ERROR: "init_tss" [drivers/acpi/acpi_pad.ko] undefined!
ERROR: "init_tss" [arch/x86/oprofile/oprofile.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm-intel.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

I took one and investigated how it references init_tss.

drivers/hid/uhid.c:

static int uhid_event_from_user(const char __user *buffer, size_t len,
struct uhid_event *event)

{
        if (is_compat_task()) {   <=========== HERE
                u32 type;

                if (get_user(type, buffer))
                        return -EFAULT;

                if (type == UHID_CREATE) {
Signature · 1 line
vda
author
#16 lore raw

Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0

Quoted 101 lines · Denys Vlasenko
Quoted 16 lines · Andy Lutomirski

This will make modifying the semantics of kernel_stack easier.

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc Rusty Russell <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/thread_info.h +0 -0
index e82e95abc92b..92549053d86d 100644

arch/x86/include/asm/thread_info.h DECLARE_PER_CPU(unsigned long, kernel_stack); · line 168

Quoted 8 lines
arch/x86/include/asm/thread_info.h +1 -2
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
      struct thread_info *ti;
-     ti = (void *)(this_cpu_read_stable(kernel_stack) +
-                   KERNEL_STACK_OFFSET - THREAD_SIZE);
+     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
      return ti;
 }

This makes modpost unhappy, it says these modules need init_tss symbol to be visible to them:

Building modules, stage 2.
MODPOST 2556 modules
ERROR: "init_tss" [sound/usb/usx2y/snd-usb-usx2y.ko] undefined!
ERROR: "init_tss" [sound/pci/hda/snd-hda-codec.ko] undefined!
ERROR: "init_tss" [sound/pci/emu10k1/snd-emu10k1.ko] undefined!
ERROR: "init_tss" [sound/core/snd-pcm.ko] undefined!
ERROR: "init_tss" [sound/core/snd-hwdep.ko] undefined!
ERROR: "init_tss" [sound/core/seq/snd-seq.ko] undefined!
ERROR: "init_tss" [sound/core/oss/snd-pcm-oss.ko] undefined!
ERROR: "init_tss" [net/sunrpc/sunrpc.ko] undefined!
ERROR: "init_tss" [net/sctp/sctp.ko] undefined!
ERROR: "init_tss" [net/rds/rds_tcp.ko] undefined!
ERROR: "init_tss" [net/core/pktgen.ko] undefined!
ERROR: "init_tss" [net/batman-adv/batman-adv.ko] undefined!
ERROR: "init_tss" [net/9p/9pnet.ko] undefined!
ERROR: "init_tss" [fs/reiserfs/reiserfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/dlmfs/ocfs2_dlmfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/cluster/ocfs2_nodemanager.ko] undefined!
ERROR: "init_tss" [fs/nfsd/nfsd.ko] undefined!
ERROR: "init_tss" [fs/fscache/fscache.ko] undefined!
ERROR: "init_tss" [fs/fat/fat.ko] undefined!
ERROR: "init_tss" [fs/ecryptfs/ecryptfs.ko] undefined!
ERROR: "init_tss" [fs/cachefiles/cachefiles.ko] undefined!
ERROR: "init_tss" [fs/btrfs/btrfs.ko] undefined!
ERROR: "init_tss" [fs/9p/9p.ko] undefined!
ERROR: "init_tss" [drivers/xen/xen-privcmd.ko] undefined!
ERROR: "init_tss" [drivers/vhost/vhost.ko] undefined!
ERROR: "init_tss" [drivers/tty/n_hdlc.ko] undefined!
ERROR: "init_tss" [drivers/target/target_core_file.ko] undefined!
ERROR: "init_tss" [drivers/scsi/pmcraid.ko] undefined!
ERROR: "init_tss" [drivers/scsi/lpfc/lpfc.ko] undefined!
ERROR: "init_tss" [drivers/parport/parport.ko] undefined!
ERROR: "init_tss" [drivers/net/ethernet/broadcom/tg3.ko] undefined!
ERROR: "init_tss" [drivers/net/bonding/bonding.ko] undefined!
ERROR: "init_tss" [drivers/mtd/mtd.ko] undefined!
ERROR: "init_tss" [drivers/misc/vmw_vmci/vmw_vmci.ko] undefined!
ERROR: "init_tss" [drivers/media/v4l2-core/videodev.ko] undefined!
ERROR: "init_tss" [drivers/media/usb/uvc/uvcvideo.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/saa7134/saa7134.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/ivtv/ivtvfb.ko] undefined!
ERROR: "init_tss" [drivers/media/i2c/vpx3220.ko] undefined!
ERROR: "init_tss" [drivers/md/bcache/bcache.ko] undefined!
ERROR: "init_tss" [drivers/isdn/i4l/isdn.ko] undefined!
ERROR: "init_tss" [drivers/input/misc/uinput.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/qib/ib_qib.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/ipath/ib_ipath.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/core/ib_uverbs.ko] undefined!
ERROR: "init_tss" [drivers/hid/uhid.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/radeon/radeon.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/qxl/qxl.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/i915/i915.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "init_tss" [drivers/firewire/firewire-core.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm_tis.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "init_tss" [drivers/char/pcmcia/cm4000_cs.ko] undefined!
ERROR: "init_tss" [drivers/char/lp.ko] undefined!
ERROR: "init_tss" [drivers/char/ipmi/ipmi_devintf.ko] undefined!
ERROR: "init_tss" [drivers/block/drbd/drbd.ko] undefined!
ERROR: "init_tss" [drivers/acpi/acpi_pad.ko] undefined!
ERROR: "init_tss" [arch/x86/oprofile/oprofile.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm-intel.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

I took one and investigated how it references init_tss.

drivers/hid/uhid.c:

static int uhid_event_from_user(const char __user *buffer, size_t len,
struct uhid_event *event)

Quoted 10 lines
{
        if (is_compat_task()) {   <=========== HERE
                u32 type;

                if (get_user(type, buffer))
                        return -EFAULT;

                if (type == UHID_CREATE) {

Whoops! I fixed it locally with an EXPORT_SYMBOL_GPL.

--Andy