Re: Linux powerpc new system call instruction and ABI

Re: Linux powerpc new system call instruction and ABI

[Nicholas Piggin]

Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am:
> Hi,
> 
> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
> [...]
>> - Error handling: The consensus among kernel, glibc, and musl is to move to
>>   using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>>   which matches most other architectures, and is closer to a function call.
> 
> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was
> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and
> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used.
> This includes syscall_get_error() and all its users including
> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable
> when scv is used.
> 
> See also https://bugzilla.redhat.com/1929836

I see, thanks. Using latest strace from github.com, the attached kernel
patch makes strace -k check results a lot greener.

Some of the remaining failing tests look like this (I didn't look at all
of them yet):

signal(SIGUSR1, 0xfacefeeddeadbeef)     = 0 (SIG_DFL)
write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
) = 50
signal(SIGUSR1, SIG_IGN)                = 0xfacefeeddeadbeef
write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown errno 559038737) = 41
write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737
) = 26
exit_group(1)                           = ?

I think the problem is glibc testing for -ve, but it should be comparing
against -4095 (+cc Matheus)

  #define RET_SCV \
      cmpdi r3,0; \
      bgelr+; \
      neg r3,r3;

With this patch, I think the ptrace ABI should mostly be fixed. I think 
a problem remains with applications that look at system call return 
registers directly and have powerpc specific error cases. Those probably
will just need to be updated unfortunately. Michael thought it might be
possible to return an indication via ptrace somehow that the syscall is
using a new ABI, so such apps can be updated to test for it. I don't 
know how that would be done.

Thanks,
Nick

--
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 9c9ab2746168..b476a685f066 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -19,6 +19,7 @@
 #ifndef _ASM_POWERPC_PTRACE_H
 #define _ASM_POWERPC_PTRACE_H
 
+#include <linux/err.h>
 #include <uapi/asm/ptrace.h>
 #include <asm/asm-const.h>
 
@@ -152,25 +153,6 @@ extern unsigned long profile_pc(struct pt_regs *regs);
 long do_syscall_trace_enter(struct pt_regs *regs);
 void do_syscall_trace_leave(struct pt_regs *regs);
 
-#define kernel_stack_pointer(regs) ((regs)->gpr[1])
-static inline int is_syscall_success(struct pt_regs *regs)
-{
-	return !(regs->ccr & 0x10000000);
-}
-
-static inline long regs_return_value(struct pt_regs *regs)
-{
-	if (is_syscall_success(regs))
-		return regs->gpr[3];
-	else
-		return -regs->gpr[3];
-}
-
-static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
-{
-	regs->gpr[3] = rc;
-}
-
 #ifdef __powerpc64__
 #define user_mode(regs) ((((regs)->msr) >> MSR_PR_LG) & 0x1)
 #else
@@ -235,6 +217,31 @@ static __always_inline void set_trap_norestart(struct pt_regs *regs)
 	regs->trap |= 0x1;
 }
 
+#define kernel_stack_pointer(regs) ((regs)->gpr[1])
+static inline int is_syscall_success(struct pt_regs *regs)
+{
+	if (trap_is_scv(regs))
+		return !IS_ERR_VALUE((unsigned long)regs->gpr[3]);
+	else
+		return !(regs->ccr & 0x10000000);
+}
+
+static inline long regs_return_value(struct pt_regs *regs)
+{
+	if (trap_is_scv(regs))
+		return regs->gpr[3];
+
+	if (is_syscall_success(regs))
+		return regs->gpr[3];
+	else
+		return -regs->gpr[3];
+}
+
+static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
+{
+	regs->gpr[3] = rc;
+}
+
 #define arch_has_single_step()	(1)
 #define arch_has_block_step()	(true)
 #define ARCH_HAS_USER_SINGLE_STEP_REPORT
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index fd1b518eed17..e8b40149bf7e 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -41,11 +41,20 @@ static inline void syscall_rollback(struct task_struct *task,
 static inline long syscall_get_error(struct task_struct *task,
 				     struct pt_regs *regs)
 {
-	/*
-	 * If the system call failed,
-	 * regs->gpr[3] contains a positive ERRORCODE.
-	 */
-	return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
+	if (trap_is_scv(regs)) {
+		unsigned long error = regs->gpr[3];
+
+		if (task_is_32bit(task))
+			error = (long)(int)error;
+
+		return IS_ERR_VALUE(error) ? error : 0;
+	} else {
+		/*
+		 * If the system call failed,
+		 * regs->gpr[3] contains a positive ERRORCODE.
+		 */
+		return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
+	}
 }
 
 static inline long syscall_get_return_value(struct task_struct *task,
@@ -58,18 +67,22 @@ static inline void syscall_set_return_value(struct task_struct *task,
 					    struct pt_regs *regs,
 					    int error, long val)
 {
-	/*
-	 * In the general case it's not obvious that we must deal with CCR
-	 * here, as the syscall exit path will also do that for us. However
-	 * there are some places, eg. the signal code, which check ccr to
-	 * decide if the value in r3 is actually an error.
-	 */
-	if (error) {
-		regs->ccr |= 0x10000000L;
-		regs->gpr[3] = error;
+	if (trap_is_scv(regs)) {
+		regs->gpr[3] = (long) error ?: val;
 	} else {
-		regs->ccr &= ~0x10000000L;
-		regs->gpr[3] = val;
+		/*
+		 * In the general case it's not obvious that we must deal with
+		 * CCR here, as the syscall exit path will also do that for us.
+		 * However there are some places, eg. the signal code, which
+		 * check ccr to decide if the value in r3 is actually an error.
+		 */
+		if (error) {
+			regs->ccr |= 0x10000000L;
+			regs->gpr[3] = error;
+		} else {
+			regs->ccr &= ~0x10000000L;
+			regs->gpr[3] = val;
+		}
 	}
 }
 
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index b4ec6c7dd72e..c4bb9f305eaf 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -165,8 +165,10 @@ static inline bool test_thread_local_flags(unsigned int flags)
 
 #ifdef CONFIG_COMPAT
 #define is_32bit_task()	(test_thread_flag(TIF_32BIT))
+#define task_is_32bit(task) (test_tsk_thread_flag(task, TIF_32BIT))
 #else
 #define is_32bit_task()	(IS_ENABLED(CONFIG_PPC32))
+#define task_is_32bit(task) (IS_ENABLED(CONFIG_PPC32))
 #endif
 
 #if defined(CONFIG_PPC64)

Re: Linux powerpc new system call instruction and ABI

[Nicholas Piggin]

Excerpts from Nicholas Piggin's message of May 19, 2021 12:50 pm:
> Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am:
>> Hi,
>> 
>> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
>> [...]
>>> - Error handling: The consensus among kernel, glibc, and musl is to move to
>>>   using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>>>   which matches most other architectures, and is closer to a function call.
>> 
>> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was
>> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and
>> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used.
>> This includes syscall_get_error() and all its users including
>> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable
>> when scv is used.
>> 
>> See also https://bugzilla.redhat.com/1929836
> 
> I see, thanks. Using latest strace from github.com, the attached kernel
> patch makes strace -k check results a lot greener.
> 
> Some of the remaining failing tests look like this (I didn't look at all
> of them yet):
> 
> signal(SIGUSR1, 0xfacefeeddeadbeef)     = 0 (SIG_DFL)
> write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
> ) = 50
> signal(SIGUSR1, SIG_IGN)                = 0xfacefeeddeadbeef
> write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown errno 559038737) = 41
> write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737
> ) = 26
> exit_group(1)                           = ?
> 
> I think the problem is glibc testing for -ve, but it should be comparing
> against -4095 (+cc Matheus)
> 
>   #define RET_SCV \
>       cmpdi r3,0; \
>       bgelr+; \
>       neg r3,r3;

This glibc patch at least gets that signal test working. Haven't run the 
full suite yet because of trouble making it work with a local glibc
install...

Thanks,
Nick

---

diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
index c57bb1c05d..1ea4c3b917 100644
--- a/sysdeps/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/powerpc/powerpc64/sysdep.h
@@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \
 #endif
 
 #define RET_SCV \
-    cmpdi r3,0; \
-    bgelr+; \
+    li r9,-4095; \
+    cmpld r3,r9; \
+    bltlr+; \
     neg r3,r3;
 
 #define RET_SC \

Re: Linux powerpc new system call instruction and ABI

[Dmitry V. Levin]

On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
[...]
> With this patch, I think the ptrace ABI should mostly be fixed. I think 
> a problem remains with applications that look at system call return 
> registers directly and have powerpc specific error cases. Those probably
> will just need to be updated unfortunately. Michael thought it might be
> possible to return an indication via ptrace somehow that the syscall is
> using a new ABI, so such apps can be updated to test for it. I don't 
> know how that would be done.

Is there any sane way for these applications to handle the scv case?
How can they tell that the scv semantics is being used for the given
syscall invocation?  Can this information be obtained e.g. from struct
pt_regs?

For example, in strace we have the following powerpc-specific code used
for syscall tampering:

$ cat src/linux/powerpc/set_error.c
/*
 * Copyright (c) 2016-2021 The strace developers.
 * All rights reserved.
 *
 * SPDX-License-Identifier: LGPL-2.1-or-later
 */

static int
arch_set_r3_ccr(struct tcb *tcp, const unsigned long r3,
		const unsigned long ccr_set, const unsigned long ccr_clear)
{
	if (ptrace_syscall_info_is_valid() &&
	    upeek(tcp, sizeof(long) * PT_CCR, &ppc_regs.ccr))
                return -1;
	const unsigned long old_ccr = ppc_regs.ccr;
	ppc_regs.gpr[3] = r3;
	ppc_regs.ccr |= ccr_set;
	ppc_regs.ccr &= ~ccr_clear;
	if (ppc_regs.ccr != old_ccr &&
	    upoke(tcp, sizeof(long) * PT_CCR, ppc_regs.ccr))
		return -1;
	return upoke(tcp, sizeof(long) * (PT_R0 + 3), ppc_regs.gpr[3]);
}

static int
arch_set_error(struct tcb *tcp)
{
	return arch_set_r3_ccr(tcp, tcp->u_error, 0x10000000, 0);
}

static int
arch_set_success(struct tcb *tcp)
{
	return arch_set_r3_ccr(tcp, tcp->u_rval, 0, 0x10000000);
}


-- 
ldv

Re: Linux powerpc new system call instruction and ABI

[Nicholas Piggin]

Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm:
> On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
> [...]
>> With this patch, I think the ptrace ABI should mostly be fixed. I think 
>> a problem remains with applications that look at system call return 
>> registers directly and have powerpc specific error cases. Those probably
>> will just need to be updated unfortunately. Michael thought it might be
>> possible to return an indication via ptrace somehow that the syscall is
>> using a new ABI, so such apps can be updated to test for it. I don't 
>> know how that would be done.
> 
> Is there any sane way for these applications to handle the scv case?
> How can they tell that the scv semantics is being used for the given
> syscall invocation?  Can this information be obtained e.g. from struct
> pt_regs?

Not that I know of. Michael suggested there might be a way to add 
something. ptrace_syscall_info has some pad bytes, could
we use one for flags bits and set a bit for "new system call ABI"?

As a more hacky thing you could make a syscall with -1 and see how
the error looks, and then assume all syscalls will be the same.

Thanks,
Nick

> 
> For example, in strace we have the following powerpc-specific code used
> for syscall tampering:
> 
> $ cat src/linux/powerpc/set_error.c
> /*
>  * Copyright (c) 2016-2021 The strace developers.
>  * All rights reserved.
>  *
>  * SPDX-License-Identifier: LGPL-2.1-or-later
>  */
> 
> static int
> arch_set_r3_ccr(struct tcb *tcp, const unsigned long r3,
> 		const unsigned long ccr_set, const unsigned long ccr_clear)
> {
> 	if (ptrace_syscall_info_is_valid() &&
> 	    upeek(tcp, sizeof(long) * PT_CCR, &ppc_regs.ccr))
>                 return -1;
> 	const unsigned long old_ccr = ppc_regs.ccr;
> 	ppc_regs.gpr[3] = r3;
> 	ppc_regs.ccr |= ccr_set;
> 	ppc_regs.ccr &= ~ccr_clear;
> 	if (ppc_regs.ccr != old_ccr &&
> 	    upoke(tcp, sizeof(long) * PT_CCR, ppc_regs.ccr))
> 		return -1;
> 	return upoke(tcp, sizeof(long) * (PT_R0 + 3), ppc_regs.gpr[3]);
> }
> 
> static int
> arch_set_error(struct tcb *tcp)
> {
> 	return arch_set_r3_ccr(tcp, tcp->u_error, 0x10000000, 0);
> }
> 
> static int
> arch_set_success(struct tcb *tcp)
> {
> 	return arch_set_r3_ccr(tcp, tcp->u_rval, 0, 0x10000000);
> }
> 
> 
> -- 
> ldv
> 

Re: Linux powerpc new system call instruction and ABI

[Tulio Magno Quites Machado Filho]

Nicholas Piggin via Libc-alpha <libc-alpha@sourceware.org> writes:

> As a more hacky thing you could make a syscall with -1 and see how
> the error looks, and then assume all syscalls will be the same.

I'm not sure this would work.
Even in glibc, it's expected that early syscalls will use sc while scv is used
later in the execution.

-- 
Tulio Magno

Re: Linux powerpc new system call instruction and ABI

[Dmitry V. Levin]

On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote:
> Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm:
> > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
> > [...]
> >> With this patch, I think the ptrace ABI should mostly be fixed. I think 
> >> a problem remains with applications that look at system call return 
> >> registers directly and have powerpc specific error cases. Those probably
> >> will just need to be updated unfortunately. Michael thought it might be
> >> possible to return an indication via ptrace somehow that the syscall is
> >> using a new ABI, so such apps can be updated to test for it. I don't 
> >> know how that would be done.
> > 
> > Is there any sane way for these applications to handle the scv case?
> > How can they tell that the scv semantics is being used for the given
> > syscall invocation?  Can this information be obtained e.g. from struct
> > pt_regs?
> 
> Not that I know of. Michael suggested there might be a way to add 
> something. ptrace_syscall_info has some pad bytes, could
> we use one for flags bits and set a bit for "new system call ABI"?

PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all
architecture-specific details behind struct ptrace_syscall_info which has
the same meaning on all architectures.  ptrace_syscall_info.exit contains
both rval and is_error fields to support every architecture regardless of
its syscall ABI.

ptrace_syscall_info.exit is extensible, but every architecture would have
to define a method of telling whether the system call follows the "new
system call ABI" conventions to export this bit of information.

This essentially means implementing something like
static inline long syscall_get_error_abi(struct task_struct *task, struct pt_regs *regs)
for every architecture, and using it along with syscall_get_error
in ptrace_get_syscall_info_exit to initialize the new field in
ptrace_syscall_info.exit structure.

> As a more hacky thing you could make a syscall with -1 and see how
> the error looks, and then assume all syscalls will be the same.

This would be very unreliable because sc and scv are allowed to intermingle,
so every syscall invocation can follow any of these two error handling
conventions.

> Or... is it possible at syscall entry to peek the address of
> the instruction which caused the call and see if that was a
> scv instruction? That would be about as reliable as possible
> without having that new flag bit.

No other architecture requires peeking into tracee memory just to find out
the syscall ABI.  This would make powerpc the most ugly architecture for
ptracing.

I wonder why can't this information be just exported to the tracer via
struct pt_regs?


-- 
ldv

Re: Linux powerpc new system call instruction and ABI

[Nicholas Piggin]

Excerpts from Dmitry V. Levin's message of May 19, 2021 11:26 pm:
> On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote:
>> Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm:
>> > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
>> > [...]
>> >> With this patch, I think the ptrace ABI should mostly be fixed. I think 
>> >> a problem remains with applications that look at system call return 
>> >> registers directly and have powerpc specific error cases. Those probably
>> >> will just need to be updated unfortunately. Michael thought it might be
>> >> possible to return an indication via ptrace somehow that the syscall is
>> >> using a new ABI, so such apps can be updated to test for it. I don't 
>> >> know how that would be done.
>> > 
>> > Is there any sane way for these applications to handle the scv case?
>> > How can they tell that the scv semantics is being used for the given
>> > syscall invocation?  Can this information be obtained e.g. from struct
>> > pt_regs?
>> 
>> Not that I know of. Michael suggested there might be a way to add 
>> something. ptrace_syscall_info has some pad bytes, could
>> we use one for flags bits and set a bit for "new system call ABI"?
> 
> PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all
> architecture-specific details behind struct ptrace_syscall_info which has
> the same meaning on all architectures.  ptrace_syscall_info.exit contains
> both rval and is_error fields to support every architecture regardless of
> its syscall ABI.
> 
> ptrace_syscall_info.exit is extensible, but every architecture would have
> to define a method of telling whether the system call follows the "new
> system call ABI" conventions to export this bit of information.

It's already architecture speicfic if you look at registers of syscall 
exit state so I don't see a problem with a flag that ppc can use for
ABI.

> 
> This essentially means implementing something like
> static inline long syscall_get_error_abi(struct task_struct *task, struct pt_regs *regs)
> for every architecture, and using it along with syscall_get_error
> in ptrace_get_syscall_info_exit to initialize the new field in
> ptrace_syscall_info.exit structure.

Yes this could work. Other architectures can just use a generic
implementation if they don't define their own so that's easy. And
in userspace they can continue to ignore the flag.

> 
>> As a more hacky thing you could make a syscall with -1 and see how
>> the error looks, and then assume all syscalls will be the same.
> 
> This would be very unreliable because sc and scv are allowed to intermingle,
> so every syscall invocation can follow any of these two error handling
> conventions.
> 
>> Or... is it possible at syscall entry to peek the address of
>> the instruction which caused the call and see if that was a
>> scv instruction? That would be about as reliable as possible
>> without having that new flag bit.
> 
> No other architecture requires peeking into tracee memory just to find out
> the syscall ABI.  This would make powerpc the most ugly architecture for
> ptracing.
> 
> I wonder why can't this information be just exported to the tracer via
> struct pt_regs?

It might be able to, I don't see why that would be superior though.

Where could you put it... I guess it could go in the trap field in a 
high bit. But could that break things that just test for syscall 
trap number (and don't care about register ABI)? I'm not sure.

Thanks,
Nick

Re: Linux powerpc new system call instruction and ABI

[Dmitry V. Levin]

On Thu, May 20, 2021 at 08:51:53AM +1000, Nicholas Piggin wrote:
> Excerpts from Dmitry V. Levin's message of May 19, 2021 11:26 pm:
> > On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote:
> >> Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm:
> >> > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
> >> > [...]
> >> >> With this patch, I think the ptrace ABI should mostly be fixed. I think 
> >> >> a problem remains with applications that look at system call return 
> >> >> registers directly and have powerpc specific error cases. Those probably
> >> >> will just need to be updated unfortunately. Michael thought it might be
> >> >> possible to return an indication via ptrace somehow that the syscall is
> >> >> using a new ABI, so such apps can be updated to test for it. I don't 
> >> >> know how that would be done.
> >> > 
> >> > Is there any sane way for these applications to handle the scv case?
> >> > How can they tell that the scv semantics is being used for the given
> >> > syscall invocation?  Can this information be obtained e.g. from struct
> >> > pt_regs?
> >> 
> >> Not that I know of. Michael suggested there might be a way to add 
> >> something. ptrace_syscall_info has some pad bytes, could
> >> we use one for flags bits and set a bit for "new system call ABI"?
> > 
> > PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all
> > architecture-specific details behind struct ptrace_syscall_info which has
> > the same meaning on all architectures.  ptrace_syscall_info.exit contains
> > both rval and is_error fields to support every architecture regardless of
> > its syscall ABI.
> > 
> > ptrace_syscall_info.exit is extensible, but every architecture would have
> > to define a method of telling whether the system call follows the "new
> > system call ABI" conventions to export this bit of information.
> 
> It's already architecture speicfic if you look at registers of syscall 
> exit state so I don't see a problem with a flag that ppc can use for
> ABI.

To be honest, I don't see anything architecture-specific in
PTRACE_GET_SYSCALL_INFO API.  Yes, it's implementation uses various
functions defined in asm/syscall.h, but this doesn't make the interface
architecture-specific.

PTRACE_GET_SYSCALL_INFO saves its users from necessity to be aware of
tracee registers.  That's why the only place where strace has to deal
with tracee registers nowadays is syscall tampering.  The most reliable
solution is to introduce PTRACE_SET_SYSCALL_INFO, this would make the
whole syscall abi issue irrelevant for ptracers, maybe the time has come
to implement it.

Unfortunately, extending ptrace API takes time, and it's not going to be
backported to older kernels anyway, but scv-enabled kernels are already
in the wild, so we need a quick powerpc-specific fix that would be
backported to all maintained scv-enabled kernels.

[...]
> > I wonder why can't this information be just exported to the tracer via
> > struct pt_regs?
> 
> It might be able to, I don't see why that would be superior though.
> 
> Where could you put it... I guess it could go in the trap field in a 
> high bit. But could that break things that just test for syscall 
> trap number (and don't care about register ABI)? I'm not sure.

Looks like struct pt_regs.trap already contains the information that could
be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then
it's scv.  Is my reading of arch/powerpc/include/asm/ptrace.h correct?


-- 
ldv

Re: Linux powerpc new system call instruction and ABI

[Nicholas Piggin]

Excerpts from Dmitry V. Levin's message of May 20, 2021 9:27 am:
> On Thu, May 20, 2021 at 08:51:53AM +1000, Nicholas Piggin wrote:
>> Excerpts from Dmitry V. Levin's message of May 19, 2021 11:26 pm:
>> > On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote:
>> >> Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm:
>> >> > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
>> >> > [...]
>> >> >> With this patch, I think the ptrace ABI should mostly be fixed. I think 
>> >> >> a problem remains with applications that look at system call return 
>> >> >> registers directly and have powerpc specific error cases. Those probably
>> >> >> will just need to be updated unfortunately. Michael thought it might be
>> >> >> possible to return an indication via ptrace somehow that the syscall is
>> >> >> using a new ABI, so such apps can be updated to test for it. I don't 
>> >> >> know how that would be done.
>> >> > 
>> >> > Is there any sane way for these applications to handle the scv case?
>> >> > How can they tell that the scv semantics is being used for the given
>> >> > syscall invocation?  Can this information be obtained e.g. from struct
>> >> > pt_regs?
>> >> 
>> >> Not that I know of. Michael suggested there might be a way to add 
>> >> something. ptrace_syscall_info has some pad bytes, could
>> >> we use one for flags bits and set a bit for "new system call ABI"?
>> > 
>> > PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all
>> > architecture-specific details behind struct ptrace_syscall_info which has
>> > the same meaning on all architectures.  ptrace_syscall_info.exit contains
>> > both rval and is_error fields to support every architecture regardless of
>> > its syscall ABI.
>> > 
>> > ptrace_syscall_info.exit is extensible, but every architecture would have
>> > to define a method of telling whether the system call follows the "new
>> > system call ABI" conventions to export this bit of information.
>> 
>> It's already architecture speicfic if you look at registers of syscall 
>> exit state so I don't see a problem with a flag that ppc can use for
>> ABI.
> 
> To be honest, I don't see anything architecture-specific in
> PTRACE_GET_SYSCALL_INFO API.  Yes, it's implementation uses various
> functions defined in asm/syscall.h, but this doesn't make the interface
> architecture-specific.

No. But a field or flag it exports could be architecture dependent.
It doesn't detract independence from the rest of the ABI. That said...

> PTRACE_GET_SYSCALL_INFO saves its users from necessity to be aware of
> tracee registers.  That's why the only place where strace has to deal
> with tracee registers nowadays is syscall tampering.  The most reliable
> solution is to introduce PTRACE_SET_SYSCALL_INFO, this would make the
> whole syscall abi issue irrelevant for ptracers, maybe the time has come
> to implement it.
> 
> Unfortunately, extending ptrace API takes time, and it's not going to be
> backported to older kernels anyway, but scv-enabled kernels are already
> in the wild, so we need a quick powerpc-specific fix that would be
> backported to all maintained scv-enabled kernels.
> 
> [...]
>> > I wonder why can't this information be just exported to the tracer via
>> > struct pt_regs?
>> 
>> It might be able to, I don't see why that would be superior though.
>> 
>> Where could you put it... I guess it could go in the trap field in a 
>> high bit. But could that break things that just test for syscall 
>> trap number (and don't care about register ABI)? I'm not sure.
> 
> Looks like struct pt_regs.trap already contains the information that could
> be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then
> it's scv.  Is my reading of arch/powerpc/include/asm/ptrace.h correct?

Hmm, I think it is. Certainly in the kernel regs struct it is, I had in 
my mind that we put it to 0xc00 when populating the user struct for
compatibility, but it seems not. So I guess this would work.

Thanks,
Nick

Re: Linux powerpc new system call instruction and ABI

[Dmitry V. Levin]

On Thu, May 20, 2021 at 12:40:36PM +1000, Nicholas Piggin wrote:
[...]
> > Looks like struct pt_regs.trap already contains the information that could
> > be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then
> > it's scv.  Is my reading of arch/powerpc/include/asm/ptrace.h correct?
> 
> Hmm, I think it is. Certainly in the kernel regs struct it is, I had in 
> my mind that we put it to 0xc00 when populating the user struct for
> compatibility, but it seems not. So I guess this would work.

OK, can we state that (pt_regs.trap & ~0xf) == 0x3000 is a part of the scv
ABI, so it's not going to change and could be relied upon by userspace?
Could this be documented in Documentation/powerpc/syscall64-abi.rst,
please?


-- 
ldv

Re: Linux powerpc new system call instruction and ABI

[Nicholas Piggin]

Excerpts from Dmitry V. Levin's message of May 20, 2021 1:06 pm:
> On Thu, May 20, 2021 at 12:40:36PM +1000, Nicholas Piggin wrote:
> [...]
>> > Looks like struct pt_regs.trap already contains the information that could
>> > be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then
>> > it's scv.  Is my reading of arch/powerpc/include/asm/ptrace.h correct?
>> 
>> Hmm, I think it is. Certainly in the kernel regs struct it is, I had in 
>> my mind that we put it to 0xc00 when populating the user struct for
>> compatibility, but it seems not. So I guess this would work.
> 
> OK, can we state that (pt_regs.trap & ~0xf) == 0x3000 is a part of the scv
> ABI, so it's not going to change and could be relied upon by userspace?
> Could this be documented in Documentation/powerpc/syscall64-abi.rst,
> please?

Yeah I think we can do that. The kernel doesn't care what is put in the
userspace pt_regs.trap too much so if this is your preferred approach
then I will document it in the ABI.

Thanks,
Nick

Re: Linux powerpc new system call instruction and ABI

[Matheus Castanho]

Nicholas Piggin <npiggin@gmail.com> writes:

> Excerpts from Nicholas Piggin's message of May 19, 2021 12:50 pm:
>> Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am:
>>> Hi,
>>>
>>> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
>>> [...]
>>>> - Error handling: The consensus among kernel, glibc, and musl is to move to
>>>>   using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>>>>   which matches most other architectures, and is closer to a function call.
>>>
>>> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was
>>> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and
>>> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used.
>>> This includes syscall_get_error() and all its users including
>>> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable
>>> when scv is used.
>>>
>>> See also https://bugzilla.redhat.com/1929836
>>
>> I see, thanks. Using latest strace from github.com, the attached kernel
>> patch makes strace -k check results a lot greener.
>>
>> Some of the remaining failing tests look like this (I didn't look at all
>> of them yet):
>>
>> signal(SIGUSR1, 0xfacefeeddeadbeef)     = 0 (SIG_DFL)
>> write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
>> ) = 50
>> signal(SIGUSR1, SIG_IGN)                = 0xfacefeeddeadbeef
>> write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown errno 559038737) = 41
>> write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737
>> ) = 26
>> exit_group(1)                           = ?
>>
>> I think the problem is glibc testing for -ve, but it should be comparing
>> against -4095 (+cc Matheus)
>>
>>   #define RET_SCV \
>>       cmpdi r3,0; \
>>       bgelr+; \
>>       neg r3,r3;
>
> This glibc patch at least gets that signal test working. Haven't run the
> full suite yet because of trouble making it work with a local glibc
> install...
>
> Thanks,
> Nick
>
> ---
>
> diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
> index c57bb1c05d..1ea4c3b917 100644
> --- a/sysdeps/powerpc/powerpc64/sysdep.h
> +++ b/sysdeps/powerpc/powerpc64/sysdep.h
> @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \
>  #endif
>
>  #define RET_SCV \
> -    cmpdi r3,0; \
> -    bgelr+; \
> +    li r9,-4095; \
> +    cmpld r3,r9; \
> +    bltlr+; \
>      neg r3,r3;
>
>  #define RET_SC \

Hi Nick,

I agree the current code is accepting more values as errors than it
should. This change looks good to me. All glibc tests are passing with
it. I also built strace and checked one of the failing tests against a
glibc with your patch:

~/src/strace/tests$ uname -r
5.10.16-1-default

~/src/strace/tests$ /lib64/libc.so.6
GNU C Library (GNU libc) release release version 2.33 (git 9826b03b74).
[...]

~/src/strace/tests$ ./signal.gen.test
errno2name.c:461: unknown errno 559038737: Unknown error 559038737
signal.gen.test: failed test: ../signal failed with code 1

~/src/strace/tests$ ./signal
signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL)
signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN)
signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
errno2name.c:461: unknown errno 559038737: Unknown error 559038737


Running with glibc containing the patch:

~/src/strace/tests$ ~/build/glibc/testrun.sh ./signal
signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL)
signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN)
signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef
signal(-559038737, SIG_IGN) = -1 EINVAL (Invalid argument)
+++ exited with 0 +++



If the patch below looks OK to you and no one objects, I'll commit it to
glibc on Monday.

Thanks,
Matheus Castanho

---

From: Nicholas Piggin <npiggin@gmail.com>
Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes

When using scv on templated ASM syscalls, current code interprets any
negative return value as error, but the only valid error codes are in
the range -4095..-1 according to the ABI.

Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
---
 sysdeps/powerpc/powerpc64/sysdep.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
index c57bb1c05d..1ea4c3b917 100644
--- a/sysdeps/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/powerpc/powerpc64/sysdep.h
@@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \
 #endif

 #define RET_SCV \
-    cmpdi r3,0; \
-    bgelr+; \
+    li r9,-4095; \
+    cmpld r3,r9; \
+    bltlr+; \
     neg r3,r3;

 #define RET_SC \
--
2.31.1

Re: Linux powerpc new system call instruction and ABI

[Florian Weimer]

* Matheus Castanho via Libc-alpha:

> From: Nicholas Piggin <npiggin@gmail.com>
> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes
>
> When using scv on templated ASM syscalls, current code interprets any
> negative return value as error, but the only valid error codes are in
> the range -4095..-1 according to the ABI.
>
> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>

Please reference bug 27892 in the commit message.  I'd also appreciate a
backport to the 2.33 release branch (where you need to add NEWS manually
to add the bug reference).

Thanks,
Florian

Re: Linux powerpc new system call instruction and ABI

[Matheus Castanho]

Florian Weimer <fweimer@redhat.com> writes:

> * Matheus Castanho via Libc-alpha:
>
>> From: Nicholas Piggin <npiggin@gmail.com>
>> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes
>>
>> When using scv on templated ASM syscalls, current code interprets any
>> negative return value as error, but the only valid error codes are in
>> the range -4095..-1 according to the ABI.
>>
>> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
>
> Please reference bug 27892 in the commit message.  I'd also appreciate a
> backport to the 2.33 release branch (where you need to add NEWS manually
> to add the bug reference).

No problem. [BZ #27892] appended to the commit title. I'll make sure to
backport to 2.33 as well.

Thanks

--
Matheus Castanho

Re: Linux powerpc new system call instruction and ABI

[Dmitry V. Levin]

On Fri, May 21, 2021 at 05:00:36PM -0300, Matheus Castanho wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> > * Matheus Castanho via Libc-alpha:
> >> From: Nicholas Piggin <npiggin@gmail.com>
> >> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes
> >>
> >> When using scv on templated ASM syscalls, current code interprets any
> >> negative return value as error, but the only valid error codes are in
> >> the range -4095..-1 according to the ABI.
> >>
> >> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
> >
> > Please reference bug 27892 in the commit message.  I'd also appreciate a
> > backport to the 2.33 release branch (where you need to add NEWS manually
> > to add the bug reference).
> 
> No problem. [BZ #27892] appended to the commit title. I'll make sure to
> backport to 2.33 as well.

Could you also mention in the commit message that the change fixes
'signal.gen.test' strace test where it was observed initially?


-- 
ldv

Re: Linux powerpc new system call instruction and ABI

[Matheus Castanho]

Dmitry V. Levin <ldv@altlinux.org> writes:

> On Fri, May 21, 2021 at 05:00:36PM -0300, Matheus Castanho wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>> > * Matheus Castanho via Libc-alpha:
>> >> From: Nicholas Piggin <npiggin@gmail.com>
>> >> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes
>> >>
>> >> When using scv on templated ASM syscalls, current code interprets any
>> >> negative return value as error, but the only valid error codes are in
>> >> the range -4095..-1 according to the ABI.
>> >>
>> >> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
>> >
>> > Please reference bug 27892 in the commit message.  I'd also appreciate a
>> > backport to the 2.33 release branch (where you need to add NEWS manually
>> > to add the bug reference).
>>
>> No problem. [BZ #27892] appended to the commit title. I'll make sure to
>> backport to 2.33 as well.
>
> Could you also mention in the commit message that the change fixes
> 'signal.gen.test' strace test where it was observed initially?

Sure, no problem. I'll commit it later today.

--
Matheus Castanho

Re: Linux powerpc new system call instruction and ABI

[Matheus Castanho]

Matheus Castanho <msc@linux.ibm.com> writes:

> Dmitry V. Levin <ldv@altlinux.org> writes:
>
>> On Fri, May 21, 2021 at 05:00:36PM -0300, Matheus Castanho wrote:
>>> Florian Weimer <fweimer@redhat.com> writes:
>>> > * Matheus Castanho via Libc-alpha:
>>> >> From: Nicholas Piggin <npiggin@gmail.com>
>>> >> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes
>>> >>
>>> >> When using scv on templated ASM syscalls, current code interprets any
>>> >> negative return value as error, but the only valid error codes are in
>>> >> the range -4095..-1 according to the ABI.
>>> >>
>>> >> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
>>> >
>>> > Please reference bug 27892 in the commit message.  I'd also appreciate a
>>> > backport to the 2.33 release branch (where you need to add NEWS manually
>>> > to add the bug reference).
>>>
>>> No problem. [BZ #27892] appended to the commit title. I'll make sure to
>>> backport to 2.33 as well.
>>
>> Could you also mention in the commit message that the change fixes
>> 'signal.gen.test' strace test where it was observed initially?
>
> Sure, no problem. I'll commit it later today.

Since the patch falls into the less-than-15-LOC category and this is
Nick's first contribution to glibc, looks like he doesn't need a
copyright assignment.

Pushed to master as 7de36744ee1325f35d3fe0ca079dd33c40b12267

Backported to 2.33 via commit 0ef0e6de7fdfa18328b09ba2afb4f0112d4bdab4

Thanks,
Matheus Castanho