Re: [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
signature.asc
Description: PGP signature
Re: [PATCH v6] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
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...
> 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.
Oleg.
[PATCH v6] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
From: Elvira Khabirova
Arch code should use tracehook_*() helpers, as documented
in include/linux/tracehook.h,
ptrace_report_syscall() is not expected to be used outside that file.
Co-authored-by: Dmitry V. Levin
Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
Cc: Michael Ellerman
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Oleg Nesterov
Cc: Breno Leitao
Cc: Andy Lutomirski
Cc: Eugene Syromyatnikov
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Elvira Khabirova
Signed-off-by: Dmitry V. Levin
---
Please make either v5 or v6 edition of this fix, or any similar fix,
into v4.20.
v6: reverted to a fixed version of v4, compile- and run-tested with strace
v5: reverted to a simple approach, compile- and run-tested
v4: rewritten to call tracehook_report_syscall_entry() once, compile-tested
v3: add a descriptive comment
v2: explicitly ignore tracehook_report_syscall_entry() return code
arch/powerpc/kernel/ptrace.c | 54 +++-
1 file changed, 35 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index afb819f4ca68..fcfdc1229f08 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3263,27 +3263,43 @@ static inline int do_seccomp(struct pt_regs *regs) {
return 0; }
*/
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))
trace_sys_enter(regs, regs->gpr[0]);
#ifdef CONFIG_PPC64
--
ldv
