[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1621385544.nttlk5qugb.astroid@bobo.none>
Date: Wed, 19 May 2021 12:50:24 +1000
From: Nicholas Piggin <npiggin@...il.com>
To: "Dmitry V. Levin" <ldv@...linux.org>, Michael Ellerman
<mpe@...erman.id.au>
Cc: libc-dev@...ts.llvm.org, linux-api@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, musl@...ts.openwall.com, Matheus Castanho
<msc@...ux.ibm.com>, libc-alpha@...rceware.org
Subject: Re: Linux powerpc new system call instruction and ABI
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)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
