[PATCH] powerpc/misc: fix exported functions that reference the TOC

[PATCH] powerpc/misc: fix exported functions that reference the TOC

[Oliver O'Halloran]

When the kernel is compiled to use 64bit ABIv2 the _GLOBAL() macro does not
include a global entry point. A function's global entry point is used when the
function is called from a different TOC context and in the kernel this
typically means a call from a module into the vmlinux (or vis-a-vis).

There are a few exported ASM functions declared with _GLOBAL() and calling
them from a module will module will likely crash the kernel since any TOC
relative load will yield garbage.

To fix this use _GLOBAL_TOC() for exported asm functions rather than _GLOBAL()
and some documentation about when to use each.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/include/asm/ppc_asm.h | 12 ++++++++++++
 arch/powerpc/kernel/misc_64.S      |  4 ++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index 359c443..3abf8c3 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -198,6 +198,18 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 
 #ifdef PPC64_ELF_ABI_v2
 
+/*
+ * When to use _GLOBAL_TOC() instead of _GLOBAL():
+ *
+ * a) The function is exported using EXPORT_SYMBOL_*()
+ *          *and*
+ * b) The function, or any function that it calls, references the TOC.
+ *
+ * In this situation _GLOBAL_TOC() is required because exported functions are
+ * callable from modules which may a different TOC to the kernel proper and the
+ * _GLOBAL() macro skips the TOC setup which is required on ELF ABIv2.
+ */
+
 #define _GLOBAL(name) \
 	.align 2 ; \
 	.type name,@function; \
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index ec94aef..d18da8c 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -67,7 +67,7 @@ PPC64_CACHES:
  *   flush all bytes from start through stop-1 inclusive
  */
 
-_GLOBAL(flush_icache_range)
+_GLOBAL_TOC(flush_icache_range)
 BEGIN_FTR_SECTION
 	PURGE_PREFETCHED_INS
 	blr
@@ -120,7 +120,7 @@ EXPORT_SYMBOL(flush_icache_range)
  *
  *    flush all bytes from start to stop-1 inclusive
  */
-_GLOBAL(flush_dcache_range)
+_GLOBAL_TOC(flush_dcache_range)
 
 /*
  * Flush the data cache to memory 
-- 
2.9.3

Re: [PATCH] powerpc/misc: fix exported functions that reference the TOC

[Michael Ellerman]

Oliver O'Halloran <oohall@gmail.com> writes:

> When the kernel is compiled to use 64bit ABIv2 the _GLOBAL() macro does not
> include a global entry point. A function's global entry point is used when the
> function is called from a different TOC context and in the kernel this
> typically means a call from a module into the vmlinux (or vis-a-vis).
>
> There are a few exported ASM functions declared with _GLOBAL() and calling
> them from a module will module will likely crash the kernel since any TOC
> relative load will yield garbage.
>
> To fix this use _GLOBAL_TOC() for exported asm functions rather than _GLOBAL()
> and some documentation about when to use each.

I wonder if we should just change _GLOBAL() to include the global entry
point. Persisting with _GLOBAL_TOC() seems like it's just going to be a
game of whack-a-mole.

Just grepping now I see ~50 functions defined with _GLOBAL() which are
also EXPORT'ed. Now presumably none of them use the TOC? But there's no
way to verify it, other than inspecting each one, and there's no way to
ensure we don't break it again in future.

The other option would be just to make a rule that anything EXPORT'ed
must use _GLOBAL_TOC().

cheers

Re: [PATCH] powerpc/misc: fix exported functions that reference the TOC

[Benjamin Herrenschmidt]

On Mon, 2017-04-03 at 23:29 +1000, Michael Ellerman wrote:
> The other option would be just to make a rule that anything EXPORT'ed
> must use _GLOBAL_TOC().

Can we enforce that somewhat at build time ?

Cheers,
Ben.

Re: [PATCH] powerpc/misc: fix exported functions that reference the TOC

[Michael Ellerman]

Benjamin Herrenschmidt <benh@au1.ibm.com> writes:

> On Mon, 2017-04-03 at 23:29 +1000, Michael Ellerman wrote:
>> The other option would be just to make a rule that anything EXPORT'ed
>> must use _GLOBAL_TOC().
>
> Can we enforce that somewhat at build time ?

Yeah I had a quick look at doing that last night but didn't see anything
obvious.

So yes we could, but perhaps not easily - EXPORT_SYMBOL() is generic
code so changing that could be a pain.

cheers

Re: [PATCH] powerpc/misc: fix exported functions that reference the TOC

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Oliver O'Halloran <oohall@gmail.com> writes:
>
>> When the kernel is compiled to use 64bit ABIv2 the _GLOBAL() macro does not
>> include a global entry point. A function's global entry point is used when the
>> function is called from a different TOC context and in the kernel this
>> typically means a call from a module into the vmlinux (or vis-a-vis).
>>
>> There are a few exported ASM functions declared with _GLOBAL() and calling
>> them from a module will module will likely crash the kernel since any TOC
>> relative load will yield garbage.
>>
>> To fix this use _GLOBAL_TOC() for exported asm functions rather than _GLOBAL()
>> and some documentation about when to use each.
>
> I wonder if we should just change _GLOBAL() to include the global entry
> point. Persisting with _GLOBAL_TOC() seems like it's just going to be a
> game of whack-a-mole.

Turns out that doesn't work well at all.

We have *a lot* of places that use _GLOBAL() to make a symbol global,
but which aren't functions entry points, eg:

  	.align	7
  _GLOBAL(ret_from_except)
  	ld	r11,_TRAP(r1)
  	andi.	r0,r11,1
  	bne	ret_from_except_lite
  	REST_NVGPRS(r1)
  
  _GLOBAL(ret_from_except_lite)
  	...


Because ret_from_except wants to fallthrough there, we can't have
_GLOBAL() insert instructions at ret_from_except_lite.

So for now we'll merge a version of this fix.

In the medium term we should change all our asm functions to use
FUNC_START(), so it's clear they are functions, and then FUNC_START()
should add the global entry point.

We can then have a hard rule that anything EXPORT_SYMBOL'ed should use
FUNC_START().

cheers