new objtool warnings again...

new objtool warnings again...

[Linus Torvalds]

Josh,

 the current F24 toolchain causes

    kernel/signal.o: warning: objtool: .altinstr_replacement+0x54:
call without frame pointer save/setup

during a regular allmodconfig build.

Doing an objdump says:

...
  54:   e8 00 00 00 00          callq  59 <.altinstr_replacement+0x59>
                        55: R_X86_64_PC32       copy_user_generic_string-0x4
  59:   e8 00 00 00 00          callq  5e <.altinstr_replacement+0x5e>
                        5a: R_X86_64_PC32
copy_user_enhanced_fast_string-0x4
...

so it seems to come from the alternative_call_2() in copy_user_generic().

It's somewhere in copy_siginfo_to_user(), so I assume it's just the

        if (from->si_code < 0)
                return __copy_to_user(to, from, sizeof(siginfo_t))
                        ? -EFAULT : 0;

case.  Looking at the code generation, it looks like the frame pointer
generation in that function has been moved down past this code, so the
objtool warning seems to be correct, but this indicates that gcc has
decided that we don't need a frame for that alternative_call_2()
thing.

So this code is clearly missing the magic to tell gcc that the asm
needs a frame pointer.

What was that magic again? Mind sending a patch?

                Linus

Re: new objtool warnings again...

[Linus Torvalds]

On Fri, Sep 23, 2016 at 1:33 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So this code is clearly missing the magic to tell gcc that the asm
> needs a frame pointer.

Independently of that, the objtool build seems racy or somehow
fragile. I've now twice gotten into a situation where I end up getting

  cat: /home/torvalds/v2.6/linux/tools/objtool/.fixdep.o.d: No such
file or directory
  make[4]: *** [/home/torvalds/v2.6/linux/tools/objtool/fixdep.o] Error 1
  make[3]: *** [/home/torvalds/v2.6/linux/tools/objtool/fixdep-in.o] Error 2
  make[2]: *** [fixdep] Error 2
  make[1]: *** [objtool] Error 2
  make: *** [tools/objtool] Error 2

with just the right timings, and then ccache ends up remembering that
as a build failure and causing that to be "sticky" even across "git
clean -dqfx" builds (and the "ccache -C" clears it).

Adding Michal to the cc, in case he can see what the problem is.

                 Linus

Re: new objtool warnings again...

[Josh Poimboeuf]

On Fri, Sep 23, 2016 at 02:06:03PM -0700, Linus Torvalds wrote:
> On Fri, Sep 23, 2016 at 1:33 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So this code is clearly missing the magic to tell gcc that the asm
> > needs a frame pointer.
> 
> Independently of that, the objtool build seems racy or somehow
> fragile. I've now twice gotten into a situation where I end up getting
> 
>   cat: /home/torvalds/v2.6/linux/tools/objtool/.fixdep.o.d: No such
> file or directory
>   make[4]: *** [/home/torvalds/v2.6/linux/tools/objtool/fixdep.o] Error 1
>   make[3]: *** [/home/torvalds/v2.6/linux/tools/objtool/fixdep-in.o] Error 2
>   make[2]: *** [fixdep] Error 2
>   make[1]: *** [objtool] Error 2
>   make: *** [tools/objtool] Error 2
> 
> with just the right timings, and then ccache ends up remembering that
> as a build failure and causing that to be "sticky" even across "git
> clean -dqfx" builds (and the "ccache -C" clears it).
> 
> Adding Michal to the cc, in case he can see what the problem is.

I just started seeing this problem today.  I suspect it's a ccache
issue, since it only showed up after ccache was updated.  I "fixed" it
with:

  yum downgrade ccache

I'll open a bug...

-- 
Josh

Re: new objtool warnings again...

[Linus Torvalds]

On Fri, Sep 23, 2016 at 2:16 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> I just started seeing this problem today.  I suspect it's a ccache
> issue, since it only showed up after ccache was updated.

Ahh, I didn't even notice that ccache got updated, but yeah, that makes sense.

            Linus

Re: new objtool warnings again...

[Arnaldo Carvalho de Melo]

Em Fri, Sep 23, 2016 at 02:19:04PM -0700, Linus Torvalds escreveu:
> On Fri, Sep 23, 2016 at 2:16 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > I just started seeing this problem today.  I suspect it's a ccache
> > issue, since it only showed up after ccache was updated.
> 
> Ahh, I didn't even notice that ccache got updated, but yeah, that makes sense.

yeah, that explains the problems on my side as well, I "fixed" it by
uninstalling ccache, will get the older version..


- Arnaldo

Re: new objtool warnings again...

[Josh Poimboeuf]

On Mon, Sep 26, 2016 at 01:49:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 23, 2016 at 02:19:04PM -0700, Linus Torvalds escreveu:
> > On Fri, Sep 23, 2016 at 2:16 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > I just started seeing this problem today.  I suspect it's a ccache
> > > issue, since it only showed up after ccache was updated.
> > 
> > Ahh, I didn't even notice that ccache got updated, but yeah, that makes sense.
> 
> yeah, that explains the problems on my side as well, I "fixed" it by
> uninstalling ccache, will get the older version..

FYI, here's the upstream ccache github issue:

  https://github.com/ccache/ccache/issues/133

and possibly related:

  https://github.com/ccache/ccache/issues/134

-- 
Josh

Re: new objtool warnings again...

[Josh Poimboeuf]

On Fri, Sep 23, 2016 at 01:33:45PM -0700, Linus Torvalds wrote:
> Josh,
> 
>  the current F24 toolchain causes
> 
>     kernel/signal.o: warning: objtool: .altinstr_replacement+0x54:
> call without frame pointer save/setup
> 
> during a regular allmodconfig build.
> 
> Doing an objdump says:
> 
> ...
>   54:   e8 00 00 00 00          callq  59 <.altinstr_replacement+0x59>
>                         55: R_X86_64_PC32       copy_user_generic_string-0x4
>   59:   e8 00 00 00 00          callq  5e <.altinstr_replacement+0x5e>
>                         5a: R_X86_64_PC32
> copy_user_enhanced_fast_string-0x4
> ...
> 
> so it seems to come from the alternative_call_2() in copy_user_generic().
> 
> It's somewhere in copy_siginfo_to_user(), so I assume it's just the
> 
>         if (from->si_code < 0)
>                 return __copy_to_user(to, from, sizeof(siginfo_t))
>                         ? -EFAULT : 0;
> 
> case.  Looking at the code generation, it looks like the frame pointer
> generation in that function has been moved down past this code, so the
> objtool warning seems to be correct, but this indicates that gcc has
> decided that we don't need a frame for that alternative_call_2()
> thing.
> 
> So this code is clearly missing the magic to tell gcc that the asm
> needs a frame pointer.
> 
> What was that magic again? Mind sending a patch?

Is this with your latest pushed master branch?  I have F24, but I don't
see the warning.

In any case, I'll come up with a patch for you to test.

-- 
Josh

Re: new objtool warnings again...

[Linus Torvalds]

On Fri, Sep 23, 2016 at 2:14 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Is this with your latest pushed master branch?  I have F24, but I don't
> see the warning.

It is with the latest branch, but I was wrong - it doesn't show up for
"allmodconfig", it only shows up for my fairly-minimal config on this
laptop.

> In any case, I'll come up with a patch for you to test.

I can send you my odd config in case you want it, but I'm assuming you
just went "ahh, I know what's up" and don't even need it.

            Linus

[PATCH] x86/alternatives: add stack frame dependency to alternative_call_2()

[Josh Poimboeuf]

Linus reported the following objtool warning:

  kernel/signal.o: warning: objtool: .altinstr_replacement+0x54: call without frame pointer save/setup

The warning is valid.  It's caused by the fact that gcc placed the call
instruction in alternative_call_2()'s inline asm before the frame
pointer setup, which breaks frame pointer convention and can result in a
bad stack trace.

Force a stack frame to be created before the call instruction by listing
the stack pointer as an output operand in the inline asm statement.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/alternative.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index e77a644..1b02038 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -217,10 +217,14 @@ static inline int alternatives_text_reserved(void *start, void *end)
  */
 #define alternative_call_2(oldfunc, newfunc1, feature1, newfunc2, feature2,   \
 			   output, input...)				      \
+{									      \
+	register void *__sp asm(_ASM_SP);				      \
 	asm volatile (ALTERNATIVE_2("call %P[old]", "call %P[new1]", feature1,\
 		"call %P[new2]", feature2)				      \
-		: output : [old] "i" (oldfunc), [new1] "i" (newfunc1),	      \
-		[new2] "i" (newfunc2), ## input)
+		: output, "+r" (__sp)					      \
+		: [old] "i" (oldfunc), [new1] "i" (newfunc1),		      \
+		  [new2] "i" (newfunc2), ## input);			      \
+}
 
 /*
  * use this macro(s) if you need more than one output parameter
-- 
2.7.4

Re: [PATCH] x86/alternatives: add stack frame dependency to alternative_call_2()

[Linus Torvalds]

On Fri, Sep 23, 2016 at 2:49 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>

Works for me. So:

  Reported-and-tested-by: Linus Torvalds <torvalds@linux-foundation.org>

but I don't think I'll apply this to my tree now, since it's largely
harmless, and we're already going to do an rc8 and I worry that it
will end up showing something else. But it probably would be good to
queue for the x86 tree for 4.9

                Linus

[tip:x86/asm] x86/alternatives: Add stack frame dependency to alternative_call_2()

[tip-bot for Josh Poimboeuf]

Commit-ID:  317c2ce77d8ab73c24f4fb9c75e5bb441fbe3e30
Gitweb:     http://git.kernel.org/tip/317c2ce77d8ab73c24f4fb9c75e5bb441fbe3e30
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Fri, 23 Sep 2016 16:49:39 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 24 Sep 2016 09:30:03 +0200

x86/alternatives: Add stack frame dependency to alternative_call_2()

Linus reported the following objtool warning:

  kernel/signal.o: warning: objtool: .altinstr_replacement+0x54: call without frame pointer save/setup

The warning is valid.  It's caused by the fact that gcc placed the call
instruction in alternative_call_2()'s inline asm before the frame
pointer setup, which breaks frame pointer convention and can result in a
bad stack trace.

Force a stack frame to be created before the call instruction by listing
the stack pointer as an output operand in the inline asm statement.

Reported-and-tested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
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: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20160923214939.j5o7c67nhepzmh3t@treble
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/alternative.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index e77a644..1b02038 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -217,10 +217,14 @@ static inline int alternatives_text_reserved(void *start, void *end)
  */
 #define alternative_call_2(oldfunc, newfunc1, feature1, newfunc2, feature2,   \
 			   output, input...)				      \
+{									      \
+	register void *__sp asm(_ASM_SP);				      \
 	asm volatile (ALTERNATIVE_2("call %P[old]", "call %P[new1]", feature1,\
 		"call %P[new2]", feature2)				      \
-		: output : [old] "i" (oldfunc), [new1] "i" (newfunc1),	      \
-		[new2] "i" (newfunc2), ## input)
+		: output, "+r" (__sp)					      \
+		: [old] "i" (oldfunc), [new1] "i" (newfunc1),		      \
+		  [new2] "i" (newfunc2), ## input);			      \
+}
 
 /*
  * use this macro(s) if you need more than one output parameter