[RFC 0/3] Baby steps toward cleaning up KERNEL_STACK_OFFSET

[RFC 0/3] Baby steps toward cleaning up KERNEL_STACK_OFFSET

[Andy Lutomirski]

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

-- 
2.1.0

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

[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
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 <luto@amacapital.net>
---
 arch/x86/include/asm/processor.h | 5 +++++
 arch/x86/lguest/boot.c           | 1 +
 arch/x86/xen/enlighten.c         | 1 +
 3 files changed, 7 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index a092a0cce0b7..477b1ff4a6c9 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -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
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index c1c1544b8485..77a609d1898d 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -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. */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 78a881b7fc41..d8195ca15346 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -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)
-- 
2.1.0

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

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

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

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

nit: s/On/on/

> 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

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

[Andy Lutomirski]

On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> 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?

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.

>
>> This needs minor paravirt tweaks to ensure that On native, sp0
>
> nit: s/On/on/
>
>> 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

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

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >> 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?
> 
> 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

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

[Andy Lutomirski]

On Wed, Mar 4, 2015 at 12:39 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Andy Lutomirski <luto@amacapital.net> wrote:
>> >
>> >> 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?
>>
>> 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

>
> Thanks,
>
>         Ingo



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[Borislav Petkov]

On Wed, Mar 04, 2015 at 12:41:35PM -0800, Andy Lutomirski wrote:
> 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

-- 
Regards/Gruss,
    Boris.

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

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

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, Mar 4, 2015 at 12:39 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >> >
> >> >> 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?
> >>
> >> 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.

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

/me gets sick of design patterns ;-)

Thanks,

	Ingo

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

[Andy Lutomirski]

On Wed, Mar 4, 2015 at 2:12 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Wed, Mar 4, 2015 at 12:39 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Andy Lutomirski <luto@amacapital.net> wrote:
>> >
>> >> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >
>> >> > * Andy Lutomirski <luto@amacapital.net> wrote:
>> >> >
>> >> >> 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?
>> >>
>> >> 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.
>
> 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

>
> Thanks,
>
>         Ingo



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[Andy Lutomirski]

This will make modifying the semantics of kernel_stack easier.

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index e82e95abc92b..92549053d86d 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -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;
 }
 
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c74f2f5652da..d287ea779728 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -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);
-- 
2.1.0

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

[Denys Vlasenko]

On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> This will make modifying the semantics of kernel_stack easier.
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/thread_info.h | 3 +--
>  arch/x86/kernel/traps.c            | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index e82e95abc92b..92549053d86d 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -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;
>  }
>  
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index c74f2f5652da..d287ea779728 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -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.

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

[Andy Lutomirski]

On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
>
> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> > This will make modifying the semantics of kernel_stack easier.
> >
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> > ---
> >  arch/x86/include/asm/thread_info.h | 3 +--
> >  arch/x86/kernel/traps.c            | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> > index e82e95abc92b..92549053d86d 100644
> > --- a/arch/x86/include/asm/thread_info.h
> > +++ b/arch/x86/include/asm/thread_info.h
> > @@ -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;
> >  }
> >
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index c74f2f5652da..d287ea779728 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -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.

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

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

[Denys Vlasenko]

On 02/27/2015 08:56 PM, Andy Lutomirski wrote:
> On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
>>
>> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
>>> This will make modifying the semantics of kernel_stack easier.
>>>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>>> ---
>>>  arch/x86/include/asm/thread_info.h | 3 +--
>>>  arch/x86/kernel/traps.c            | 2 +-
>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>>> index e82e95abc92b..92549053d86d 100644
>>> --- a/arch/x86/include/asm/thread_info.h
>>> +++ b/arch/x86/include/asm/thread_info.h
>>> @@ -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;
>>>  }
>>>
>>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>>> index c74f2f5652da..d287ea779728 100644
>>> --- a/arch/x86/kernel/traps.c
>>> +++ b/arch/x86/kernel/traps.c
>>> @@ -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
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Oops...
I meant to say "...the fact that this_cpu_sp0() points to the very top
of the stack, 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.

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

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

[Andy Lutomirski]

On Feb 27, 2015 1:12 PM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
>
> On 02/27/2015 08:56 PM, Andy Lutomirski wrote:
> > On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
> >>
> >> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> >>> This will make modifying the semantics of kernel_stack easier.
> >>>
> >>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>> Cc: Rusty Russell <rusty@rustcorp.com.au>
> >>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >>> ---
> >>>  arch/x86/include/asm/thread_info.h | 3 +--
> >>>  arch/x86/kernel/traps.c            | 2 +-
> >>>  2 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> >>> index e82e95abc92b..92549053d86d 100644
> >>> --- a/arch/x86/include/asm/thread_info.h
> >>> +++ b/arch/x86/include/asm/thread_info.h
> >>> @@ -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;
> >>>  }
> >>>
> >>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> >>> index c74f2f5652da..d287ea779728 100644
> >>> --- a/arch/x86/kernel/traps.c
> >>> +++ b/arch/x86/kernel/traps.c
> >>> @@ -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
>              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Oops...
> I meant to say "...the fact that this_cpu_sp0() points to the very top
> of the stack, 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.
>
> 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.
>

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

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

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

[Denys Vlasenko]

On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> This will make modifying the semantics of kernel_stack easier.
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/thread_info.h | 3 +--
>  arch/x86/kernel/traps.c            | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index e82e95abc92b..92549053d86d 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -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) {


-- 
vda

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

[Andy Lutomirski]

On Fri, Feb 27, 2015 at 8:52 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
>> This will make modifying the semantics of kernel_stack easier.
>>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  arch/x86/include/asm/thread_info.h | 3 +--
>>  arch/x86/kernel/traps.c            | 2 +-
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> index e82e95abc92b..92549053d86d 100644
>> --- a/arch/x86/include/asm/thread_info.h
>> +++ b/arch/x86/include/asm/thread_info.h
>> @@ -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) {
>
>

Whoops!  I fixed it locally with an EXPORT_SYMBOL_GPL.

--Andy

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

[Andy Lutomirski]

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 <luto@amacapital.net>
---
 arch/x86/ia32/ia32entry.S        | 3 +--
 arch/x86/kernel/asm-offsets_64.c | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index ed9746340363..719db63b35c4 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -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:
diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c
index fdcbb4d27c9f..5ce6f2da8763 100644
--- a/arch/x86/kernel/asm-offsets_64.c
+++ b/arch/x86/kernel/asm-offsets_64.c
@@ -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);
-- 
2.1.0