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

Groups > linux.kernel > #1351269 > unrolled thread

[PATCH 2/2] x86/entry: Do enter_from_user_mode with IRQs off

Started byAndy Lutomirski <luto@kernel.org>
First post2016-03-07 03:40 +0100
Last post2016-03-08 18:30 +0100
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] x86/entry: Do enter_from_user_mode with IRQs off Andy Lutomirski <luto@kernel.org> - 2016-03-07 03:40 +0100
    Re: [PATCH 2/2] x86/entry: Do enter_from_user_mode with IRQs off Borislav Petkov <bp@alien8.de> - 2016-03-08 14:50 +0100
      Re: [PATCH 2/2] x86/entry: Do enter_from_user_mode with IRQs off Andy Lutomirski <luto@amacapital.net> - 2016-03-08 18:30 +0100

#1351269 — [PATCH 2/2] x86/entry: Do enter_from_user_mode with IRQs off

FromAndy Lutomirski <luto@kernel.org>
Date2016-03-07 03:40 +0100
Subject[PATCH 2/2] x86/entry: Do enter_from_user_mode with IRQs off
Message-ID<r9Tfs-576-5@gated-at.bofh.it>
Now that slow-path syscalls always enter C before enabling
interrupts, it's straightforward to do enter_from_user_mode before
enabling interrupts rather than doing it as part of entry tracing.

With this change, we should finally be able to retire
exception_enter.

This will also enable optimizations based on knowing that we never
change context tracking state with interrupts on.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/entry/common.c            | 39 ++++++++++++++------------------------
 arch/x86/include/asm/thread_info.h |  5 ++++-
 2 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 9ef3258320ec..8e8ca0eb36be 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -38,14 +38,17 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
 	return (struct thread_info *)(top_of_stack - THREAD_SIZE);
 }
 
-#ifdef CONFIG_CONTEXT_TRACKING
+#ifndef CONFIG_CONTEXT_TRACKING
+static
+#else
+__visible
+#endif
 /* Called on entry from user mode with IRQs off. */
-__visible void enter_from_user_mode(void)
+void enter_from_user_mode(void)
 {
 	CT_WARN_ON(ct_state() != CONTEXT_USER);
 	user_exit();
 }
-#endif
 
 static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
 {
@@ -85,17 +88,6 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
 
 	work = ACCESS_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
 
-#ifdef CONFIG_CONTEXT_TRACKING
-	/*
-	 * If TIF_NOHZ is set, we are required to call user_exit() before
-	 * doing anything that could touch RCU.
-	 */
-	if (work & _TIF_NOHZ) {
-		enter_from_user_mode();
-		work &= ~_TIF_NOHZ;
-	}
-#endif
-
 #ifdef CONFIG_SECCOMP
 	/*
 	 * Do seccomp first -- it should minimize exposure of other
@@ -354,6 +346,7 @@ __visible void do_syscall_64(struct pt_regs *regs)
 	struct thread_info *ti = pt_regs_to_thread_info(regs);
 	unsigned long nr = regs->orig_ax;
 
+	enter_from_user_mode();
 	local_irq_enable();
 
 	if (READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY)
@@ -376,9 +369,9 @@ __visible void do_syscall_64(struct pt_regs *regs)
 
 #if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
 /*
- * Does a 32-bit syscall.  Called with IRQs on and does all entry and
- * exit work and returns with IRQs off.  This function is extremely hot
- * in workloads that use it, and it's usually called from
+ * Does a 32-bit syscall.  Called with IRQs on in CONTEXT_KERNEL.  Does
+ * all entry and exit work and returns with IRQs off.  This function is
+ * extremely hot in workloads that use it, and it's usually called from
  * do_fast_syscall_32, so forcibly inline it to improve performance.
  */
 static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
@@ -419,6 +412,7 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 /* Handles int $0x80 */
 __visible void do_int80_syscall_32(struct pt_regs *regs)
 {
+	enter_from_user_mode();
 	local_irq_enable();
 	do_syscall_32_irqs_on(regs);
 }
@@ -441,11 +435,9 @@ __visible long do_fast_syscall_32(struct pt_regs *regs)
 	 */
 	regs->ip = landing_pad;
 
-	/*
-	 * Fetch EBP from where the vDSO stashed it.
-	 *
-	 * WARNING: We are in CONTEXT_USER and RCU isn't paying attention!
-	 */
+	enter_from_user_mode();
+
+	/* Fetch EBP from where the vDSO stashed it. */
 	local_irq_enable();
 	if (
 #ifdef CONFIG_X86_64
@@ -464,9 +456,6 @@ __visible long do_fast_syscall_32(struct pt_regs *regs)
 		/* User code screwed up. */
 		local_irq_disable();
 		regs->ax = -EFAULT;
-#ifdef CONFIG_CONTEXT_TRACKING
-		enter_from_user_mode();
-#endif
 		prepare_exit_to_usermode(regs);
 		return 0;	/* Keep it simple: use IRET. */
 	}
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index c0778fcab06d..fbdb24e61835 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -134,7 +134,10 @@ struct thread_info {
 #define _TIF_ADDR32		(1 << TIF_ADDR32)
 #define _TIF_X32		(1 << TIF_X32)
 
-/* work to do in syscall_trace_enter() */
+/*
+ * work to do in syscall_trace_enter().  Also includes TIF_NOHZ for
+ * enter_from_user_mode()
+ */
 #define _TIF_WORK_SYSCALL_ENTRY	\
 	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_EMU | _TIF_SYSCALL_AUDIT |	\
 	 _TIF_SECCOMP | _TIF_SINGLESTEP | _TIF_SYSCALL_TRACEPOINT |	\
-- 
2.5.0

[toc] | [next] | [standalone]

#1353058

FromBorislav Petkov <bp@alien8.de>
Date2016-03-08 14:50 +0100
Message-ID<raqbp-1xM-37@gated-at.bofh.it>
In reply to#1351269
On Sun, Mar 06, 2016 at 06:30:07PM -0800, Andy Lutomirski wrote:
> Now that slow-path syscalls always enter C before enabling
> interrupts, it's straightforward to do enter_from_user_mode before
> enabling interrupts rather than doing it as part of entry tracing.
> 
> With this change, we should finally be able to retire
> exception_enter.
> 
> This will also enable optimizations based on knowing that we never
> change context tracking state with interrupts on.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/common.c            | 39 ++++++++++++++------------------------
>  arch/x86/include/asm/thread_info.h |  5 ++++-
>  2 files changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 9ef3258320ec..8e8ca0eb36be 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -38,14 +38,17 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
>  	return (struct thread_info *)(top_of_stack - THREAD_SIZE);
>  }
>  
> -#ifdef CONFIG_CONTEXT_TRACKING
> +#ifndef CONFIG_CONTEXT_TRACKING
> +static
> +#else
> +__visible
> +#endif
>  /* Called on entry from user mode with IRQs off. */
> -__visible void enter_from_user_mode(void)
> +void enter_from_user_mode(void)
>  {
>  	CT_WARN_ON(ct_state() != CONTEXT_USER);
>  	user_exit();
>  }
> -#endif

I'm guessing you're relying on the compiler to optimize out everything
in the !CONFIG_CONTEXT_TRACKING case, and it seems to do that here.

However, I would do the regular thing we do for cases like that. It is
much easier and faster to read because we do use it everywhere:

#ifdef CONFIG_CONTEXT_TRACKING
/* Called on entry from user mode with IRQs off. */
__visible void enter_from_user_mode(void)
{
        CT_WARN_ON(ct_state() != CONTEXT_USER);
        user_exit();
}
#else
#define enter_from_user_mode() do { } while (0)
#endif


>  
>  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
>  {
> @@ -85,17 +88,6 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
>  
>  	work = ACCESS_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
>  
> -#ifdef CONFIG_CONTEXT_TRACKING
> -	/*
> -	 * If TIF_NOHZ is set, we are required to call user_exit() before
> -	 * doing anything that could touch RCU.
> -	 */
> -	if (work & _TIF_NOHZ) {
> -		enter_from_user_mode();
> -		work &= ~_TIF_NOHZ;
> -	}
> -#endif
> -
>  #ifdef CONFIG_SECCOMP
>  	/*
>  	 * Do seccomp first -- it should minimize exposure of other
> @@ -354,6 +346,7 @@ __visible void do_syscall_64(struct pt_regs *regs)
>  	struct thread_info *ti = pt_regs_to_thread_info(regs);
>  	unsigned long nr = regs->orig_ax;
>  
> +	enter_from_user_mode();
>  	local_irq_enable();
>  
>  	if (READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY)
> @@ -376,9 +369,9 @@ __visible void do_syscall_64(struct pt_regs *regs)
>  
>  #if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
>  /*
> - * Does a 32-bit syscall.  Called with IRQs on and does all entry and
> - * exit work and returns with IRQs off.  This function is extremely hot
> - * in workloads that use it, and it's usually called from
> + * Does a 32-bit syscall.  Called with IRQs on in CONTEXT_KERNEL.  Does
> + * all entry and exit work and returns with IRQs off.  This function is
> + * extremely hot in workloads that use it, and it's usually called from
>   * do_fast_syscall_32, so forcibly inline it to improve performance.
>   */
>  static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
> @@ -419,6 +412,7 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
>  /* Handles int $0x80 */
>  __visible void do_int80_syscall_32(struct pt_regs *regs)
>  {
> +	enter_from_user_mode();
>  	local_irq_enable();
>  	do_syscall_32_irqs_on(regs);
>  }
> @@ -441,11 +435,9 @@ __visible long do_fast_syscall_32(struct pt_regs *regs)
>  	 */
>  	regs->ip = landing_pad;
>  
> -	/*
> -	 * Fetch EBP from where the vDSO stashed it.
> -	 *
> -	 * WARNING: We are in CONTEXT_USER and RCU isn't paying attention!
> -	 */
> +	enter_from_user_mode();
> +
> +	/* Fetch EBP from where the vDSO stashed it. */
>  	local_irq_enable();

Comment above should be here.

>  	if (
>  #ifdef CONFIG_X86_64
> @@ -464,9 +456,6 @@ __visible long do_fast_syscall_32(struct pt_regs *regs)
>  		/* User code screwed up. */
>  		local_irq_disable();
>  		regs->ax = -EFAULT;
> -#ifdef CONFIG_CONTEXT_TRACKING
> -		enter_from_user_mode();
> -#endif
>  		prepare_exit_to_usermode(regs);
>  		return 0;	/* Keep it simple: use IRET. */
>  	}
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index c0778fcab06d..fbdb24e61835 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -134,7 +134,10 @@ struct thread_info {
>  #define _TIF_ADDR32		(1 << TIF_ADDR32)
>  #define _TIF_X32		(1 << TIF_X32)
>  
> -/* work to do in syscall_trace_enter() */
> +/*
> + * work to do in syscall_trace_enter().  Also includes TIF_NOHZ for
> + * enter_from_user_mode()
> + */
>  #define _TIF_WORK_SYSCALL_ENTRY	\
>  	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_EMU | _TIF_SYSCALL_AUDIT |	\
>  	 _TIF_SECCOMP | _TIF_SINGLESTEP | _TIF_SYSCALL_TRACEPOINT |	\
> -- 
> 2.5.0
> 
> 

-- 
Regards/Gruss,
    Boris.

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

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

#1353253

FromAndy Lutomirski <luto@amacapital.net>
Date2016-03-08 18:30 +0100
Message-ID<ratCi-3TL-3@gated-at.bofh.it>
In reply to#1353058
On Mar 8, 2016 5:46 AM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Sun, Mar 06, 2016 at 06:30:07PM -0800, Andy Lutomirski wrote:
> > Now that slow-path syscalls always enter C before enabling
> > interrupts, it's straightforward to do enter_from_user_mode before
> > enabling interrupts rather than doing it as part of entry tracing.
> >
> > With this change, we should finally be able to retire
> > exception_enter.
> >
> > This will also enable optimizations based on knowing that we never
> > change context tracking state with interrupts on.
> >
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> >  arch/x86/entry/common.c            | 39 ++++++++++++++------------------------
> >  arch/x86/include/asm/thread_info.h |  5 ++++-
> >  2 files changed, 18 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> > index 9ef3258320ec..8e8ca0eb36be 100644
> > --- a/arch/x86/entry/common.c
> > +++ b/arch/x86/entry/common.c
> > @@ -38,14 +38,17 @@ static struct thread_info *pt_regs_to_thread_info(struct pt_regs *regs)
> >       return (struct thread_info *)(top_of_stack - THREAD_SIZE);
> >  }
> >
> > -#ifdef CONFIG_CONTEXT_TRACKING
> > +#ifndef CONFIG_CONTEXT_TRACKING
> > +static
> > +#else
> > +__visible
> > +#endif
> >  /* Called on entry from user mode with IRQs off. */
> > -__visible void enter_from_user_mode(void)
> > +void enter_from_user_mode(void)
> >  {
> >       CT_WARN_ON(ct_state() != CONTEXT_USER);
> >       user_exit();
> >  }
> > -#endif
>
> I'm guessing you're relying on the compiler to optimize out everything
> in the !CONFIG_CONTEXT_TRACKING case, and it seems to do that here.
>
> However, I would do the regular thing we do for cases like that. It is
> much easier and faster to read because we do use it everywhere:
>
> #ifdef CONFIG_CONTEXT_TRACKING
> /* Called on entry from user mode with IRQs off. */
> __visible void enter_from_user_mode(void)
> {
>         CT_WARN_ON(ct_state() != CONTEXT_USER);
>         user_exit();
> }
> #else
> #define enter_from_user_mode() do { } while (0)
> #endif
>

Will do, after I test to make sure it builds.

>
> >
> >  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
> >  {
> > @@ -85,17 +88,6 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
> >
> >       work = ACCESS_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
> >
> > -#ifdef CONFIG_CONTEXT_TRACKING
> > -     /*
> > -      * If TIF_NOHZ is set, we are required to call user_exit() before
> > -      * doing anything that could touch RCU.
> > -      */
> > -     if (work & _TIF_NOHZ) {
> > -             enter_from_user_mode();
> > -             work &= ~_TIF_NOHZ;
> > -     }
> > -#endif
> > -
> >  #ifdef CONFIG_SECCOMP
> >       /*
> >        * Do seccomp first -- it should minimize exposure of other
> > @@ -354,6 +346,7 @@ __visible void do_syscall_64(struct pt_regs *regs)
> >       struct thread_info *ti = pt_regs_to_thread_info(regs);
> >       unsigned long nr = regs->orig_ax;
> >
> > +     enter_from_user_mode();
> >       local_irq_enable();
> >
> >       if (READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY)
> > @@ -376,9 +369,9 @@ __visible void do_syscall_64(struct pt_regs *regs)
> >
> >  #if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
> >  /*
> > - * Does a 32-bit syscall.  Called with IRQs on and does all entry and
> > - * exit work and returns with IRQs off.  This function is extremely hot
> > - * in workloads that use it, and it's usually called from
> > + * Does a 32-bit syscall.  Called with IRQs on in CONTEXT_KERNEL.  Does
> > + * all entry and exit work and returns with IRQs off.  This function is
> > + * extremely hot in workloads that use it, and it's usually called from
> >   * do_fast_syscall_32, so forcibly inline it to improve performance.
> >   */
> >  static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
> > @@ -419,6 +412,7 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
> >  /* Handles int $0x80 */
> >  __visible void do_int80_syscall_32(struct pt_regs *regs)
> >  {
> > +     enter_from_user_mode();
> >       local_irq_enable();
> >       do_syscall_32_irqs_on(regs);
> >  }
> > @@ -441,11 +435,9 @@ __visible long do_fast_syscall_32(struct pt_regs *regs)
> >        */
> >       regs->ip = landing_pad;
> >
> > -     /*
> > -      * Fetch EBP from where the vDSO stashed it.
> > -      *
> > -      * WARNING: We are in CONTEXT_USER and RCU isn't paying attention!
> > -      */
> > +     enter_from_user_mode();
> > +
> > +     /* Fetch EBP from where the vDSO stashed it. */
> >       local_irq_enable();
>
> Comment above should be here.

Indeed.

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web