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

[Philippe De Muyter <phdm@macq.eu>]

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 <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
>>
>> 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