[PATCH] ARM: move body of head-common.S back to text section

[PATCH] ARM: move body of head-common.S back to text section

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Commit 281ecb7 "arm: delete __cpuinit/__CPUINIT usage from all ARM
users" removed a __CPUINIT from head-common.S. However, the code
immediately before the removed tag was marked __INIT, and was missing
a match __FINIT. This caused code after the removed __CPUINIT to end
up in the init section rather than the main text section as desired.
This caused issues such as secondary CPU boot failures or crashes.
Add the missing __FINIT to solve this.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/kernel/head-common.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
index 529ce99..a9dc804 100644
--- a/arch/arm/kernel/head-common.S
+++ b/arch/arm/kernel/head-common.S
@@ -122,6 +122,8 @@ __mmap_switched_data:
 	.long	init_thread_union + THREAD_START_SP @ sp
 	.size	__mmap_switched_data, . - __mmap_switched_data
 
+	__FINIT
+
 /*
  * This provides a C-API version of __lookup_processor_type
  */
-- 
1.8.1.5

Re: [PATCH] ARM: move body of head-common.S back to text section

[Stephen Boyd]

On 07/02, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Commit 281ecb7 "arm: delete __cpuinit/__CPUINIT usage from all ARM
> users" removed a __CPUINIT from head-common.S. However, the code
> immediately before the removed tag was marked __INIT, and was missing
> a match __FINIT. This caused code after the removed __CPUINIT to end
> up in the init section rather than the main text section as desired.
> This caused issues such as secondary CPU boot failures or crashes.
> Add the missing __FINIT to solve this.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/kernel/head-common.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
> index 529ce99..a9dc804 100644
> --- a/arch/arm/kernel/head-common.S
> +++ b/arch/arm/kernel/head-common.S
> @@ -122,6 +122,8 @@ __mmap_switched_data:
>  	.long	init_thread_union + THREAD_START_SP @ sp
>  	.size	__mmap_switched_data, . - __mmap_switched_data
>  
> +	__FINIT
> +
>  /*
>   * This provides a C-API version of __lookup_processor_type
>   */

Shouldn't this come after lookup_processor_type()? Otherwise you
just removed that function from the init section?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] ARM: move body of head-common.S back to text section

[Stephen Warren]

On 07/02/2013 05:22 PM, Stephen Boyd wrote:
> On 07/02, Stephen Warren wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> Commit 281ecb7 "arm: delete __cpuinit/__CPUINIT usage from all ARM
>> users" removed a __CPUINIT from head-common.S. However, the code
>> immediately before the removed tag was marked __INIT, and was missing
>> a match __FINIT. This caused code after the removed __CPUINIT to end
>> up in the init section rather than the main text section as desired.
>> This caused issues such as secondary CPU boot failures or crashes.
>> Add the missing __FINIT to solve this.
>>
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/arm/kernel/head-common.S | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
>> index 529ce99..a9dc804 100644
>> --- a/arch/arm/kernel/head-common.S
>> +++ b/arch/arm/kernel/head-common.S
>> @@ -122,6 +122,8 @@ __mmap_switched_data:
>>  	.long	init_thread_union + THREAD_START_SP @ sp
>>  	.size	__mmap_switched_data, . - __mmap_switched_data
>>  
>> +	__FINIT
>> +
>>  /*
>>   * This provides a C-API version of __lookup_processor_type
>>   */
> 
> Shouldn't this come after lookup_processor_type()? Otherwise you
> just removed that function from the init section?

I think this is correct. Prior to "arm: delete __cpuinit/__CPUINIT usage
from all ARM users", lookup_processor_type() was in the cpuinit section.
After that commit, it moved to the init section and hence got dumped
soon after boot. The change above moves lookup_processor_type() back
into the regular text section, which I believe was the intent of the
__cpuinit removal patch.

Re: [PATCH] ARM: move body of head-common.S back to text section

[Paul Gortmaker]

[Re: [PATCH] ARM: move body of head-common.S back to text section] On 02/07/2013 (Tue 20:44) Stephen Warren wrote:

> On 07/02/2013 05:22 PM, Stephen Boyd wrote:
> > On 07/02, Stephen Warren wrote:
> >> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>
> >> Commit 281ecb7 "arm: delete __cpuinit/__CPUINIT usage from all ARM

We shouldn't reference this commit since (a) the SHA is from linux-next
and hence is transient, and (b) this commit really isn't the one that
triggers the breakage.  It is the commit where we dummy out the macros
that will reveal the missing __FINIT.

So, assuming Linus takes my pull request here...

https://lkml.org/lkml/2013/7/2/405

...for phase one of the __cpuinit purge, then the SHA you want to
blame for triggering this hidden bug into life is:

22f0a2736 "init.h: remove __cpuinit sections from the kernel" at:

http://git.kernel.org/cgit/linux/kernel/git/paulg/linux.git/log/?h=cpuinit-delete

After you update the commit log (and make the change below), it probably
makes sense for this to go in via one of the ARM trees, since if I take
it, it will go in at the very end of the merge window with the phase two
commits.  If it goes in via an arm tree, the regression window within
3.10 --> 3.11-rc1 will be minimized. (That and it is a pre-existing bug).

> >> users" removed a __CPUINIT from head-common.S. However, the code
> >> immediately before the removed tag was marked __INIT, and was missing
> >> a match __FINIT. This caused code after the removed __CPUINIT to end
> >> up in the init section rather than the main text section as desired.
> >> This caused issues such as secondary CPU boot failures or crashes.
> >> Add the missing __FINIT to solve this.
> >>
> >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  arch/arm/kernel/head-common.S | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
> >> index 529ce99..a9dc804 100644
> >> --- a/arch/arm/kernel/head-common.S
> >> +++ b/arch/arm/kernel/head-common.S
> >> @@ -122,6 +122,8 @@ __mmap_switched_data:
> >>  	.long	init_thread_union + THREAD_START_SP @ sp
> >>  	.size	__mmap_switched_data, . - __mmap_switched_data
> >>  
> >> +	__FINIT
> >> +
> >>  /*
> >>   * This provides a C-API version of __lookup_processor_type
> >>   */
> > 
> > Shouldn't this come after lookup_processor_type()? Otherwise you
> > just removed that function from the init section?
> 
> I think this is correct. Prior to "arm: delete __cpuinit/__CPUINIT usage
> from all ARM users", lookup_processor_type() was in the cpuinit section.

No, it was __lookup_processor_type that was in the cpuinit section.
The non-underscore (C-API) version was __INIT, and if you look at the
one and only calling function in C, it too is tagged __init.  So it
appears to me that was a thought out and deliberate choice.

> After that commit, it moved to the init section and hence got dumped
> soon after boot. The change above moves lookup_processor_type() back
> into the regular text section, which I believe was the intent of the
> __cpuinit removal patch.

Given the above, actually I think StephenB is right here.  The __FINIT
can go exactly where the removed __CPUINIT was (plus or minus a blank
line...)

Thanks for testing, reporting and then tracking down where it was once
you knew what it was that you were looking for (and to Russell for the
quick and accurate diagnosis.)  You can add an Acked-by from me to the
updated patch if you want.

As an aside, I'm now thinking any __INIT that implicitly rely on EOF for
closure are nasty traps waiting to happen and it might be worthwhile to
audit and explicitly __FINIT them before someone appends to the file...

Paul.
--

Re: [PATCH] ARM: move body of head-common.S back to text section

[Russell King - ARM Linux]

On Wed, Jul 03, 2013 at 01:19:07AM -0400, Paul Gortmaker wrote:
> As an aside, I'm now thinking any __INIT that implicitly rely on EOF for
> closure are nasty traps waiting to happen and it might be worthwhile to
> audit and explicitly __FINIT them before someone appends to the file...

That hides a different kind of bug though - I hate __FINIT for exactly
that reason.  Consider this:

	.text
	blah blah blah
	__INIT
	lots of init stuff
	__FINIT
	more .text stuff

Now, someone comes along and modifies this to be:

	.text
	blah blah blah
	.data
	something else
	__INIT
	lots of init stuff
	__FINIT
	more .text stuff

Now, what is the effect of that __FINIT now?  You get the following .text
emitted into the .data section instead.  This is basically the same problem
you've just encounted.

Maybe:

	__FINIT
	.text

is the safest solution - and __FINIT becomes just a no-op marker to avoid
anyone relying on its properties.

Re: [PATCH] ARM: move body of head-common.S back to text section

[Paul Gortmaker]

[Re: [PATCH] ARM: move body of head-common.S back to text section] On 03/07/2013 (Wed 11:00) Russell King - ARM Linux wrote:

> On Wed, Jul 03, 2013 at 01:19:07AM -0400, Paul Gortmaker wrote:
> > As an aside, I'm now thinking any __INIT that implicitly rely on EOF for
> > closure are nasty traps waiting to happen and it might be worthwhile to
> > audit and explicitly __FINIT them before someone appends to the file...
> 
> That hides a different kind of bug though - I hate __FINIT for exactly
> that reason.  Consider this:

Agreed - perhaps masking that it is a ".previous" just hides the fact
that it is more like a pop operation vs. an on/off operation, or per
function as we have in C.

> 
> 	.text
> 	blah blah blah
> 	__INIT
> 	lots of init stuff
> 	__FINIT
> 	more .text stuff
> 
> Now, someone comes along and modifies this to be:
> 
> 	.text
> 	blah blah blah
> 	.data
> 	something else

Yeah, that would be kind of careless; not putting .data above the .text,
or at least closing with a .previous, but sure it could sneak past
review.

> 	__INIT
> 	lots of init stuff
> 	__FINIT

The presence of the above 3 lines of init block (i.e. here or not)
doesn't really change the fact that the .data guy broke the below .text
code by grandfathering it into .data -- But you could argue that him
seeing the 1st __INIT and that influenced him to decide to not read any
further down into the file -- which probably does happen, though.... :(

> 	more .text stuff
> 
> Now, what is the effect of that __FINIT now?  You get the following .text
> emitted into the .data section instead.  This is basically the same problem
> you've just encounted.
> 
> Maybe:
> 
> 	__FINIT
> 	.text
> 
> is the safest solution - and __FINIT becomes just a no-op marker to avoid
> anyone relying on its properties.

That seems reasonable to me.  I can't think of any self auditing that is
reasonably simple to implement.  One downside of __FINIT as a no-op vs.
what it is today, is that a dangling __FINIT in a file with no other
previous sections will emit a warning.  But that is a small low value
corner case I think.

Re: [PATCH] ARM: move body of head-common.S back to text section

[Russell King - ARM Linux]

On Wed, Jul 03, 2013 at 11:30:12AM -0400, Paul Gortmaker wrote:
> [Re: [PATCH] ARM: move body of head-common.S back to text section] On 03/07/2013 (Wed 11:00) Russell King - ARM Linux wrote:
> 
> > On Wed, Jul 03, 2013 at 01:19:07AM -0400, Paul Gortmaker wrote:
> > > As an aside, I'm now thinking any __INIT that implicitly rely on EOF for
> > > closure are nasty traps waiting to happen and it might be worthwhile to
> > > audit and explicitly __FINIT them before someone appends to the file...
> > 
> > That hides a different kind of bug though - I hate __FINIT for exactly
> > that reason.  Consider this:
> 
> Agreed - perhaps masking that it is a ".previous" just hides the fact
> that it is more like a pop operation vs. an on/off operation, or per
> function as we have in C.

I read the info pages, because I thought it was a pop operation too.
I was concerned that .section didn't push the previous section onto the
stack.

However, .popsection is the pseudio-op which pops.  .previous just toggles
the current section with the section immediately before it.

So:

	.text
	.data
	.previous
	/* this is .text */
	.previous
	/* this is .data */
	.previous
	/* this is .text */
	.previous
	/* this is .data */

> That seems reasonable to me.  I can't think of any self auditing that is
> reasonably simple to implement.  One downside of __FINIT as a no-op vs.
> what it is today, is that a dangling __FINIT in a file with no other
> previous sections will emit a warning.  But that is a small low value
> corner case I think.

That warning from __FINIT will only happen if there has been no section
or .text or .data statement in the file at all.  As soon as you have any
statement setting any kind of section, .previous doesn't warn.

So:

	.text
	...
	__FINIT

produces no warning.

Re: [PATCH] ARM: move body of head-common.S back to text section

[Paul Gortmaker]

[Re: [PATCH] ARM: move body of head-common.S back to text section] On 03/07/2013 (Wed 18:20) Russell King - ARM Linux wrote:

> On Wed, Jul 03, 2013 at 11:30:12AM -0400, Paul Gortmaker wrote:
> > [Re: [PATCH] ARM: move body of head-common.S back to text section] On 03/07/2013 (Wed 11:00) Russell King - ARM Linux wrote:
> > 
> > > On Wed, Jul 03, 2013 at 01:19:07AM -0400, Paul Gortmaker wrote:
> > > > As an aside, I'm now thinking any __INIT that implicitly rely on EOF for
> > > > closure are nasty traps waiting to happen and it might be worthwhile to
> > > > audit and explicitly __FINIT them before someone appends to the file...
> > > 
> > > That hides a different kind of bug though - I hate __FINIT for exactly
> > > that reason.  Consider this:
> > 
> > Agreed - perhaps masking that it is a ".previous" just hides the fact
> > that it is more like a pop operation vs. an on/off operation, or per
> > function as we have in C.
> 
> I read the info pages, because I thought it was a pop operation too.
> I was concerned that .section didn't push the previous section onto the
> stack.
> 
> However, .popsection is the pseudio-op which pops.  .previous just toggles
> the current section with the section immediately before it.
> 
> So:
> 
> 	.text
> 	.data
> 	.previous
> 	/* this is .text */
> 	.previous
> 	/* this is .data */
> 	.previous
> 	/* this is .text */
> 	.previous
> 	/* this is .data */

Cool -- I bet we weren't the only ones thinking it was a pop.  Thanks.

Does that make __FINIT less evil than we previously assumed?  I think
your example was the following pseudo-patch:


	.text
	<some text>
+	.data
+ 	<some data>
	__INIT
	<big hunk of init>
	__FINIT
	/* this below used to be text */
	<more stuff that was originally meant for text>

Even if it is a toggle (vs. pop), the end text will now become data,
so the no-op __FINIT with an explicit section called out just below
it may still be the most unambiguous way to indicate what is going on.

> 
> > That seems reasonable to me.  I can't think of any self auditing that is
> > reasonably simple to implement.  One downside of __FINIT as a no-op vs.
> > what it is today, is that a dangling __FINIT in a file with no other
> > previous sections will emit a warning.  But that is a small low value
> > corner case I think.
> 
> That warning from __FINIT will only happen if there has been no section
> or .text or .data statement in the file at all.  As soon as you have any
> statement setting any kind of section, .previous doesn't warn.
> 
> So:
> 
> 	.text
> 	...
> 	__FINIT
> 
> produces no warning.o

Yep -- we are both saying the same thing here - hence why I called it a
small low value corner case.

Re: [PATCH] ARM: move body of head-common.S back to text section

[Dave P Martin]

On Wed, Jul 03, 2013 at 08:22:35PM -0400, Paul Gortmaker wrote:
> [Re: [PATCH] ARM: move body of head-common.S back to text section] On 03/07/2013 (Wed 18:20) Russell King - ARM Linux wrote:
> 
> > On Wed, Jul 03, 2013 at 11:30:12AM -0400, Paul Gortmaker wrote:
> > > [Re: [PATCH] ARM: move body of head-common.S back to text section] On 03/07/2013 (Wed 11:00) Russell King - ARM Linux wrote:
> > > 
> > > > On Wed, Jul 03, 2013 at 01:19:07AM -0400, Paul Gortmaker wrote:
> > > > > As an aside, I'm now thinking any __INIT that implicitly rely on EOF for
> > > > > closure are nasty traps waiting to happen and it might be worthwhile to
> > > > > audit and explicitly __FINIT them before someone appends to the file...
> > > > 
> > > > That hides a different kind of bug though - I hate __FINIT for exactly
> > > > that reason.  Consider this:
> > > 
> > > Agreed - perhaps masking that it is a ".previous" just hides the fact
> > > that it is more like a pop operation vs. an on/off operation, or per
> > > function as we have in C.
> > 
> > I read the info pages, because I thought it was a pop operation too.
> > I was concerned that .section didn't push the previous section onto the
> > stack.
> > 
> > However, .popsection is the pseudio-op which pops.  .previous just toggles
> > the current section with the section immediately before it.
> > 
> > So:
> > 
> > 	.text
> > 	.data
> > 	.previous
> > 	/* this is .text */
> > 	.previous
> > 	/* this is .data */
> > 	.previous
> > 	/* this is .text */
> > 	.previous
> > 	/* this is .data */
> 
> Cool -- I bet we weren't the only ones thinking it was a pop.  Thanks.
> 
> Does that make __FINIT less evil than we previously assumed?  I think
> your example was the following pseudo-patch:
> 
> 
> 	.text
> 	<some text>
> +	.data
> + 	<some data>
> 	__INIT
> 	<big hunk of init>
> 	__FINIT
> 	/* this below used to be text */
> 	<more stuff that was originally meant for text>
> 
> Even if it is a toggle (vs. pop), the end text will now become data,
> so the no-op __FINIT with an explicit section called out just below
> it may still be the most unambiguous way to indicate what is going on.
> 
> > 
> > > That seems reasonable to me.  I can't think of any self auditing that is
> > > reasonably simple to implement.  One downside of __FINIT as a no-op vs.
> > > what it is today, is that a dangling __FINIT in a file with no other
> > > previous sections will emit a warning.  But that is a small low value
> > > corner case I think.
> > 
> > That warning from __FINIT will only happen if there has been no section
> > or .text or .data statement in the file at all.  As soon as you have any
> > statement setting any kind of section, .previous doesn't warn.
> > 
> > So:
> > 
> > 	.text
> > 	...
> > 	__FINIT
> > 
> > produces no warning.o
> 
> Yep -- we are both saying the same thing here - hence why I called it a
> small low value corner case.

Note that .previous has another important gotcha.  Consider:

	__INIT
	/* now in .text.init */
	ALT_UP(...)
	/* now in .text.init */
	__FINIT

	/* now in .alt.smp.text! */


.previous (or macros containing a dangling .previous) shouldn't be used
unless you're absolutely certain what the previous section was.

In general:

	label:
		<stuff>

		.previous

restores to the section which was current at label, only if there are
no section directives in <stuff>, nor anything which could contain a
section directive after macro expansion.

The same goes for the hidden, dangling .previous embedded in __FINIT
and friends.

Cheers
---Dave