[PATCH] powerpc/64: system call implement the bulk of the logic in C fix

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE2399d

4 messages, 2 authors, 2020-01-28 · open the first message on its own page

[PATCH] powerpc/64: system call implement the bulk of the logic in C fix

From: Nicholas Piggin <npiggin@gmail.com>
Date: 2020-01-27 14:20:29

This incremental patch fixes several soft-mask debug and unsafe
smp_processor_id messages due to tracing and false positives in
"unreconciled" code.

It also fixes a bug with syscall tracing functions that set registers
(e.g., PTRACE_SETREG) not setting GPRs properly.

There was a bug reported with the TM selftests, I haven't been able
to reproduce that one.

I can squash this into the main patch and resend the series if it
helps but the incremental helps to see the bug fixes.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/cputime.h | 39 +++++++++++++++++-------------
 arch/powerpc/kernel/syscall_64.c   | 26 ++++++++++++++------
 2 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h
index c43614cffaac..6639a6847cc0 100644
--- a/arch/powerpc/include/asm/cputime.h
+++ b/arch/powerpc/include/asm/cputime.h
@@ -44,6 +44,28 @@ static inline unsigned long cputime_to_usecs(const cputime_t ct)
 #ifdef CONFIG_PPC64
 #define get_accounting(tsk)	(&get_paca()->accounting)
 static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
+
+/*
+ * account_cpu_user_entry/exit runs "unreconciled", so can't trace,
+ * can't use use get_paca()
+ */
+static notrace inline void account_cpu_user_entry(void)
+{
+	unsigned long tb = mftb();
+	struct cpu_accounting_data *acct = &local_paca->accounting;
+
+	acct->utime += (tb - acct->starttime_user);
+	acct->starttime = tb;
+}
+static notrace inline void account_cpu_user_exit(void)
+{
+	unsigned long tb = mftb();
+	struct cpu_accounting_data *acct = &local_paca->accounting;
+
+	acct->stime += (tb - acct->starttime);
+	acct->starttime_user = tb;
+}
+
 #else
 #define get_accounting(tsk)	(&task_thread_info(tsk)->accounting)
 /*
@@ -60,23 +82,6 @@ static inline void arch_vtime_task_switch(struct task_struct *prev)
 }
 #endif
 
-static inline void account_cpu_user_entry(void)
-{
-	unsigned long tb = mftb();
-	struct cpu_accounting_data *acct = get_accounting(current);
-
-	acct->utime += (tb - acct->starttime_user);
-	acct->starttime = tb;
-}
-static inline void account_cpu_user_exit(void)
-{
-	unsigned long tb = mftb();
-	struct cpu_accounting_data *acct = get_accounting(current);
-
-	acct->stime += (tb - acct->starttime);
-	acct->starttime_user = tb;
-}
-
 #endif /* __KERNEL__ */
 #else /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
 static inline void account_cpu_user_entry(void)
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 529393a1ff1e..cfe458adde07 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -19,7 +19,8 @@ extern void __noreturn tabort_syscall(void);
 
 typedef long (*syscall_fn)(long, long, long, long, long, long);
 
-long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
+/* Has to run notrace because it is entered "unreconciled" */
+notrace long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 			   unsigned long r0, struct pt_regs *regs)
 {
 	unsigned long ti_flags;
@@ -36,7 +37,7 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 #ifdef CONFIG_PPC_SPLPAR
 	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) &&
 	    firmware_has_feature(FW_FEATURE_SPLPAR)) {
-		struct lppaca *lp = get_lppaca();
+		struct lppaca *lp = local_paca->lppaca_ptr;
 
 		if (unlikely(local_paca->dtl_ridx != be64_to_cpu(lp->dtl_idx)))
 			accumulate_stolen_time();
@@ -71,13 +72,22 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 		 * We use the return value of do_syscall_trace_enter() as the
 		 * syscall number. If the syscall was rejected for any reason
 		 * do_syscall_trace_enter() returns an invalid syscall number
-		 * and the test below against NR_syscalls will fail.
+		 * and the test against NR_syscalls will fail and the return
+		 * value to be used is in regs->gpr[3].
 		 */
 		r0 = do_syscall_trace_enter(regs);
-	}
-
-	if (unlikely(r0 >= NR_syscalls))
+		if (unlikely(r0 >= NR_syscalls))
+			return regs->gpr[3];
+		r3 = regs->gpr[3];
+		r4 = regs->gpr[4];
+		r5 = regs->gpr[5];
+		r6 = regs->gpr[6];
+		r7 = regs->gpr[7];
+		r8 = regs->gpr[8];
+
+	} else if (unlikely(r0 >= NR_syscalls)) {
 		return -ENOSYS;
+	}
 
 	/* May be faster to do array_index_nospec? */
 	barrier_nospec();
@@ -139,8 +149,10 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 		regs->gpr[3] = r3;
 	}
 
-	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE))
+	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
 		do_syscall_trace_leave(regs);
+		ret |= _TIF_RESTOREALL;
+	}
 
 again:
 	local_irq_disable();
-- 
2.23.0

Re: [PATCH] powerpc/64: system call implement the bulk of the logic in C fix

From: Michal Suchánek <hidden>
Date: 2020-01-27 18:18:30

On Tue, Jan 28, 2020 at 12:17:12AM +1000, Nicholas Piggin wrote:
This incremental patch fixes several soft-mask debug and unsafe
smp_processor_id messages due to tracing and false positives in
"unreconciled" code.

It also fixes a bug with syscall tracing functions that set registers
(e.g., PTRACE_SETREG) not setting GPRs properly.

There was a bug reported with the TM selftests, I haven't been able
to reproduce that one.

I can squash this into the main patch and resend the series if it
helps but the incremental helps to see the bug fixes.
There are some whitespace differences between this and the series I have
applied locally. What does it apply to?

Is there some revision of the patchset I missed?

Thanks

Michal
quoted hunk
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/cputime.h | 39 +++++++++++++++++-------------
 arch/powerpc/kernel/syscall_64.c   | 26 ++++++++++++++------
 2 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h
index c43614cffaac..6639a6847cc0 100644
--- a/arch/powerpc/include/asm/cputime.h
+++ b/arch/powerpc/include/asm/cputime.h
@@ -44,6 +44,28 @@ static inline unsigned long cputime_to_usecs(const cputime_t ct)
 #ifdef CONFIG_PPC64
 #define get_accounting(tsk)	(&get_paca()->accounting)
 static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
+
+/*
+ * account_cpu_user_entry/exit runs "unreconciled", so can't trace,
+ * can't use use get_paca()
+ */
+static notrace inline void account_cpu_user_entry(void)
+{
+	unsigned long tb = mftb();
+	struct cpu_accounting_data *acct = &local_paca->accounting;
+
+	acct->utime += (tb - acct->starttime_user);
+	acct->starttime = tb;
+}
+static notrace inline void account_cpu_user_exit(void)
+{
+	unsigned long tb = mftb();
+	struct cpu_accounting_data *acct = &local_paca->accounting;
+
+	acct->stime += (tb - acct->starttime);
+	acct->starttime_user = tb;
+}
+
 #else
 #define get_accounting(tsk)	(&task_thread_info(tsk)->accounting)
 /*
@@ -60,23 +82,6 @@ static inline void arch_vtime_task_switch(struct task_struct *prev)
 }
 #endif
 
-static inline void account_cpu_user_entry(void)
-{
-	unsigned long tb = mftb();
-	struct cpu_accounting_data *acct = get_accounting(current);
-
-	acct->utime += (tb - acct->starttime_user);
-	acct->starttime = tb;
-}
-static inline void account_cpu_user_exit(void)
-{
-	unsigned long tb = mftb();
-	struct cpu_accounting_data *acct = get_accounting(current);
-
-	acct->stime += (tb - acct->starttime);
-	acct->starttime_user = tb;
-}
-
 #endif /* __KERNEL__ */
 #else /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
 static inline void account_cpu_user_entry(void)
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 529393a1ff1e..cfe458adde07 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -19,7 +19,8 @@ extern void __noreturn tabort_syscall(void);
 
 typedef long (*syscall_fn)(long, long, long, long, long, long);
 
-long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
+/* Has to run notrace because it is entered "unreconciled" */
+notrace long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 			   unsigned long r0, struct pt_regs *regs)
 {
 	unsigned long ti_flags;
@@ -36,7 +37,7 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 #ifdef CONFIG_PPC_SPLPAR
 	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) &&
 	    firmware_has_feature(FW_FEATURE_SPLPAR)) {
-		struct lppaca *lp = get_lppaca();
+		struct lppaca *lp = local_paca->lppaca_ptr;
 
 		if (unlikely(local_paca->dtl_ridx != be64_to_cpu(lp->dtl_idx)))
 			accumulate_stolen_time();
@@ -71,13 +72,22 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 		 * We use the return value of do_syscall_trace_enter() as the
 		 * syscall number. If the syscall was rejected for any reason
 		 * do_syscall_trace_enter() returns an invalid syscall number
-		 * and the test below against NR_syscalls will fail.
+		 * and the test against NR_syscalls will fail and the return
+		 * value to be used is in regs->gpr[3].
 		 */
 		r0 = do_syscall_trace_enter(regs);
-	}
-
-	if (unlikely(r0 >= NR_syscalls))
+		if (unlikely(r0 >= NR_syscalls))
+			return regs->gpr[3];
+		r3 = regs->gpr[3];
+		r4 = regs->gpr[4];
+		r5 = regs->gpr[5];
+		r6 = regs->gpr[6];
+		r7 = regs->gpr[7];
+		r8 = regs->gpr[8];
+
+	} else if (unlikely(r0 >= NR_syscalls)) {
 		return -ENOSYS;
+	}
 
 	/* May be faster to do array_index_nospec? */
 	barrier_nospec();
@@ -139,8 +149,10 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 		regs->gpr[3] = r3;
 	}
 
-	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE))
+	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
 		do_syscall_trace_leave(regs);
+		ret |= _TIF_RESTOREALL;
+	}
 
 again:
 	local_irq_disable();
-- 
2.23.0

Re: [PATCH] powerpc/64: system call implement the bulk of the logic in C fix

From: Nicholas Piggin <npiggin@gmail.com>
Date: 2020-01-28 00:43:50

Michal Suchánek's on January 28, 2020 4:08 am:
On Tue, Jan 28, 2020 at 12:17:12AM +1000, Nicholas Piggin wrote:
quoted
This incremental patch fixes several soft-mask debug and unsafe
smp_processor_id messages due to tracing and false positives in
"unreconciled" code.

It also fixes a bug with syscall tracing functions that set registers
(e.g., PTRACE_SETREG) not setting GPRs properly.

There was a bug reported with the TM selftests, I haven't been able
to reproduce that one.

I can squash this into the main patch and resend the series if it
helps but the incremental helps to see the bug fixes.
There are some whitespace differences between this and the series I have
applied locally. What does it apply to?

Is there some revision of the patchset I missed?
No I may have just missed some of your whitespace cleanups, or maybe I got
some that Michael made which you don't have in his next-test branch.

Thanks,
Nick

Re: [PATCH] powerpc/64: system call implement the bulk of the logic in C fix

From: Michal Suchánek <hidden>
Date: 2020-01-28 09:43:49

On Tue, Jan 28, 2020 at 10:41:02AM +1000, Nicholas Piggin wrote:
Michal Suchánek's on January 28, 2020 4:08 am:
quoted
On Tue, Jan 28, 2020 at 12:17:12AM +1000, Nicholas Piggin wrote:
quoted
This incremental patch fixes several soft-mask debug and unsafe
smp_processor_id messages due to tracing and false positives in
"unreconciled" code.

It also fixes a bug with syscall tracing functions that set registers
(e.g., PTRACE_SETREG) not setting GPRs properly.

There was a bug reported with the TM selftests, I haven't been able
to reproduce that one.

I can squash this into the main patch and resend the series if it
helps but the incremental helps to see the bug fixes.
There are some whitespace differences between this and the series I have
applied locally. What does it apply to?

Is there some revision of the patchset I missed?
No I may have just missed some of your whitespace cleanups, or maybe I got
some that Michael made which you don't have in his next-test branch.
Looks like the latter. I will pick patches from next-test.

Thanks

Michal
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help