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
signature.asc
Description: PGP signature
