Re: [PATCH 18/23] powerpc: Use common syscall handler type

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Sep 16, 2022 at 3:32 PM AEST, Rohan McLure wrote:
> Cause syscall handlers to be typed as follows when called indirectly
> throughout the kernel. This is to allow for better type checking.
>
> typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
>                            unsigned long, unsigned long, unsigned long);
>
> Since both 32 and 64-bit abis allow for at least the first six
> machine-word length parameters to a function to be passed by registers,
> even handlers which admit fewer than six parameters may be viewed as
> having the above type.
>
> Coercing syscalls to syscall_fn requires a cast to void* to avoid
> -Wcast-function-type.

This could possibly be a comment in systbl.c as well?

>
> Fixup comparisons in VDSO to avoid pointer-integer comparison. Introduce
> explicit cast on systems with SPUs.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: New patch.
> V2 -> V3: Remove unnecessary cast from const syscall_fn to syscall_fn
> V4 -> V5: Update patch description.
> ---
>  arch/powerpc/include/asm/syscall.h          | 7 +++++--
>  arch/powerpc/include/asm/syscalls.h         | 1 +
>  arch/powerpc/kernel/systbl.c                | 6 +++---
>  arch/powerpc/kernel/vdso.c                  | 4 ++--
>  arch/powerpc/platforms/cell/spu_callbacks.c | 6 +++---

What's happened to arch/powerpc/kernel/syscall.c? We have
approximately the same type defined there with the same name.
That can just use your new type AFAIKS.

We're hopefully solving the rodata thing separately, so long
as you've got the const there that's enough.

Other than that,

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>  5 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> index 25fc8ad9a27a..d2a8dfd5de33 100644
> --- a/arch/powerpc/include/asm/syscall.h
> +++ b/arch/powerpc/include/asm/syscall.h
> @@ -14,9 +14,12 @@
>  #include <linux/sched.h>
>  #include <linux/thread_info.h>
>  
> +typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
> +			   unsigned long, unsigned long, unsigned long);
> +
>  /* ftrace syscalls requires exporting the sys_call_table */
> -extern const unsigned long sys_call_table[];
> -extern const unsigned long compat_sys_call_table[];
> +extern const syscall_fn sys_call_table[];
> +extern const syscall_fn compat_sys_call_table[];
>  
>  static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
>  {
> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> index 5d106acf7906..cc87168d6ecb 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -8,6 +8,7 @@
>  #include <linux/types.h>
>  #include <linux/compat.h>
>  
> +#include <asm/syscall.h>
>  #ifdef CONFIG_PPC64
>  #include <asm/syscalls_32.h>
>  #endif
> diff --git a/arch/powerpc/kernel/systbl.c b/arch/powerpc/kernel/systbl.c
> index ce52bd2ec292..e5d419822b4e 100644
> --- a/arch/powerpc/kernel/systbl.c
> +++ b/arch/powerpc/kernel/systbl.c
> @@ -16,9 +16,9 @@
>  #include <asm/syscalls.h>
>  
>  #define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
> -#define __SYSCALL(nr, entry) [nr] = (unsigned long) &entry,
> +#define __SYSCALL(nr, entry) [nr] = (void *) entry,
>  
> -const unsigned long sys_call_table[] = {
> +const syscall_fn sys_call_table[] = {
>  #ifdef CONFIG_PPC64
>  #include <asm/syscall_table_64.h>
>  #else
> @@ -29,7 +29,7 @@ const unsigned long sys_call_table[] = {
>  #ifdef CONFIG_COMPAT
>  #undef __SYSCALL_WITH_COMPAT
>  #define __SYSCALL_WITH_COMPAT(nr, native, compat)	__SYSCALL(nr, compat)
> -const unsigned long compat_sys_call_table[] = {
> +const syscall_fn compat_sys_call_table[] = {
>  #include <asm/syscall_table_32.h>
>  };
>  #endif /* CONFIG_COMPAT */
> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
> index bf9574ec26ce..fcca06d200d3 100644
> --- a/arch/powerpc/kernel/vdso.c
> +++ b/arch/powerpc/kernel/vdso.c
> @@ -304,10 +304,10 @@ static void __init vdso_setup_syscall_map(void)
>  	unsigned int i;
>  
>  	for (i = 0; i < NR_syscalls; i++) {
> -		if (sys_call_table[i] != (unsigned long)&sys_ni_syscall)
> +		if (sys_call_table[i] != (void *)&sys_ni_syscall)
>  			vdso_data->syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
>  		if (IS_ENABLED(CONFIG_COMPAT) &&
> -		    compat_sys_call_table[i] != (unsigned long)&sys_ni_syscall)
> +		    compat_sys_call_table[i] != (void *)&sys_ni_syscall)
>  			vdso_data->compat_syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
>  	}
>  }
> diff --git a/arch/powerpc/platforms/cell/spu_callbacks.c b/arch/powerpc/platforms/cell/spu_callbacks.c
> index fe0d8797a00a..e780c14c5733 100644
> --- a/arch/powerpc/platforms/cell/spu_callbacks.c
> +++ b/arch/powerpc/platforms/cell/spu_callbacks.c
> @@ -34,15 +34,15 @@
>   *	mbind, mq_open, ipc, ...
>   */
>  
> -static void *spu_syscall_table[] = {
> +static const syscall_fn spu_syscall_table[] = {
>  #define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
> -#define __SYSCALL(nr, entry) [nr] = entry,
> +#define __SYSCALL(nr, entry) [nr] = (void *) entry,
>  #include <asm/syscall_table_spu.h>
>  };
>  
>  long spu_sys_callback(struct spu_syscall_block *s)
>  {
> -	long (*syscall)(u64 a1, u64 a2, u64 a3, u64 a4, u64 a5, u64 a6);
> +	syscall_fn syscall;
>  
>  	if (s->nr_ret >= ARRAY_SIZE(spu_syscall_table)) {
>  		pr_debug("%s: invalid syscall #%lld", __func__, s->nr_ret);
> -- 
> 2.34.1