[PATCH] pop previous section in alternative.c

[PATCH] pop previous section in alternative.c

[Steven Rostedt]

gcc expects all toplevel assembly to return to the original section type.
The code in alteranative.c does not do this. This caused some strange bugs
in sched-devel where code would end up in the .rodata section and when
the kernel sets the NX bit on all .rodata, the kernel would crash when
executing this code.

This patch adds a .previous marker to return the code back to the
original section.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/x86/kernel/alternative.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux-compile.git/arch/x86/kernel/alternative.c
===================================================================
--- linux-compile.git.orig/arch/x86/kernel/alternative.c	2008-04-09 18:55:28.000000000 -0400
+++ linux-compile.git/arch/x86/kernel/alternative.c	2008-04-09 18:55:49.000000000 -0400
@@ -65,7 +65,8 @@ __setup("noreplace-paravirt", setup_nore
    get them easily into strings. */
 asm("\t.section .rodata, \"a\"\nintelnops: "
 	GENERIC_NOP1 GENERIC_NOP2 GENERIC_NOP3 GENERIC_NOP4 GENERIC_NOP5 GENERIC_NOP6
-	GENERIC_NOP7 GENERIC_NOP8);
+	GENERIC_NOP7 GENERIC_NOP8
+    "\t.previous");
 extern const unsigned char intelnops[];
 static const unsigned char *const intel_nops[ASM_NOP_MAX+1] = {
 	NULL,
@@ -83,7 +84,8 @@ static const unsigned char *const intel_
 #ifdef K8_NOP1
 asm("\t.section .rodata, \"a\"\nk8nops: "
 	K8_NOP1 K8_NOP2 K8_NOP3 K8_NOP4 K8_NOP5 K8_NOP6
-	K8_NOP7 K8_NOP8);
+	K8_NOP7 K8_NOP8
+    "\t.previous");
 extern const unsigned char k8nops[];
 static const unsigned char *const k8_nops[ASM_NOP_MAX+1] = {
 	NULL,
@@ -101,7 +103,8 @@ static const unsigned char *const k8_nop
 #ifdef K7_NOP1
 asm("\t.section .rodata, \"a\"\nk7nops: "
 	K7_NOP1 K7_NOP2 K7_NOP3 K7_NOP4 K7_NOP5 K7_NOP6
-	K7_NOP7 K7_NOP8);
+	K7_NOP7 K7_NOP8
+    "\t.previous");
 extern const unsigned char k7nops[];
 static const unsigned char *const k7_nops[ASM_NOP_MAX+1] = {
 	NULL,
@@ -119,7 +122,8 @@ static const unsigned char *const k7_nop
 #ifdef P6_NOP1
 asm("\t.section .rodata, \"a\"\np6nops: "
 	P6_NOP1 P6_NOP2 P6_NOP3 P6_NOP4 P6_NOP5 P6_NOP6
-	P6_NOP7 P6_NOP8);
+	P6_NOP7 P6_NOP8
+    "\t.previous");
 extern const unsigned char p6nops[];
 static const unsigned char *const p6_nops[ASM_NOP_MAX+1] = {
 	NULL,

Re: [PATCH] pop previous section in alternative.c

[Steven Rostedt]

On Wed, 9 Apr 2008, Steven Rostedt wrote:

>
> gcc expects all toplevel assembly to return to the original section type.
> The code in alteranative.c does not do this. This caused some strange bugs
> in sched-devel where code would end up in the .rodata section and when
> the kernel sets the NX bit on all .rodata, the kernel would crash when
> executing this code.
>
> This patch adds a .previous marker to return the code back to the
> original section.
>

Oh, and this would not be complete without giving Andrew Pinski complete
credit for telling me it wasn't a gcc bug but a bug in the toplevel asm
code in the kernel. ;-)

-- Steve

Re: [PATCH] pop previous section in alternative.c

[H. Peter Anvin]

Steven Rostedt wrote:
> On Wed, 9 Apr 2008, Steven Rostedt wrote:
> 
>> gcc expects all toplevel assembly to return to the original section type.
>> The code in alteranative.c does not do this. This caused some strange bugs
>> in sched-devel where code would end up in the .rodata section and when
>> the kernel sets the NX bit on all .rodata, the kernel would crash when
>> executing this code.
>>
>> This patch adds a .previous marker to return the code back to the
>> original section.
>>
> 
> Oh, and this would not be complete without giving Andrew Pinski complete
> credit for telling me it wasn't a gcc bug but a bug in the toplevel asm
> code in the kernel. ;-)
> 

In many ways it's kind of silly for this even to be in assembly, since 
all it is is a sequence of comma-separated byte values; I guess it was 
the easiest way to deal with it given the ".byte" prefix, but still...

	-hpa

Re: [PATCH] pop previous section in alternative.c

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:



> On Wed, 9 Apr 2008, Steven Rostedt wrote:

> 

> > gcc expects all toplevel assembly to return to the original section 

> > type. The code in alteranative.c does not do this. This caused some 

> > strange bugs in sched-devel where code would end up in the .rodata 

> > section and when the kernel sets the NX bit on all .rodata, the 

> > kernel would crash when executing this code.

> >

> > This patch adds a .previous marker to return the code back to the 

> > original section.

> 

> Oh, and this would not be complete without giving Andrew Pinski 

> complete credit for telling me it wasn't a gcc bug but a bug in the 

> toplevel asm code in the kernel. ;-)



thanks Steve and Andrew for resolving this so quickly! For the record, 

here's the original, mysterious-looking crash that Peter saw with ftrace 

enabled, under sched-devel:



---------->

ACPI: PCI Interrupt 0000:01:0e.1[A] -> GSI 11 (level, low) -> IRQ 11

Waiting for driver initialization.

Loading shpchp.ko module

BUG: unable to handle kernel paging request at ffffffff8054f06a

IP: [<ffffffff8054f06a>] constant_test_bit+0x0/0x26

PGD 203067 PUD 207063 PMD 7f81d163 PTE 800000000054f161

Oops: 0011 [1] PREEMPT SMP 

CPU 0 

Modules linked in: sata_svw ata_generic

Pid: 565, comm: insmod Not tainted 2.6.25-rc8-sched-devel.git-x86-latest.git #177

RIP: 0010:[<ffffffff8054f06a>]  [<ffffffff8054f06a>] constant_test_bit+0x0/0x26

RSP: 0018:ffff81007d693d40  EFLAGS: 00010246

RAX: ffff81007f805db0 RBX: ffff81007d4e5b80 RCX: 0000000000000000

RDX: ffff81007d4e5bb0 RSI: ffffffff8074c114 RDI: 0000000000000069

RBP: ffff81007d693d88 R08: ffffffff80964770 R09: 0000000000000000

R10: 000000037d693c78 R11: ffff81007d693cd8 R12: ffffffffa000b464

R13: ffffffffa0008000 R14: ffffffffa000cdc0 R15: ffffffffa000cda0

FS:  00007f96cec7d6f0(0000) GS:ffffffff8072a000(0000) knlGS:0000000000000000

CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b

CR2: ffffffff8054f06a CR3: 000000007d5c8000 CR4: 00000000000006e0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000

DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400

Process insmod (pid: 565, threadinfo ffff81007d692000, task ffff81007d690000)

Stack:  ffffffff802125c9 ffff81007d693d78 ffffffffa0010998 ffffffffa0010980

 ffffc20000032790 ffffc20000031f1d 0000000000000000 ffffc20000032010

 ffffc2000002b000 ffff81007d693de8 ffffffff8022359f ffffffffa0010980

Call Trace:

 [<ffffffff802125c9>] ? alternatives_smp_module_add+0x117/0x14e

 [<ffffffff8022359f>] module_finalize+0x118/0x136

 [<ffffffff80268b08>] sys_init_module+0x1496/0x1a79

 [<ffffffff8053c61a>] ? _spin_unlock_irqrestore+0x6b/0x79

 [<ffffffff80383ba5>] ? acpi_get_hp_params_from_firmware+0x0/0x50b

 [<ffffffff8053b88d>] ? trace_hardirqs_on_thunk+0x3a/0x3f

 [<ffffffff8053b88d>] ? trace_hardirqs_on_thunk+0x3a/0x3f

 [<ffffffff8025ee36>] ? trace_hardirqs_on_caller+0x109/0x12d

 [<ffffffff8053b88d>] ? trace_hardirqs_on_thunk+0x3a/0x3f

 [<ffffffff8020c41b>] system_call_after_swapgs+0x7b/0x80



Code: 00 00 0f 1f 80 00 00 00 00 0f 1f 84 00 00 00 00 00 55 48 89 e5 e8 d7 d1 cb ff 89 f8 c1 f8 05 48 98 48 8d 04 86 f0 0f b3 3e c9 c3 <55> 48 89 e5 e8 bd d1 cb ff 89 f9 bf 40 00 00 00 89 c8 99 f7 ff 

RIP  [<ffffffff8054f06a>] constant_test_bit+0x0/0x26

 RSP <ffff81007d693d40>

CR2: ffffffff8054f06a

---[ end trace 778e504de7e3b1e3 ]---

note: insmod[565] exited with preempt_count 1

BUG: sleeping function called from invalid context at /mnt/md0/src/linux-2.6-2/kernel/rwsem.c:21



(it only triggered on Peter's hardware)



	Ingo

Re: [PATCH] pop previous section in alternative.c

[Andi Kleen]

"H. Peter Anvin" <hpa@zytor.com> writes:
>>>
>> Oh, and this would not be complete without giving Andrew Pinski
>> complete
>> credit for telling me it wasn't a gcc bug but a bug in the toplevel asm
>> code in the kernel. ;-)

We've actually had such bugs before, it isn't the first time.

>
> In many ways it's kind of silly for this even to be in assembly, since
> all it is is a sequence of comma-separated byte values; I guess it was
> the easiest way to deal with it given the ".byte" prefix, but still...

The nops are primarily used in inline assembler statements (in alternative)
and only once in this table. Not using this would have meant to write
them all twice which would have been nasty. There is also no sane 
way to get standard arrays into inline assembler as instructions.

BTW it looks like the problem was added with 121d7bf5a246d282ba91234d03a4edf9ccc9c940,
signed off by me, sorry for not catching it in review.

Perhaps that is something that would make sense adding to checkpatch.pl?
Complain for .section in inline assembler without .previous or popsection
(cc Andy). I think such a check would make sense. 

-Andi

Re: [PATCH] pop previous section in alternative.c

[Andy Whitcroft]

On Thu, Apr 10, 2008 at 10:47:18AM +0200, Andi Kleen wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> >>>
> >> Oh, and this would not be complete without giving Andrew Pinski
> >> complete
> >> credit for telling me it wasn't a gcc bug but a bug in the toplevel asm
> >> code in the kernel. ;-)
> 
> We've actually had such bugs before, it isn't the first time.
> 
> >
> > In many ways it's kind of silly for this even to be in assembly, since
> > all it is is a sequence of comma-separated byte values; I guess it was
> > the easiest way to deal with it given the ".byte" prefix, but still...
> 
> The nops are primarily used in inline assembler statements (in alternative)
> and only once in this table. Not using this would have meant to write
> them all twice which would have been nasty. There is also no sane 
> way to get standard arrays into inline assembler as instructions.
> 
> BTW it looks like the problem was added with 121d7bf5a246d282ba91234d03a4edf9ccc9c940,
> signed off by me, sorry for not catching it in review.
> 
> Perhaps that is something that would make sense adding to checkpatch.pl?
> Complain for .section in inline assembler without .previous or popsection
> (cc Andy). I think such a check would make sense. 

Do you have an example of such a bad thing?  My only concern is this
sounds like a check which could potentially need to see more lines than are
available in the patch context and so the test might be rather unreliable.

-apw

Re: [PATCH] pop previous section in alternative.c

[Steven Rostedt]

On Thu, 10 Apr 2008, Andy Whitcroft wrote:
> >
> > BTW it looks like the problem was added with 121d7bf5a246d282ba91234d03a4edf9ccc9c940,
> > signed off by me, sorry for not catching it in review.
> >
> > Perhaps that is something that would make sense adding to checkpatch.pl?
> > Complain for .section in inline assembler without .previous or popsection
> > (cc Andy). I think such a check would make sense.
>
> Do you have an example of such a bad thing?  My only concern is this
> sounds like a check which could potentially need to see more lines than are
> available in the patch context and so the test might be rather unreliable.
>

>From commit 121d7bf5a246d282ba91234d03a4edf9ccc9c940:

    get them easily into strings. */
-asm("\t.data\nintelnops: "
+asm("\t.section .rodata, \"a\"\nintelnops: "
        GENERIC_NOP1 GENERIC_NOP2 GENERIC_NOP3 GENERIC_NOP4 GENERIC_NOP5 GENERIC
        GENERIC_NOP7 GENERIC_NOP8);
-extern unsigned char intelnops[];


I would say anytime there's a "^+.*\.section" there had better be a
"^-.*\.section" or a "+.*.previous" matching it. Off hand I can't think of
any exceptions to this rule although I may be wrong.

-- Steve

Re: [PATCH] pop previous section in alternative.c

[Andi Kleen]

> I would say anytime there's a "^+.*\.section" there had better be a
> "^-.*\.section" or a "+.*.previous" matching it. Off hand I can't think o
> any exceptions to this rule although I may be wrong.


Second section is wrong because the compiler expects that the same
section is active afterwards and that can be different ones (like
init.text vs normal text)

Also pushsection/popsection is also valid

So in summary valid section patterns are either
.section / .previous or .pushsection .section .popsection

-Andi

Re: [PATCH] pop previous section in alternative.c

[Steven Rostedt]

On Thu, 10 Apr 2008, Andi Kleen wrote:

> > I would say anytime there's a "^+.*\.section" there had better be a
> > "^-.*\.section" or a "+.*.previous" matching it. Off hand I can't think o
> > any exceptions to this rule although I may be wrong.
>
>
> Second section is wrong because the compiler expects that the same
> section is active afterwards and that can be different ones (like
> init.text vs normal text)

No the second section in my example is not wrong, because it starts with a
"-", which in a patch would be a replacement of one .section with another,
not an addition of two sections.

>
> Also pushsection/popsection is also valid
>
> So in summary valid section patterns are either
> ..section / .previous or .pushsection .section .popsection

But you are right with this part. Yeah, either a .section is replaced, or
is added with a .previous, or is encapsulated with push and popsections.

-- Steve

Re: [PATCH] pop previous section in alternative.c

[Linus Torvalds]

On Thu, 10 Apr 2008, Andi Kleen wrote:
> 
> So in summary valid section patterns are either
> .section / .previous or .pushsection .section .popsection

The thing is, we'd be much better off with some sanity checking in the 
assembler.

Which is likely not going to happen - oh well.

In particular, the assembler should see patterns like

	.size function, .-function

and it should be _trivially_ able to check that "." and "function" are in 
the same section, and warn if they aren't. Because I don't see how it 
could ever be valid to have sizes that cross section boundaries (it's a 
totally nonsensical concept).

But it doesn't. Oh, well.

But maybe we can see it in the resulting object file somehow, and do the 
check there (the same way we do the init-section analysis). I assume the 
.size directive writes some debug info or similar, and we can create a big 
warning when a size is unexpectedly huge and crosses section size 
boundaries?

			Linus

Re: [PATCH] pop previous section in alternative.c

[Steven Rostedt]

On Thu, 10 Apr 2008, Linus Torvalds wrote:
>
> But maybe we can see it in the resulting object file somehow, and do the
> check there (the same way we do the init-section analysis). I assume the
> ..size directive writes some debug info or similar, and we can create a big
> warning when a size is unexpectedly huge and crosses section size
> boundaries?

I'm not sure how much this would help, but where I saw my red flag was
examining the objdump and seeing this:

  89:   bf 69 00 00 00          mov    $0x69,%edi
  8e:   e8 00 00 00 00          callq  93 <alternatives_smp_module_add+0x30>
                        8f: R_X86_64_PC32       .rodata+0x8c


A call to .rodata??

-- Steve

Re: [PATCH] pop previous section in alternative.c

[Andi Kleen]

On Thu, Apr 10, 2008 at 07:46:25AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 10 Apr 2008, Andi Kleen wrote:
> > 
> > So in summary valid section patterns are either
> > .section / .previous or .pushsection .section .popsection
> 
> The thing is, we'd be much better off with some sanity checking in the 
> assembler.
> 
> Which is likely not going to happen - oh well.

A simple way to detect it on the assembler level would be checking
that the section is the same after #NO_APP as before #APP

-Andi

Re: [PATCH] pop previous section in alternative.c

[Linus Torvalds]

On Thu, 10 Apr 2008, Andi Kleen wrote:
> 
> A simple way to detect it on the assembler level would be checking
> that the section is the same after #NO_APP as before #APP

That would mark gcc itself as buggy, because gcc will move some things 
into the #APP/#NO_APP thing, and sometimes doesn't end the #APP at all!

Try gcc -S on this this trivial "program"

	asm("Hello world");

and at least I personally get

	        .file   "bug.c"
	#APP
	        Hello world
	        .ident  "GCC: (GNU) 4.1.2 20070925 (Red Hat 4.1.2-33)"
	        .section        .note.GNU-stack,"",@progbits

and nothing else. Note the lack of #NO_APP ;)

(I also swear I've seen code or data move _into_ the #NO_APP - ie gcc did 
eventually close the #APP section, but did it too late, after it had 
emitted other things itself - but maybe I just dreamed it because I can't 
seem to reproduce it now. Or maybe it's just a historical gcc bug that got 
fixed)

			Linus

Re: [PATCH] pop previous section in alternative.c

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> That would mark gcc itself as buggy, because gcc will move some things 
> into the #APP/#NO_APP thing, and sometimes doesn't end the #APP at all!
> 

That is quite buggy indeed, since the whole #APP and #NO_APP string set 
are arbitrary strings that are part of the gcc configuration, marked as 
"what to insert before user assembly" and "what to insert after user 
assembly."  They are not guaranteed to be simple comments, in fact.

In fact, gas *is* sensitive to #APP ... #NO_APP, so they are not simple 
comments.

	-hpa

Re: [PATCH] pop previous section in alternative.c

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> The thing is, we'd be much better off with some sanity checking in the 
> assembler.
> 
> Which is likely not going to happen - oh well.
> 
> In particular, the assembler should see patterns like
> 
> 	.size function, .-function
> 
> and it should be _trivially_ able to check that "." and "function" are in 
> the same section, and warn if they aren't. Because I don't see how it 
> could ever be valid to have sizes that cross section boundaries (it's a 
> totally nonsensical concept).
> 
> But it doesn't. Oh, well.
> 

Wow.  That is utterly braindamaged on the part of gas.

> But maybe we can see it in the resulting object file somehow, and do the 
> check there (the same way we do the init-section analysis). I assume the 
> .size directive writes some debug info or similar, and we can create a big 
> warning when a size is unexpectedly huge and crosses section size 
> boundaries?

I just tried it, and it output the difference across the sections, 
without any consideration that the base was different.  Didn't know one 
could write an assembler that dumb.

	-hpa

Re: [PATCH] pop previous section in alternative.c

[Steven Rostedt]

On Thu, 10 Apr 2008, H. Peter Anvin wrote:

> Linus Torvalds wrote:
> >
> > That would mark gcc itself as buggy, because gcc will move some things
> > into the #APP/#NO_APP thing, and sometimes doesn't end the #APP at all!
> >
>
> That is quite buggy indeed, since the whole #APP and #NO_APP string set
> are arbitrary strings that are part of the gcc configuration, marked as
> "what to insert before user assembly" and "what to insert after user
> assembly."  They are not guaranteed to be simple comments, in fact.
>
> In fact, gas *is* sensitive to #APP ... #NO_APP, so they are not simple
> comments.

Looking at the output below, shows that testing .sections in between
#APP and #NO_APP would in fact catch this bug.

-- Steve

.Ltext0:
#APP
                .section .rodata, "a"
intelnops: .byte 0x90
.byte 0x89,0xf6
.byte 0x8d,0x76,0x00
.byte 0x8d,0x74,0x26,0x00
.byte 0x90
.byte 0x8d,0x74,0x26,0x00
.byte 0x8d,0xb6,0x00,0x00,0x00,0x00
.byte 0x8d,0xb4,0x26,0x00,0x00,0x00,0x00
.byte 0x90
.byte 0x8d,0xb4,0x26,0x00,0x00,0x00,0x00

                .section .rodata, "a"
k8nops: .byte 0x90
.byte 0x66,0x90
.byte 0x66,0x66,0x90
.byte 0x66,0x66,0x66,0x90
.byte 0x66,0x66,0x90
.byte 0x66,0x90
.byte 0x66,0x66,0x90
.byte 0x66,0x66,0x90
.byte 0x66,0x66,0x66,0x90
.byte 0x66,0x66,0x90
.byte 0x66,0x66,0x66,0x90
.byte 0x66,0x66,0x66,0x90

                .section .rodata, "a"
k7nops: .byte 0x90
.byte 0x8b,0xc0
.byte 0x8d,0x04,0x20
.byte 0x8d,0x44,0x20,0x00
.byte 0x8d,0x44,0x20,0x00
.byte 0x90
.byte 0x8d,0x80,0,0,0,0
.byte 0x8D,0x04,0x05,0,0,0,0
.byte 0x8D,0x04,0x05,0,0,0,0
.byte 0x90

                .section .rodata, "a"
p6nops: .byte 0x90
.byte 0x66,0x90
.byte 0x0f,0x1f,0x00
.byte 0x0f,0x1f,0x40,0
.byte 0x0f,0x1f,0x44,0x00,0
.byte 0x66,0x0f,0x1f,0x44,0x00,0
.byte 0x0f,0x1f,0x80,0,0,0,0
.byte 0x0f,0x1f,0x84,0x00,0,0,0,0

#NO_APP
        .type   constant_test_bit, @function
constant_test_bit:
.LFB30:

Re: [PATCH] pop previous section in alternative.c

[H. Peter Anvin]

Steven Rostedt wrote:
> 
> Looking at the output below, shows that testing .sections in between
> #APP and #NO_APP would in fact catch this bug.
> 

Of course, The Right Thing[TM] would be for gcc to emit .pushsection ... 
.popsection around #APP ... #NO_APP.

	-hpa

Re: [PATCH] pop previous section in alternative.c

[Andrew Pinski]

On Thu, Apr 10, 2008 at 10:07 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>  Of course, The Right Thing[TM] would be for gcc to emit .pushsection ...
> .popsection around #APP ... #NO_APP.

Why??  GCC assumes that people don't do crazy things in their
inline-asm and the source did in this case.

Thanks,
Andrew Pinski

Re: [PATCH] pop previous section in alternative.c

[Linus Torvalds]

On Thu, 10 Apr 2008, H. Peter Anvin wrote:

> Steven Rostedt wrote:
> > 
> > Looking at the output below, shows that testing .sections in between
> > #APP and #NO_APP would in fact catch this bug.
> > 
> 
> Of course, The Right Thing[TM] would be for gcc to emit .pushsection ...
> .popsection around #APP ... #NO_APP.

No, that would not help anything. It would still be open to bugs in the 
asm, ie if there was a unpaired "pushsection" there, making gcc emit the 
section directives around it would just cause a _different_ bug.

So I don't think gcc does the wrong thing per se. It was clearly a bug in 
our inline asm, and the blame is solidly on us. 

Obviously, it would have been really nice if something like the assembler 
had caught it with some simple sanity-test (ie I think the .size thing 
would be a good sanity check _regardless_), so in that sense it's our bug 
that might have been avoided with soem sanity testing, but on the other 
hand, I can well understand that gas didn't do it - since it would matter 
only for totally buggy code that was never emitted by the compiler.

Gas historically used to not do any sanity-checking what-so-ever, and was 
very much meant to be just for compiler output (which is why #APP exists 
in the first place - to mark places that aren't pure compiler input). It's 
actually improved immensely in that area and now is useful as a 
traditional human-usable assembler with lots of support like macros etc.

			Linus

Re: [PATCH] pop previous section in alternative.c

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Obviously, it would have been really nice if something like the assembler 
> had caught it with some simple sanity-test (ie I think the .size thing 
> would be a good sanity check _regardless_), so in that sense it's our bug 
> that might have been avoided with soem sanity testing, but on the other 
> hand, I can well understand that gas didn't do it - since it would matter 
> only for totally buggy code that was never emitted by the compiler.
> 

Agreed.

> Gas historically used to not do any sanity-checking what-so-ever, and was 
> very much meant to be just for compiler output (which is why #APP exists 
> in the first place - to mark places that aren't pure compiler input). It's 
> actually improved immensely in that area and now is useful as a 
> traditional human-usable assembler with lots of support like macros etc.

Indeed it has.

	-hpa

Re: [PATCH] pop previous section in alternative.c

[H. Peter Anvin]

Andrew Pinski wrote:
> On Thu, Apr 10, 2008 at 10:07 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>>  Of course, The Right Thing[TM] would be for gcc to emit .pushsection ...
>> .popsection around #APP ... #NO_APP.
> 
> Why??  GCC assumes that people don't do crazy things in their
> inline-asm and the source did in this case.

Because even good people do bad things sometimes.

	-hpa

Re: [PATCH] pop previous section in alternative.c

[Sam Ravnborg]

On Thu, Apr 10, 2008 at 10:55:44AM -0400, Steven Rostedt wrote:
> 
> On Thu, 10 Apr 2008, Linus Torvalds wrote:
> >
> > But maybe we can see it in the resulting object file somehow, and do the
> > check there (the same way we do the init-section analysis). I assume the
> > ..size directive writes some debug info or similar, and we can create a big
> > warning when a size is unexpectedly huge and crosses section size
> > boundaries?
> 
> I'm not sure how much this would help, but where I saw my red flag was
> examining the objdump and seeing this:
> 
>   89:   bf 69 00 00 00          mov    $0x69,%edi
>   8e:   e8 00 00 00 00          callq  93 <alternatives_smp_module_add+0x30>
>                         8f: R_X86_64_PC32       .rodata+0x8c
> 
> 
> A call to .rodata??
Did you have any relocation record reflecting this reference?
The init_section stuff today rely purely on relocation
records and is easy to extend.

	Sam

Re: [PATCH] pop previous section in alternative.c

[Maciej W. Rozycki]

On Thu, 10 Apr 2008, Andi Kleen wrote:

> A simple way to detect it on the assembler level would be checking
> that the section is the same after #NO_APP as before #APP

 Not as simple as the section stack would have to be traversed to check
whether there are any differences.  And in the end, either such a complete
check or a simple one you have proposed may not be desirable, because I
can imagine in some scenarios the change of a section in an asm may be
deliberate.  Let's not limit the tools.

  Maciej