[3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

Message ID 1303272904-31392-4-git-send-email-nicolas.pitre@linaro.org
State New
Headers show

Commit Message

Nicolas Pitre April 20, 2011, 4:15 a.m.
Many architecture specific versions of uncompress.h make use of global
variables marked static.  In such case the GOT is bypassed and the
code in head.S can't relocate them.

Instead of removing the static keyword from all those files and hope that
it won't come back, let's simply
define it out.

Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
---

This should fix remaining issues some people have with the DT append patch.

 arch/arm/boot/compressed/misc.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

Comments

Uwe Kleine-König April 20, 2011, 8:42 a.m. | #1
On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> Many architecture specific versions of uncompress.h make use of global
> variables marked static.  In such case the GOT is bypassed and the
> code in head.S can't relocate them.
> 
> Instead of removing the static keyword from all those files and hope that
> it won't come back, let's simply
> define it out.
> 
> Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> ---
> 
> This should fix remaining issues some people have with the DT append patch.
> 
>  arch/arm/boot/compressed/misc.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> index a565853..0125dae 100644
> --- a/arch/arm/boot/compressed/misc.c
> +++ b/arch/arm/boot/compressed/misc.c
> @@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
>  static void putstr(const char *ptr);
>  extern void error(char *x);
>  
> +/*
> + * Many instances of mach/uncompress.h are including global variables.
> + * Contrary to standard usage, we should _not_ mark those variables
> + * static otherwise they get accessed via GOTOFF references which cannot
> + * be modified at run time.  The entry code in head.S relies on the ability
> + * to move writable sections around, and for that to work, we must have all
> + * references going through the GOT which works only with non static
> + * variables.  So, instead of asking for a non intuitive requirement
> + * making many files non standard according to accepted coding practices
> + * we fix the issue here by simply defining the static keyword to nothing.
> + */
> +#define static /* non-static */
>  #include <mach/uncompress.h>
> +#undef static
This has a strange side effect, i.e. 

	static something *ptr;

isn't initialised to NULL anymore IIRC. So the maintainers of
the various mach/uncompress.h still need to be aware of the issue.

Best regards
Uwe
Shawn Guo April 20, 2011, 9:19 a.m. | #2
On Wed, Apr 20, 2011 at 10:42:34AM +0200, Uwe Kleine-König wrote:
[...]
> > +/*
> > + * Many instances of mach/uncompress.h are including global variables.
> > + * Contrary to standard usage, we should _not_ mark those variables
> > + * static otherwise they get accessed via GOTOFF references which cannot
> > + * be modified at run time.  The entry code in head.S relies on the ability
> > + * to move writable sections around, and for that to work, we must have all
> > + * references going through the GOT which works only with non static
> > + * variables.  So, instead of asking for a non intuitive requirement
> > + * making many files non standard according to accepted coding practices
> > + * we fix the issue here by simply defining the static keyword to nothing.
> > + */
> > +#define static /* non-static */
> >  #include <mach/uncompress.h>
> > +#undef static
> This has a strange side effect, i.e. 
> 
> 	static something *ptr;
> 
> isn't initialised to NULL anymore IIRC. So the maintainers of
> the various mach/uncompress.h still need to be aware of the issue.
> 
Also, the static attribute of functions gets lost, though it may not
matter.
Nicolas Pitre April 20, 2011, 12:28 p.m. | #3
On Wed, 20 Apr 2011, Uwe Kleine-König wrote:

> On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > Many architecture specific versions of uncompress.h make use of global
> > variables marked static.  In such case the GOT is bypassed and the
> > code in head.S can't relocate them.
> > 
> > Instead of removing the static keyword from all those files and hope that
> > it won't come back, let's simply
> > define it out.
> > 
> > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> > ---
> > 
> > This should fix remaining issues some people have with the DT append patch.
> > 
> >  arch/arm/boot/compressed/misc.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> > index a565853..0125dae 100644
> > --- a/arch/arm/boot/compressed/misc.c
> > +++ b/arch/arm/boot/compressed/misc.c
> > @@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
> >  static void putstr(const char *ptr);
> >  extern void error(char *x);
> >  
> > +/*
> > + * Many instances of mach/uncompress.h are including global variables.
> > + * Contrary to standard usage, we should _not_ mark those variables
> > + * static otherwise they get accessed via GOTOFF references which cannot
> > + * be modified at run time.  The entry code in head.S relies on the ability
> > + * to move writable sections around, and for that to work, we must have all
> > + * references going through the GOT which works only with non static
> > + * variables.  So, instead of asking for a non intuitive requirement
> > + * making many files non standard according to accepted coding practices
> > + * we fix the issue here by simply defining the static keyword to nothing.
> > + */
> > +#define static /* non-static */
> >  #include <mach/uncompress.h>
> > +#undef static
> This has a strange side effect, i.e. 
> 
> 	static something *ptr;
> 
> isn't initialised to NULL anymore IIRC. So the maintainers of
> the various mach/uncompress.h still need to be aware of the issue.

Please tell me more about your setup.  This should still be initialized 
to NULL.  If not then something else is still wrong.


Nicolas
Uwe Kleine-König April 20, 2011, 12:36 p.m. | #4
Hello,

On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> On Wed, 20 Apr 2011, Uwe Kleine-König wrote:
> > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > +#define static /* non-static */
> > >  #include <mach/uncompress.h>
> > > +#undef static
> > This has a strange side effect, i.e. 
> > 
> > 	static something *ptr;
> > 
> > isn't initialised to NULL anymore IIRC. So the maintainers of
> > the various mach/uncompress.h still need to be aware of the issue.
> 
> Please tell me more about your setup.  This should still be initialized 
> to NULL.  If not then something else is still wrong.
I didn't test your branch. If you say that non-static data is
zero-initialised, too, I believe you without further checking.

Best regards
Uwe
Nicolas Pitre April 20, 2011, 12:47 p.m. | #5
On Wed, 20 Apr 2011, Uwe Kleine-König wrote:

> Hello,
> 
> On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> > On Wed, 20 Apr 2011, Uwe Kleine-König wrote:
> > > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > > +#define static /* non-static */
> > > >  #include <mach/uncompress.h>
> > > > +#undef static
> > > This has a strange side effect, i.e. 
> > > 
> > > 	static something *ptr;
> > > 
> > > isn't initialised to NULL anymore IIRC. So the maintainers of
> > > the various mach/uncompress.h still need to be aware of the issue.
> > 
> > Please tell me more about your setup.  This should still be initialized 
> > to NULL.  If not then something else is still wrong.
> I didn't test your branch. If you say that non-static data is
> zero-initialised, too, I believe you without further checking.

Uninitialized variables are allocated to the .bss section, regardless if 
they're static or not.  The static keyword only affects the global 
visibility of the variable, and as a side effect the optimization that 
can be performed on that access.  Here the goal is to prevent gcc from 
applying such optimizations.


Nicolas
Shawn Guo April 20, 2011, 12:52 p.m. | #6
On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> On Wed, 20 Apr 2011, Uwe Kleine-König wrote:
> 
> > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > Many architecture specific versions of uncompress.h make use of global
> > > variables marked static.  In such case the GOT is bypassed and the
> > > code in head.S can't relocate them.
> > > 
> > > Instead of removing the static keyword from all those files and hope that
> > > it won't come back, let's simply
> > > define it out.
> > > 
> > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > ---
> > > 
> > > This should fix remaining issues some people have with the DT append patch.
> > > 
> > >  arch/arm/boot/compressed/misc.c |   13 +++++++++++++
> > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> > > index a565853..0125dae 100644
> > > --- a/arch/arm/boot/compressed/misc.c
> > > +++ b/arch/arm/boot/compressed/misc.c
> > > @@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
> > >  static void putstr(const char *ptr);
> > >  extern void error(char *x);
> > >  
> > > +/*
> > > + * Many instances of mach/uncompress.h are including global variables.
> > > + * Contrary to standard usage, we should _not_ mark those variables
> > > + * static otherwise they get accessed via GOTOFF references which cannot
> > > + * be modified at run time.  The entry code in head.S relies on the ability
> > > + * to move writable sections around, and for that to work, we must have all
> > > + * references going through the GOT which works only with non static
> > > + * variables.  So, instead of asking for a non intuitive requirement
> > > + * making many files non standard according to accepted coding practices
> > > + * we fix the issue here by simply defining the static keyword to nothing.
> > > + */
> > > +#define static /* non-static */
> > >  #include <mach/uncompress.h>
> > > +#undef static
> > This has a strange side effect, i.e. 
> > 
> > 	static something *ptr;
> > 
> > isn't initialised to NULL anymore IIRC. So the maintainers of
> > the various mach/uncompress.h still need to be aware of the issue.
> 
> Please tell me more about your setup.  This should still be initialized 
> to NULL.  If not then something else is still wrong.
> 
Though I'm unsure if it realistically exists, what if it's a static
variable inside a function?  It's actually changing something, right?
Nicolas Pitre April 20, 2011, 1:03 p.m. | #7
On Wed, 20 Apr 2011, Shawn Guo wrote:

> On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> > On Wed, 20 Apr 2011, Uwe Kleine-König wrote:
> > 
> > > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > > Many architecture specific versions of uncompress.h make use of global
> > > > variables marked static.  In such case the GOT is bypassed and the
> > > > code in head.S can't relocate them.
> > > > 
> > > > Instead of removing the static keyword from all those files and hope that
> > > > it won't come back, let's simply
> > > > define it out.
> > > > 
> > > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > > ---
> > > > 
> > > > This should fix remaining issues some people have with the DT append patch.
> > > > 
> > > >  arch/arm/boot/compressed/misc.c |   13 +++++++++++++
> > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> > > > index a565853..0125dae 100644
> > > > --- a/arch/arm/boot/compressed/misc.c
> > > > +++ b/arch/arm/boot/compressed/misc.c
> > > > @@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
> > > >  static void putstr(const char *ptr);
> > > >  extern void error(char *x);
> > > >  
> > > > +/*
> > > > + * Many instances of mach/uncompress.h are including global variables.
> > > > + * Contrary to standard usage, we should _not_ mark those variables
> > > > + * static otherwise they get accessed via GOTOFF references which cannot
> > > > + * be modified at run time.  The entry code in head.S relies on the ability
> > > > + * to move writable sections around, and for that to work, we must have all
> > > > + * references going through the GOT which works only with non static
> > > > + * variables.  So, instead of asking for a non intuitive requirement
> > > > + * making many files non standard according to accepted coding practices
> > > > + * we fix the issue here by simply defining the static keyword to nothing.
> > > > + */
> > > > +#define static /* non-static */
> > > >  #include <mach/uncompress.h>
> > > > +#undef static
> > > This has a strange side effect, i.e. 
> > > 
> > > 	static something *ptr;
> > > 
> > > isn't initialised to NULL anymore IIRC. So the maintainers of
> > > the various mach/uncompress.h still need to be aware of the issue.
> > 
> > Please tell me more about your setup.  This should still be initialized 
> > to NULL.  If not then something else is still wrong.
> > 
> Though I'm unsure if it realistically exists, what if it's a static
> variable inside a function?  It's actually changing something, right?

Right.  In that case the variable is allocated to the .data section, but 
we explicitly discard .data at link time to prevent a successful build.


Nicolas
Nicolas Pitre April 20, 2011, 1:17 p.m. | #8
On Wed, 20 Apr 2011, Nicolas Pitre wrote:

> On Wed, 20 Apr 2011, Shawn Guo wrote:
> 
> > Though I'm unsure if it realistically exists, what if it's a static
> > variable inside a function?  It's actually changing something, right?
> 
> Right.  In that case the variable is allocated to the .data section, but 
> we explicitly discard .data at link time to prevent a successful build.

Sorry, more clarification is needed here.

The patch changing all occurrences of "static" into nothing would break 
code using static variables within functions.  I just hope that no one 
is using local static variables in their uncompress.h file.

And local static variables are always using GOTOFF relocations which is 
what we want to avoid in the first place.

The conclusion is that no one should be using local static variables in 
code linked by the kernel decompressor, unless those variables are also 
marked const.


Nicolas
Tony Lindgren April 21, 2011, 12:50 p.m. | #9
* Nicolas Pitre <nicolas.pitre@linaro.org> [110420 07:12]:
> Many architecture specific versions of uncompress.h make use of global
> variables marked static.  In such case the GOT is bypassed and the
> code in head.S can't relocate them.
> 
> Instead of removing the static keyword from all those files and hope that
> it won't come back, let's simply
> define it out.
> 
> Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> ---
> 
> This should fix remaining issues some people have with the DT append patch.

This fixes the issue where the beginning of the DT data got overwritten
by the uncompressed kernel:

Tested-by: Tony Lindgren <tony@atomide.com>
Russell King - ARM Linux April 27, 2011, 8:55 a.m. | #10
On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> Many architecture specific versions of uncompress.h make use of global
> variables marked static.  In such case the GOT is bypassed and the
> code in head.S can't relocate them.

No, we're not doing this - it creates confusion with static variables
declared inside functions.

It's far easier to audit for the presence of 'static' than it is for
the possibility of an ignored 'static' inside a function body.

Ensure that the uncompress.h header files are right.
Russell King - ARM Linux April 27, 2011, 8:58 a.m. | #11
On Wed, Apr 20, 2011 at 08:47:17AM -0400, Nicolas Pitre wrote:
> Uninitialized variables are allocated to the .bss section, regardless if 
> they're static or not.  The static keyword only affects the global 
> visibility of the variable,

Wrong.

int foo(void)
{
	static int bar = 1;
	return bar++;
}

With static allowed to have its normal meaning, this function will return
1 on the first call, 2 on the second, etc.  With static defined away, it
will always return 1.  That's a behavioural change.

Moreover, if we have:

int foo(void)
{
	static int table[] = { ... };
}

then with static defined away, this turns into a runtime memcpy from
read-only data to read-write data each time the function is called.

Defining away static causes this kind of undesirable (and often hidden)
effect - and in these cases it most certainly is not just about variable
visibility.
Nicolas Pitre April 27, 2011, 2:55 p.m. | #12
On Wed, 27 Apr 2011, Russell King - ARM Linux wrote:

> On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > Many architecture specific versions of uncompress.h make use of global
> > variables marked static.  In such case the GOT is bypassed and the
> > code in head.S can't relocate them.
> 
> No, we're not doing this - it creates confusion with static variables
> declared inside functions.

Yes, but in such case those variables would be allocated to the .data 
section, and we're already discarding .data up front to prevent its 
usage.  So there might not be static variables within functions used 
now, unless they're also const.

However I've discovered that static variables within functions with an 
initial value of zero are instead allocated in the .bss section, and 
always using a GOTOFF relocation.  So those can't be used either and 
right now there is nothing preventing them.

Anyway I've discarded this patch as it also makes problems with 
architecture code including standard kernel header files containing 
static inline functions.

> It's far easier to audit for the presence of 'static' than it is for
> the possibility of an ignored 'static' inside a function body.
> 
> Ensure that the uncompress.h header files are right.

Exact.  And I now have a patch failing the build if the wrong usage is 
made ending up in GOTOFF relocs that we can't support which is even 
better than manual auditing.


Nicolas
Nicolas Pitre April 27, 2011, 3 p.m. | #13
On Wed, 27 Apr 2011, Russell King - ARM Linux wrote:

> On Wed, Apr 20, 2011 at 08:47:17AM -0400, Nicolas Pitre wrote:
> > Uninitialized variables are allocated to the .bss section, regardless if 
> > they're static or not.  The static keyword only affects the global 
> > visibility of the variable,
> 
> Wrong.
> 
> int foo(void)
> {
> 	static int bar = 1;
> 	return bar++;
> }

I was talking about global variables.  Variables within a function are 
always visible to the local scope, static or not.

> With static allowed to have its normal meaning, this function will return
> 1 on the first call, 2 on the second, etc.  With static defined away, it
> will always return 1.  That's a behavioural change.

Exact.  And I've dropped that patch.  That doesn't mean that static 
variables within functions are always OK in the context of zImage 
though, as they are always turned into GOTOFF relocs.


Nicolas

Patch

diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
index a565853..0125dae 100644
--- a/arch/arm/boot/compressed/misc.c
+++ b/arch/arm/boot/compressed/misc.c
@@ -30,7 +30,20 @@  unsigned int __machine_arch_type;
 static void putstr(const char *ptr);
 extern void error(char *x);
 
+/*
+ * Many instances of mach/uncompress.h are including global variables.
+ * Contrary to standard usage, we should _not_ mark those variables
+ * static otherwise they get accessed via GOTOFF references which cannot
+ * be modified at run time.  The entry code in head.S relies on the ability
+ * to move writable sections around, and for that to work, we must have all
+ * references going through the GOT which works only with non static
+ * variables.  So, instead of asking for a non intuitive requirement
+ * making many files non standard according to accepted coding practices
+ * we fix the issue here by simply defining the static keyword to nothing.
+ */
+#define static /* non-static */
 #include <mach/uncompress.h>
+#undef static
 
 #ifdef CONFIG_DEBUG_ICEDCC