Re: [PATCH] parisc: Fix syscall restarts (v2)

[Helge Deller <deller@gmx.de>]

On 21.12.2015 21:27, Mathieu Desnoyers wrote:
> ----- On Dec 21, 2015, at 4:19 AM, Helge Deller deller@gmx.de wrote:
> 
>> This is version 2 of the patch:
>>
>> On parisc syscalls which are interrupted by signals sometimes failed to
>> restart and instead returned -ENOSYS which in the worst case lead to
>> userspace crashes.
>> A similiar problem existed on MIPS and was fixed by commit e967ef02
>> ("MIPS: Fix restart of indirect syscalls").
>>
>> On parisc the current syscall restart code assumes that all syscall
>> callers load the syscall number in the delay slot of the ble
>> instruction. That's how it is e.g. done in the unistd.h header file:
>> 	ble 0x100(%sr2, %r0)
>> 	ldi #syscall_nr, %r20
>> Because of that assumption the current code never restored %r20 before
>> returning to userspace.
>>
>> This assumption is at least not true for code which uses the glibc
>> syscall() function, which instead uses this syntax:
>> 	ble 0x100(%sr2, %r0)
>> 	copy regX, %r20
>> where regX depend on how the compiler optimizes the code and register
>> usage.
>>
>> This patch fixes this problem by adding code to analyze how the syscall
>> number is loaded in the delay branch and - if needed - copy the syscall
>> number to regX prior returning to userspace for the syscall restart.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org
>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>
>> diff --git a/arch/parisc/kernel/signal.c b/arch/parisc/kernel/signal.c
>> index dc1ea79..2264f68 100644
>> --- a/arch/parisc/kernel/signal.c
>> +++ b/arch/parisc/kernel/signal.c
>> @@ -435,6 +435,55 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs,
>> int in_syscall)
>> 		regs->gr[28]);
>> }
>>
>> +/*
>> + * Check how the syscall number gets loaded into %r20 within
>> + * the delay branch in userspace and adjust as needed.
>> + */
>> +
>> +static void check_syscallno_in_delay_branch(struct pt_regs *regs)
>> +{
>> +	u32 opcode, source_reg;
>> +	u32 __user *uaddr;
>> +	int err;
>> +
>> +	/* Usually we don't have to restore %r20 (the system call number)
>> +	 * because it gets loaded in the delay slot of the branch external
>> +	 * instruction via the ldi instruction.
>> +	 * In some cases a register-to-register copy instruction might have
>> +	 * been used instead, in which case we need to copy the syscall
>> +	 * number into the source register before returning to userspace.
>> +	 */
>> +
>> +	/* A syscall is just a branch, so all we have to do is fiddle the
>> +	 * return pointer so that the ble instruction gets executed again.
>> +	 */
>> +	regs->gr[31] -= 8; /* delayed branching */
>> +
>> +	/* Get assembler opcode of code in delay branch */
>> +	uaddr = (unsigned int *) ((regs->gr[31] & ~3) + 4);
> 
> Is it valid to have unaligned instructions ? Does the architecture
> allow it, or it's a fumble and we should pr_warn ?

How can it be unaligned? It's about u32...
And, no, unaligned instructions are not allowed (I think that at least).
 
>> +	err = get_user(opcode, uaddr);
>> +	if (err)
> 
> Should we add a pr_warn here ?

No. There is no gain to have a warning here.
 
>> +		return;
>> +
>> +	/* Check if delay branch uses "ldi int,%r20" */
>> +	if ((opcode & 0xffff0000) == 0x34140000)
>> +		return;	/* everything ok, just return */
>> +
>> +	/* Check if delay branch uses "nop" */
>> +	if (opcode == INSN_NOP)
>> +		return;
> 
> When we find a NOP in the delay slot, how can we be sure %r20
> still holds the syscall value when we re-play the branch
> instruction ? 

I looked at the code and even tested it (with your testcase actually).

> Can it be overwritten during the syscall,
> either from start of syscall to here, or from here to
> return to userspace ?

No.
 
>> +
>> +	/* Check if delay branch uses "copy %rX,%r20" */
>> +	if ((opcode & 0xffe0ffff) == 0x08000254) {
>> +		source_reg = (opcode >> 16) & 31;
>> +		regs->gr[source_reg] = regs->gr[20];
> 
> Similar question here, how can we be sure regs->gr[20]
> still has the system call number at this point (not
> overwritten from start of syscall to here) ?

Those registers are saved at entry of syscall and
restored at exit (with exception of a few registers
e.g. like r28 which is the return value of the syscall). 

Helge