| Message ID | 1303272904-31392-4-git-send-email-nicolas.pitre@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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
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?
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
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
* 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>
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.
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.
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
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
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
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(-)