[PATCH] x86/arch_prctl/64: restore accidentally removed put_cpu in ARCH_SET_GS

[PATCH] x86/arch_prctl/64: restore accidentally removed put_cpu in ARCH_SET_GS

[Mateusz Guzik]

This fixes 731e33e39a5b95ad770 "Remove FSBASE/GSBASE < 4G optimization"

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
---
 arch/x86/kernel/process_64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 4285f6a..6b16c36 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -541,6 +541,7 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 			load_gs_index(0);
 			ret = wrmsrl_safe(MSR_KERNEL_GS_BASE, addr);
 		}
+		put_cpu();
 		break;
 	case ARCH_SET_FS:
 		/* Not strictly needed for fs, but do it for symmetry
-- 
1.8.3.1

Re: [PATCH] x86/arch_prctl/64: restore accidentally removed put_cpu in ARCH_SET_GS

[Andy Lutomirski]

On Tue, May 10, 2016 at 1:56 PM, Mateusz Guzik <mguzik@redhat.com> wrote:
> This fixes 731e33e39a5b95ad770 "Remove FSBASE/GSBASE < 4G optimization"

Indeed.  How did that survive lockdep?

Acked-by: Andy Lutomirski <luto@kernel.org>

>
> Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
> ---
>  arch/x86/kernel/process_64.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 4285f6a..6b16c36 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -541,6 +541,7 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
>                         load_gs_index(0);
>                         ret = wrmsrl_safe(MSR_KERNEL_GS_BASE, addr);
>                 }
> +               put_cpu();
>                 break;
>         case ARCH_SET_FS:
>                 /* Not strictly needed for fs, but do it for symmetry
> --
> 1.8.3.1
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] x86/arch_prctl/64: restore accidentally removed put_cpu in ARCH_SET_GS

[Mateusz Guzik]

On Tue, May 10, 2016 at 01:58:24PM -0700, Andy Lutomirski wrote:
> On Tue, May 10, 2016 at 1:56 PM, Mateusz Guzik <mguzik@redhat.com> wrote:
> > This fixes 731e33e39a5b95ad770 "Remove FSBASE/GSBASE < 4G optimization"
> 
> Indeed.  How did that survive lockdep?
> 

lockdep_sys_exit only checks actual locks.

In the common path after return from particular syscall interrupts get
blindly disabled (as opposed to checking first that they are enabled).
preemption count is not checked in the fast path at all and is checked
elsewhere as a side effect of calls to e.g. schedule().

How about a hack along these lines (note I don't claim this is
committable as it is, but it should work):

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index ec138e5..5887bc7 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -303,6 +303,24 @@ static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags)
 		tracehook_report_syscall_exit(regs, step);
 }
 
+#ifdef CONFIG_PROVE_LOCKING
+/*
+ * Called after syscall handlers return.
+ */
+__visible void syscall_assert_exit(struct pt_regs *regs)
+{
+	if (in_atomic() || irqs_disabled()) {
+		printk(KERN_ERR "invalid state on exit from syscall %ld: "
+			"in_atomic(): %d, irqs_disabled(): %d, pid: %d, "
+			"name: %s\n", regs->orig_ax, in_atomic(),
+			irqs_disabled(), current->pid, current->comm);
+	}
+
+	if (irqs_disabled())
+		local_irq_enable();
+}
+#endif
+
 /*
  * Called with IRQs on and fully valid regs.  Returns with IRQs off in a
  * state such that we can immediately switch to user mode.
@@ -314,9 +332,7 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
 
 	CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
 
-	if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
-	    WARN(irqs_disabled(), "syscall %ld left IRQs disabled", regs->orig_ax))
-		local_irq_enable();
+	syscall_assert_exit(regs);
 
 	/*
 	 * First do one-time work.  If these work items are enabled, we
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 9ee0da1..6c5cc23 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -210,6 +210,12 @@ entry_SYSCALL_64_fastpath:
 	movq	%rax, RAX(%rsp)
 1:
 
+#ifdef CONFIG_PROVE_LOCKING
+	/*
+	 * We want to validate bunch of stuff, which will clobber registers.
+	 */
+	jmp	2f
+#endif
 	/*
 	 * If we get here, then we know that pt_regs is clean for SYSRET64.
 	 * If we see that no exit work is required (which we are required
@@ -236,6 +242,7 @@ entry_SYSCALL_64_fastpath:
 	 */
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_NONE)
+2:
 	SAVE_EXTRA_REGS
 	movq	%rsp, %rdi
 	call	syscall_return_slowpath	/* returns with IRQs disabled */

-- 
Mateusz Guzik

Re: [PATCH] x86/arch_prctl/64: restore accidentally removed put_cpu in ARCH_SET_GS

[Andy Lutomirski]

On May 11, 2016 1:35 PM, "Mateusz Guzik" <mguzik@redhat.com> wrote:
>
> On Tue, May 10, 2016 at 01:58:24PM -0700, Andy Lutomirski wrote:
> > On Tue, May 10, 2016 at 1:56 PM, Mateusz Guzik <mguzik@redhat.com> wrote:
> > > This fixes 731e33e39a5b95ad770 "Remove FSBASE/GSBASE < 4G optimization"
> >
> > Indeed.  How did that survive lockdep?
> >
>
> lockdep_sys_exit only checks actual locks.
>
> In the common path after return from particular syscall interrupts get
> blindly disabled (as opposed to checking first that they are enabled).
> preemption count is not checked in the fast path at all and is checked
> elsewhere as a side effect of calls to e.g. schedule().
>
> How about a hack along these lines (note I don't claim this is
> committable as it is, but it should work):
>
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index ec138e5..5887bc7 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -303,6 +303,24 @@ static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags)
>                 tracehook_report_syscall_exit(regs, step);
>  }
>
> +#ifdef CONFIG_PROVE_LOCKING
> +/*
> + * Called after syscall handlers return.
> + */
> +__visible void syscall_assert_exit(struct pt_regs *regs)
> +{
> +       if (in_atomic() || irqs_disabled()) {
> +               printk(KERN_ERR "invalid state on exit from syscall %ld: "
> +                       "in_atomic(): %d, irqs_disabled(): %d, pid: %d, "
> +                       "name: %s\n", regs->orig_ax, in_atomic(),
> +                       irqs_disabled(), current->pid, current->comm);
> +       }
> +
> +       if (irqs_disabled())
> +               local_irq_enable();
> +}
> +#endif
> +
>  /*
>   * Called with IRQs on and fully valid regs.  Returns with IRQs off in a
>   * state such that we can immediately switch to user mode.
> @@ -314,9 +332,7 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
>
>         CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>
> -       if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
> -           WARN(irqs_disabled(), "syscall %ld left IRQs disabled", regs->orig_ax))
> -               local_irq_enable();
> +       syscall_assert_exit(regs);
>
>         /*
>          * First do one-time work.  If these work items are enabled, we
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 9ee0da1..6c5cc23 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -210,6 +210,12 @@ entry_SYSCALL_64_fastpath:
>         movq    %rax, RAX(%rsp)
>  1:
>
> +#ifdef CONFIG_PROVE_LOCKING
> +       /*
> +        * We want to validate bunch of stuff, which will clobber registers.
> +        */
> +       jmp     2f
> +#endif
>         /*
>          * If we get here, then we know that pt_regs is clean for SYSRET64.
>          * If we see that no exit work is required (which we are required
> @@ -236,6 +242,7 @@ entry_SYSCALL_64_fastpath:
>          */
>         TRACE_IRQS_ON
>         ENABLE_INTERRUPTS(CLBR_NONE)
> +2:
>         SAVE_EXTRA_REGS
>         movq    %rsp, %rdi
>         call    syscall_return_slowpath /* returns with IRQs disabled */

It would be nice to do this in a cross-arch way.  Maybe we could
extend lockdep_sys_exit?  Ingo, do you think that would be reasonable?

--Andy

>
> --
> Mateusz Guzik

Re: [PATCH] x86/arch_prctl/64: restore accidentally removed put_cpu in ARCH_SET_GS

[Andy Lutomirski]

On Tue, May 10, 2016 at 1:56 PM, Mateusz Guzik <mguzik@redhat.com> wrote:
> This fixes 731e33e39a5b95ad770 "Remove FSBASE/GSBASE < 4G optimization"
>
> Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
> ---
>  arch/x86/kernel/process_64.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 4285f6a..6b16c36 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -541,6 +541,7 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
>                         load_gs_index(0);
>                         ret = wrmsrl_safe(MSR_KERNEL_GS_BASE, addr);
>                 }
> +               put_cpu();
>                 break;
>         case ARCH_SET_FS:
>                 /* Not strictly needed for fs, but do it for symmetry
> --
> 1.8.3.1
>

Ingo, can you apply this before the merge window opens?

I just noticed that you weren't cc'd, so I'll repeat my ack:

Acked-by: Andy Lutomirski <luto@kernel.org>

And I'll ask, since IIRC you wrote it: would it make sense to augment
lockdep_sys_exit to see if preemption got left disabled?

Thanks,
Andy

Re: [PATCH] x86/arch_prctl/64: restore accidentally removed put_cpu in ARCH_SET_GS

[Ingo Molnar]

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

> On Tue, May 10, 2016 at 1:56 PM, Mateusz Guzik <mguzik@redhat.com> wrote:
> > This fixes 731e33e39a5b95ad770 "Remove FSBASE/GSBASE < 4G optimization"
> >
> > Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
> > ---
> >  arch/x86/kernel/process_64.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> > index 4285f6a..6b16c36 100644
> > --- a/arch/x86/kernel/process_64.c
> > +++ b/arch/x86/kernel/process_64.c
> > @@ -541,6 +541,7 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
> >                         load_gs_index(0);
> >                         ret = wrmsrl_safe(MSR_KERNEL_GS_BASE, addr);
> >                 }
> > +               put_cpu();
> >                 break;
> >         case ARCH_SET_FS:
> >                 /* Not strictly needed for fs, but do it for symmetry
> > --
> > 1.8.3.1
> >
> 
> Ingo, can you apply this before the merge window opens?

Yeah, done, applied it to tip:x86/asm.

> I just noticed that you weren't cc'd, so I'll repeat my ack:
> 
> Acked-by: Andy Lutomirski <luto@kernel.org>
> 
> And I'll ask, since IIRC you wrote it: would it make sense to augment
> lockdep_sys_exit to see if preemption got left disabled?

Yeah, absolutely. I'm quite sure early lockdep versions did such a check.

Thanks,

	Ingo

[tip:x86/asm] x86/arch_prctl/64: Restore accidentally removed put_cpu() in ARCH_SET_GS

[tip-bot for Mateusz Guzik]

Commit-ID:  4afd0565552c87f23834db9121dd9cf6955d0b43
Gitweb:     http://git.kernel.org/tip/4afd0565552c87f23834db9121dd9cf6955d0b43
Author:     Mateusz Guzik <mguzik@redhat.com>
AuthorDate: Tue, 10 May 2016 22:56:43 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 13 May 2016 13:50:15 +0200

x86/arch_prctl/64: Restore accidentally removed put_cpu() in ARCH_SET_GS

This fixes an oversight in:

	731e33e39a5b95 ("Remove FSBASE/GSBASE < 4G optimization")

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Link: http://lkml.kernel.org/r/1462913803-29634-1-git-send-email-mguzik@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/process_64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 4285f6a..6b16c36 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -541,6 +541,7 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 			load_gs_index(0);
 			ret = wrmsrl_safe(MSR_KERNEL_GS_BASE, addr);
 		}
+		put_cpu();
 		break;
 	case ARCH_SET_FS:
 		/* Not strictly needed for fs, but do it for symmetry