diff mbox series

riscv: Remove unnecessary path for syscall_trace

Message ID 1590416306-66453-1-git-send-email-guoren@kernel.org (mailing list archive)
State New, archived
Headers show
Series riscv: Remove unnecessary path for syscall_trace | expand

Commit Message

Guo Ren May 25, 2020, 2:18 p.m. UTC 
From: Guo Ren <guoren@linux.alibaba.com>

Obviously, there is no need to recover a0-a7 in reject path.

Previous modification is from commit af33d243 by Tycho, to
fixup seccomp reject syscall code path.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Tycho Andersen <tycho@tycho.ws>
Cc: Kees Cook <keescook@chromium.org>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/riscv/kernel/entry.S | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Tycho Andersen May 25, 2020, 2:36 p.m. UTC | #1 
On Mon, May 25, 2020 at 02:18:26PM +0000, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> Obviously, there is no need to recover a0-a7 in reject path.
> 
> Previous modification is from commit af33d243 by Tycho, to
> fixup seccomp reject syscall code path.

Doesn't this suffer from the same problem, though? a7 is clobbered, so
the -ERESTARTSYS behavior won't work?

I haven't run the test case that was failing before.

Tycho
Guo Ren May 26, 2020, 12:29 a.m. UTC | #2 
Hi Tycho,

On Mon, May 25, 2020 at 10:36 PM Tycho Andersen <tycho@tycho.ws> wrote:
>
> On Mon, May 25, 2020 at 02:18:26PM +0000, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Obviously, there is no need to recover a0-a7 in reject path.
> >
> > Previous modification is from commit af33d243 by Tycho, to
> > fixup seccomp reject syscall code path.
>
> Doesn't this suffer from the same problem, though? a7 is clobbered, so
> the -ERESTARTSYS behavior won't work?

Look, the patch only affects the path of ret_from_syscall_rejected,
and there are two possible paths:
1. ret_from_syscall_rejected->handle_syscall_trace_exit->ret_from_exception
2. ret_from_syscall_rejected->ret_from_exception

All the above skip the check_syscall_nr and ignore the current a7, in
the C function they use the pt_regs in the stack to get proper reg's
value.

For the -ERESTARTSYS, we only process it in:
ret_from_exception->resume_userspace->work_notifysig->do_notify_resume:
do_signal & handle_signal:

                switch (regs->a0) {
                case -ERESTARTNOHAND:
                case -ERESTARTSYS:
                case -ERESTARTNOINTR:
                        regs->a0 = regs->orig_a0;
                        regs->epc -= 0x4;
                        break;

All above are done in pt_regs and when returning to userspace, a7 will
be recovered by restore_all in entry.S.

                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Tycho Andersen May 26, 2020, 1:38 p.m. UTC | #3 
On Tue, May 26, 2020 at 08:29:45AM +0800, Guo Ren wrote:
> Hi Tycho,
> 
> On Mon, May 25, 2020 at 10:36 PM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > On Mon, May 25, 2020 at 02:18:26PM +0000, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > Obviously, there is no need to recover a0-a7 in reject path.
> > >
> > > Previous modification is from commit af33d243 by Tycho, to
> > > fixup seccomp reject syscall code path.
> >
> > Doesn't this suffer from the same problem, though? a7 is clobbered, so
> > the -ERESTARTSYS behavior won't work?
> 
> Look, the patch only affects the path of ret_from_syscall_rejected,
> and there are two possible paths:
> 1. ret_from_syscall_rejected->handle_syscall_trace_exit->ret_from_exception
> 2. ret_from_syscall_rejected->ret_from_exception
> 
> All the above skip the check_syscall_nr and ignore the current a7, in
> the C function they use the pt_regs in the stack to get proper reg's
> value.
> 
> For the -ERESTARTSYS, we only process it in:
> ret_from_exception->resume_userspace->work_notifysig->do_notify_resume:
> do_signal & handle_signal:
> 
>                 switch (regs->a0) {
>                 case -ERESTARTNOHAND:
>                 case -ERESTARTSYS:
>                 case -ERESTARTNOINTR:
>                         regs->a0 = regs->orig_a0;
>                         regs->epc -= 0x4;
>                         break;
> 
> All above are done in pt_regs and when returning to userspace, a7 will
> be recovered by restore_all in entry.S.

Yes, thanks for that explanation.

Reviewed-by: Tycho Andersen <tycho@tycho.ws>
diff mbox series

Patch

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 56d071b..ea3444d 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -306,7 +306,7 @@  work_resched:
 handle_syscall_trace_enter:
 	move a0, sp
 	call do_syscall_trace_enter
-	move t0, a0
+	bnez a0, ret_from_syscall_rejected
 	REG_L a0, PT_A0(sp)
 	REG_L a1, PT_A1(sp)
 	REG_L a2, PT_A2(sp)
@@ -315,7 +315,6 @@  handle_syscall_trace_enter:
 	REG_L a5, PT_A5(sp)
 	REG_L a6, PT_A6(sp)
 	REG_L a7, PT_A7(sp)
-	bnez t0, ret_from_syscall_rejected
 	j check_syscall_nr
 handle_syscall_trace_exit:
 	move a0, sp