diff mbox series

[v2,8/8] powerpc/entry: fix ppc syscall entry issues for common entry

Message ID AD7BF1A62C0AD367+20241111031934.1579-16-luming.yu@shingroup.cn (mailing list archive)
State RFC
Headers show
Series [v2,1/8] powerpc/entry: convert to common and generic entry | expand

Commit Message

Luming Yu Nov. 11, 2024, 3:19 a.m. UTC 
From: Yu Luming <luming.yu@gmail.com>

Due to the common layer and internal calls details are hidden from
the top level at the call side in ppc arch code, there are some
difficulties in preserving
all semantics implications of the original code in the patch. e.g  when
we got -1 returned
from syscall_enter_from_user_mode, without touching common code, we have
to do
our own inference to recover the reasonable route to return, in order to
have correct errno
and syscall work behaviors,that are tested in seccomp_bpf 98 test cases.

Signed-off-by: Luming Yu <luming.yu@shingroup.cn>
---
 arch/powerpc/kernel/interrupt.c |  4 ++++
 arch/powerpc/kernel/syscall.c   | 28 +++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

Comments

Thomas Gleixner Nov. 13, 2024, 7:06 a.m. UTC | #1 
On Mon, Nov 11 2024 at 11:19, Luming Yu wrote:
> Due to the common layer and internal calls details are hidden from
> the top level at the call side in ppc arch code, there are some
> difficulties in preserving
> all semantics implications of the original code in the patch. e.g  when
> we got -1 returned
> from syscall_enter_from_user_mode, without touching common code, we have
> to do
> our own inference to recover the reasonable route to return, in order to
> have correct errno
> and syscall work behaviors,that are tested in seccomp_bpf 98 test
> cases.

This indicates that your conversion to the common code is broken to
begin with. Which is not surprising given the amount of issues I found
already.

You need to sit down an come up with a proper conversion design and not
just randomly replace things and then hack around the fallout later on.

If that requires changes to the core code, then they have to be designed
up front and implemented in a way which does not affect existing users.

Thanks,

        tglx
Luming Yu Nov. 13, 2024, 8:19 a.m. UTC | #2 
On Wed, Nov 13, 2024 at 08:06:04AM +0100, Thomas Gleixner wrote:
> On Mon, Nov 11 2024 at 11:19, Luming Yu wrote:
> > Due to the common layer and internal calls details are hidden from
> > the top level at the call side in ppc arch code, there are some
> > difficulties in preserving
> > all semantics implications of the original code in the patch. e.g  when
> > we got -1 returned
> > from syscall_enter_from_user_mode, without touching common code, we have
> > to do
> > our own inference to recover the reasonable route to return, in order to
> > have correct errno
> > and syscall work behaviors,that are tested in seccomp_bpf 98 test
> > cases.
> 
> This indicates that your conversion to the common code is broken to
> begin with. Which is not surprising given the amount of issues I found
> already.
> 
> You need to sit down an come up with a proper conversion design and not
> just randomly replace things and then hack around the fallout later on.
> 
> If that requires changes to the core code, then they have to be designed
> up front and implemented in a way which does not affect existing users.
> 
> Thanks,
> 
>         tglx
Thanks for your time and the review comments. It is helpful.
The 3rd ver of the patch set should be able to address all these issues.
Going through v0 to v2, I think I've truely understood how came ppc64 that is stuck
in the halfway to be able to enjoy least code duplication while having fully function on top
of common entry code for so many key features. 
when the v1 is out, it is already too late to call back and the v2 was reluctantly out with
a random fix instead of clean conversion as you concluded just for making the hack working
as it was.  : -( 
> 
> 
>
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index 2a5693b5f336..380697e35d3a 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -232,6 +232,7 @@  notrace unsigned long syscall_exit_prepare(unsigned long r3,
 {
 	unsigned long ti_flags;
 	unsigned long ret = 0;
+	unsigned long work = READ_ONCE(current_thread_info()->syscall_work);
 	bool is_not_scv = !IS_ENABLED(CONFIG_PPC_BOOK3S_64) || !scv;
 
 	CT_WARN_ON(ct_state() == CT_STATE_USER);
@@ -268,6 +269,9 @@  notrace unsigned long syscall_exit_prepare(unsigned long r3,
 
 	if (ti_flags & _TIF_SIGPENDING)
 		ret |= _TIF_RESTOREALL;
+
+	if (work)
+		ret |= _TIF_RESTOREALL;
 #ifdef CONFIG_PPC64
 	regs->exit_result = ret;
 #endif
diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
index dabe7f2b4bd4..358340f7fe75 100644
--- a/arch/powerpc/kernel/syscall.c
+++ b/arch/powerpc/kernel/syscall.c
@@ -18,6 +18,7 @@  notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
 {
 	long ret;
 	syscall_fn f;
+	unsigned long work = READ_ONCE(current_thread_info()->syscall_work);
 
 	kuap_lock();
 
@@ -119,7 +120,7 @@  notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
 
 	local_irq_enable();
 
-	if (unlikely(read_thread_flags() & _TIF_SYSCALL_DOTRACE)) {
+	if (work & SYSCALL_WORK_ENTER) {
 		if (unlikely(trap_is_unsupported_scv(regs))) {
 			/* Unsupported scv vector */
 			_exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
@@ -132,7 +133,32 @@  notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
 		 * and the test against NR_syscalls will fail and the return
 		 * value to be used is in regs->gpr[3].
 		 */
+		if (test_syscall_work(SECCOMP) &&
+				!test_syscall_work(SYSCALL_EMU))
+			regs->gpr[3] = -ENOSYS;
 		r0 = syscall_enter_from_user_mode(regs, r0);
+
+		if (test_syscall_work(SECCOMP)) {
+			if (r0 != -1)
+				regs->gpr[3] = regs->orig_gpr3;
+			else
+				goto skip;
+		}
+		if ((r0 == -1) && (test_syscall_work(SYSCALL_TRACE))) {
+			goto skip1;
+		}
+		if ((r0 == -1) && test_syscall_work(SYSCALL_EMU))
+			goto skip;
+		if (regs->gpr[0] >= NR_syscalls)
+			goto skip1;
+
+		r0 = regs->gpr[0];
+		if (r0 != -1)
+			goto skip;
+skip1:
+		r0 = -1;
+		regs->gpr[3] = -ENOSYS;
+skip:
 		if (unlikely(r0 >= NR_syscalls))
 			return regs->gpr[3];