| Message ID | 1372805629-18382-1-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/02, Stephen Warren wrote: > 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 > */ Shouldn't this come after lookup_processor_type()? Otherwise you just removed that function from the init section?
On 07/02/2013 05:22 PM, Stephen Boyd wrote: > On 07/02, Stephen Warren wrote: >> 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 >> */ > > 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] 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@nvidia.com> > >> > >> 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@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 > >> */ > > > > 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. --
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] 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.
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] 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.
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
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 */