powerpc syscall_set_return_value() is confused (was Re: [PATCH v6 18/27] powerpc: define syscall_get_error())

Hi Dmitry,

"Dmitry V. Levin" [off-list ref] writes:

syscall_get_error() is required to be implemented on this
architecture in addition to already implemented syscall_get_nr(),
syscall_get_arguments(), syscall_get_return_value(), and
syscall_get_arch() functions in order to extend the generic
ptrace API with PTRACE_GET_SYSCALL_INFO request.

Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Elvira Khabirova <redacted>
Cc: Eugene Syromyatnikov <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Dmitry V. Levin <redacted>
---

Notes:
    v6: unchanged
    
    v5:
    This change has been tested with
    tools/testing/selftests/ptrace/get_syscall_info.c and strace,
    so it's correct from PTRACE_GET_SYSCALL_INFO point of view.
    
    This cast doubts on commit v4.3-rc1~86^2~81 that changed
    syscall_set_return_value() in a way that doesn't quite match
    syscall_get_error(), but syscall_set_return_value() is out
    of scope of this series, so I'll just let you know my concerns.

Sorry I only just saw this comment.

It's going to take me a while to page this stuff back into my brain, but
I think you may have a point.

I think the way it's written now *works* but only because it's only used
by seccomp, and we rely on the fact that the syscall exit path will
negate the value before returning to userspace or calling ptrace etc.

eg. we do:

syscall_set_return_value()
	if (error) {
		regs->ccr |= 0x10000000L;
		regs->gpr[3] = error;

then the asm does:

	/* Return code is already in r3 thanks to do_syscall_trace_enter() */
	b	.Lsyscall_exit
	...

.Lsyscall_exit:
	std	r3,RESULT(r1)
	...

3:	cmpld	r3,r11
	ld	r5,_CCR(r1)
	bge-	.Lsyscall_error
	...

.Lsyscall_error:
	oris	r5,r5,0x1000	/* Set SO bit in CR */
	neg	r3,r3
	std	r5,_CCR(r1)

And we do the same before calling do_syscall_trace_leave().


Still it's a bit confused, because in the C code we're setting r3 and
CCR in the C code, but we're not negating the value in r3, and we're not
setting result at all.

I'll test a patch to fix it up.

cheers