Re: [PATCH v2] m68k: use conventional function parameters for do_sigreturn

Greg Ungerer
2016-02-24T01:10:09Z

Hi Geert,

Do you have any objections or issues with this change?

Andreas and Philippe kindly gave feedback, which I have addressed
(I think to everyone's satisfaction?)

I would very much like to see this applied to resolve the compilation
issue for non-MMU with modern versions of gcc. I don't mind pushing
through my tree. I just want to make sure first that nobody objects
to this change or has any remaining issues with it.

Regards
Greg

On 15/02/16 16:36, Greg Ungerer wrote:
> Create conventional stack parameters for the calls to do_sigreturn and
> do_rt_sigreturn. The current C code for do_sigreturn and do_rt_sigreturn
> dig into the stack to create local pointers to the saved switch stack
> and the pt_regs structs.
>
> The motivation for this change is a problem with non-MMU targets that
> have broken signal return paths on newer versions of gcc. It appears as
> though gcc has determined that the pointers into the saved stack structs,
> and the saved structs themselves, are function parameters and updates to
> them will be lost on function return, so they are optimized away. This
> results in large parts of restore_sigcontext() and mangle_kernel_stack()
> functions being removed. Of course this results in non-functional code
> causing kernel oops. This problem has been observed with gcc version
> 5.2 and 5.3, and probably exists in earlier versions as well.
>
> Using conventional stack parameter pointers passed to these functions has
> the advantage of the code here not needing to know the exact details of
> how the underlying entry handler layed these structs out on the stack.
> So the rather ugly pointer setup casting and arg referencing can be
> removed.
>
> The resulting code after this change is a few bytes larger (due to the
> overhead of creating the stack args and their tear down). Not being hot
> paths I don't think this is too much of a problem here.
>
> An alternative solution is to put a barrier() in the do_sigreturn() code,
> but this doesn't feel quite as clean as this solution.
>
> This change has been compile tested on all defconfigs, and run tested on
> Atari (through aranym), ColdFire with MMU (M5407EVB) and ColdFire with
> no-MMU (QEMU and M5208EVB).
>
> Signed-off-by: Greg Ungerer <gerg@uclinux.org>
> ---
> v2: reworded the commit log message with better description of problem
>
> arch/m68k/kernel/entry.S | 6 ++++++
> arch/m68k/kernel/signal.c | 8 ++------
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
> index b54ac7a..97cd3ea 100644
> --- a/arch/m68k/kernel/entry.S
> +++ b/arch/m68k/kernel/entry.S
> @@ -71,13 +71,19 @@ ENTRY(__sys_vfork)
>
> ENTRY(sys_sigreturn)
> SAVE_SWITCH_STACK
> + movel %sp,%sp@- | switch_stack pointer
> + pea %sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer
> jbsr do_sigreturn
> + addql #8,%sp
> RESTORE_SWITCH_STACK
> rts
>
> ENTRY(sys_rt_sigreturn)
> SAVE_SWITCH_STACK
> + movel %sp,%sp@- | switch_stack pointer
> + pea %sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer
> jbsr do_rt_sigreturn
> + addql #8,%sp
> RESTORE_SWITCH_STACK
> rts
>
> diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
> index af1c4f3..2dcee3a 100644
> --- a/arch/m68k/kernel/signal.c
> +++ b/arch/m68k/kernel/signal.c
> @@ -737,10 +737,8 @@ badframe:
> return 1;
> }
>
> -asmlinkage int do_sigreturn(unsigned long __unused)
> +asmlinkage int do_sigreturn(struct pt_regs *regs, struct switch_stack *sw)
> {
> - struct switch_stack *sw = (struct switch_stack *) &__unused;
> - struct pt_regs *regs = (struct pt_regs *) (sw + 1);
> unsigned long usp = rdusp();
> struct sigframe __user *frame = (struct sigframe __user *)(usp - 4);
> sigset_t set;
> @@ -764,10 +762,8 @@ badframe:
> return 0;
> }
>
> -asmlinkage int do_rt_sigreturn(unsigned long __unused)
> +asmlinkage int do_rt_sigreturn(struct pt_regs *regs, struct switch_stack *sw)
> {
> - struct switch_stack *sw = (struct switch_stack *) &__unused;
> - struct pt_regs *regs = (struct pt_regs *) (sw + 1);
> unsigned long usp = rdusp();
> struct rt_sigframe __user *frame = (struct rt_sigframe __user *)(usp - 4);
> sigset_t set;
>