lkml.org 
[lkml]   [2018]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
On Mon, Dec 10, 2018 at 02:28:07PM +0100, Oleg Nesterov wrote:
> On 12/07, Dmitry V. Levin wrote:
> >
> > Please make either v5 or v6 edition of this fix, or any similar fix,
> > into v4.20.
>
> IIUC, v5 above means
>
> [PATCH v5 23/25] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
>
> you sent in another series...

They just happen to have the same v5 here and there.
In that series I included the most trivial variant of the change.

> > long do_syscall_trace_enter(struct pt_regs *regs)
> > {
> > + struct thread_info *ti;
> > + u32 cached_flags;
> > +
> > user_exit();
> >
> > - if (test_thread_flag(TIF_SYSCALL_EMU)) {
> > - ptrace_report_syscall(regs);
> > - /*
> > - * Returning -1 will skip the syscall execution. We want to
> > - * avoid clobbering any register also, thus, not 'gotoing'
> > - * skip label.
> > - */
> > - return -1;
> > - }
> > + ti = current_thread_info();
> > + cached_flags = READ_ONCE(ti->flags) &
> > + (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE |
> > + _TIF_SYSCALL_TRACEPOINT);
> >
> > - /*
> > - * The tracer may decide to abort the syscall, if so tracehook
> > - * will return !0. Note that the tracer may also just change
> > - * regs->gpr[0] to an invalid syscall number, that is handled
> > - * below on the exit path.
> > - */
> > - if (test_thread_flag(TIF_SYSCALL_TRACE) &&
> > - tracehook_report_syscall_entry(regs))
> > - goto skip;
> > + if (cached_flags & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> > + int rc = tracehook_report_syscall_entry(regs);
> > +
> > + if (unlikely(cached_flags & _TIF_SYSCALL_EMU)) {
> > + /*
> > + * A nonzero return code from
> > + * tracehook_report_syscall_entry() tells us
> > + * to prevent the syscall execution, but
> > + * we are not going to execute it anyway.
> > + *
> > + * Returning -1 will skip the syscall execution.
> > + * We want to avoid clobbering any register also,
> > + * thus, not 'gotoing' skip label.
> > + */
> > + return -1;
> > + }
> > +
> > + if (rc) {
> > + /*
> > + * The tracer decided to abort the syscall.
> > + * Note that the tracer may also just change
> > + * regs->gpr[0] to an invalid syscall number,
> > + * that is handled below on the exit path.
> > + */
> > + goto skip;
> > + }
> > + }
> >
> > /* Run seccomp after ptrace; allow it to set gpr[3]. */
> > if (do_seccomp(regs))
> > @@ -3293,7 +3309,7 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> > if (regs->gpr[0] >= NR_syscalls)
> > goto skip;
> >
> > - if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> > + if (unlikely(cached_flags & _TIF_SYSCALL_TRACEPOINT))
>
> I will leave this to maintainers, but to me this change looks good and imo it
> also cleanups the code.
>
> However I am not sure cached_flags should include _TIF_SYSCALL_TRACEPOINT. If
> nothing else, the caller can sleep in ptrace_stop() unpredictably long and
> TIF_SYSCALL_TRACEPOINT can be set/cleared meanwhile.

I agree, we shouldn't cache _TIF_SYSCALL_TRACEPOINT.


--
ldv
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-12-10 14:38    [W:0.088 / U:3.640 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site