Re: [PATCH v6] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

[Dmitry V. Levin]

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

Attachment: signature.asc
Description: PGP signature