powerpc: fix sys_call_table declaration

PATCH In review Romeo Cane 3 participants 4 messages Oct 2, 2014 – Oct 7, 2014 PowerPC

lore

Share & tools

lore

Romeo Cane

author

2014-10-02 14:57 UTC #0 lore raw

PATCH 1/1 powerpc: fix sys_call_table declaration

Declaring sys_call_table as a pointer causes the compiler to generate the wrong lookup code in arch_syscall_addr

Signed-off-by Romeo Cane <[email protected]>

---
 arch/powerpc/include/asm/syscall.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/include/asm/syscall.h +1 -1

index b54b2ad..528ba9d 100644
@@ -17,7 +17,7 @@
 
 /* ftrace syscalls requires exporting the sys_call_table */
 #ifdef CONFIG_FTRACE_SYSCALLS
-extern const unsigned long *sys_call_table;
+extern const unsigned long sys_call_table[];
 #endif /* CONFIG_FTRACE_SYSCALLS */
 
 static inline long syscall_get_nr(struct task_struct *task,

Signature · 1 line

1.8.3.2

Benjamin Herrenschmidt

2014-10-02 21:34 UTC #1 lore raw

Quoted 1 line · Romeo Cane

Declaring sys_call_table as a pointer causes the compiler to generate the wrong lookup code in arch_syscall_addr

Care to elaborate ?

Ben.

Quoted 18 lines

Signed-off-by Romeo Cane <[email protected]>

---
 arch/powerpc/include/asm/syscall.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/include/asm/syscall.h +1 -1

index b54b2ad..528ba9d 100644
@@ -17,7 +17,7 @@
 
 /* ftrace syscalls requires exporting the sys_call_table */
 #ifdef CONFIG_FTRACE_SYSCALLS
-extern const unsigned long *sys_call_table;
+extern const unsigned long sys_call_table[];
 #endif /* CONFIG_FTRACE_SYSCALLS */
 
 static inline long syscall_get_nr(struct task_struct *task,

the goal of the function is to retrieve the n-th element of the table (i.e. the address of a syscall)
Without the patch, the returned value is in fact the memory content pointed by the address of the first syscall plus an offset, that is not what we want. The consequence is that ftrace of syscalls doesn't work.

That table has always been declared as a pointer since the support for syscalls tracing has been introduced for powerpc years ago, so I'm wondering why nobody else had this problem before. Other architectures are not affected since in their includes the table is already declared as an array.

Romeo

Michael Ellerman

reviewer

2014-10-07 12:15 UTC #3 lore raw

Quoted 54 lines · Romeo Cane

Quoted 11 lines · Benjamin Herrenschmidt
Quoted 1 line · Romeo Cane
Declaring sys_call_table as a pointer causes the compiler to generate the wrong lookup code in arch_syscall_addr
Care to elaborate ?
Quoted 6 lines
arch/powerpc/include/asm/syscall.h +0 -0
index b54b2ad..528ba9d 100644
@@ -17,7 +17,7 @@
 
Quoted 7 lines
arch/powerpc/include/asm/syscall.h line 23
Quoted 7 lines
arch/powerpc/include/asm/syscall.h +1 -1
 /* ftrace syscalls requires exporting the sys_call_table */
 #ifdef CONFIG_FTRACE_SYSCALLS
-extern const unsigned long *sys_call_table;
+extern const unsigned long sys_call_table[];
 #endif /* CONFIG_FTRACE_SYSCALLS */
 
 static inline long syscall_get_nr(struct task_struct *task,
Hi Ben,
this is the arch_syscall_addr function from kernel/trace/trace_syscalls.c:
unsigned long __init __weak arch_syscall_addr(int nr)
{
    return (unsigned long)sys_call_table[nr];
}
on my platform (E500MC) the generated assembly code is as follows:
without the patch:
<arch_syscall_addr>:
     lis     r9,-16384
     rlwinm  r3,r3,2,0,29
     lwz     r11,30640(r9)
     lwzx    r3,r11,r3
blr
with the patch:
<arch_syscall_addr>:
     lis     r9,-16384
     rlwinm  r3,r3,2,0,29
     addi    r9,r9,30640
     lwzx    r3,r9,r3
blr
the goal of the function is to retrieve the n-th element of the table (i.e. the address of a syscall)
Without the patch, the returned value is in fact the memory content pointed by the address of the first syscall plus an offset, that is not what we want. The consequence is that ftrace of syscalls doesn't work.

Quoted 5 lines

That table has always been declared as a pointer since the support for syscalls tracing has been introduced for powerpc years ago, so I'm wondering why nobody else had this problem before.
Other architectures are not affected since in their includes the table is already declared as an array.

Yeah looks like you're right.

I've only ever used the raw_syscall tracing, which does work.

Worringly we also use sys_call_table as extern unsigned long * in vdso.c, so I wonder if that is also broken.

cheers