Hi Greg, On Mon, Feb 15, 2016 at 11:06:15PM +1000, Greg Ungerer wrote: > Hi Philippe, > > On 15/02/16 20:03, Philippe De Muyter wrote: >> On Mon, Feb 15, 2016 at 04:36:29PM +1000, 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. >> >> As an intermediate solution, I think that you can avoid part of that >> overhead ... >> >>> >>> 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 <[email protected]> >>> --- >>> 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 >> >> if you remove the above asm instruction ... >> >>> jbsr do_sigreturn >>> + addql #8,%sp >> >> change the 8 by 4 here ... >> >>> 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 >> >> ditto here ... >> >>> jbsr do_rt_sigreturn >>> + addql #8,%sp >> >> and here ... >> >>> 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) >> >> and if you use the following prototype : >> asmlinkage int do_sigreturn(struct switch_stack *sw) ... >> >>> { >>> - struct switch_stack *sw = (struct switch_stack *) &__unused; >>> - struct pt_regs *regs = (struct pt_regs *) (sw + 1); >> >> and keep the 'C' instruction above >> >>> 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) >> same here >>> { >>> - struct switch_stack *sw = (struct switch_stack *) &__unused; >>> - struct pt_regs *regs = (struct pt_regs *) (sw + 1); >> and here >>> unsigned long usp = rdusp(); >>> struct rt_sigframe __user *frame = (struct rt_sigframe __user *)(usp - >>> 4); >>> sigset_t set; > > Those changes ultimately make no difference to code size. > Although the entry point code is smaller the resulting > generated code for do_sigreturn() and do_rt_sigreturn() > is slightly larger. (At least that is the case with the gcc-5.3 > compiler I am currently using). > > Personally I don't like having to keep that ugly stack referencing > definition of regs based on sw either. I would like to remove that. > > "sw" isn't actually used in do_sigreturn() at all, so that could > be removed from the its parameters. I chose to keep it there in this > patch for consistency with do_rt_sigreturn().
OK. Thanks for the attention and explanation. > > Regards > Greg Best regards Philippe -- Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
