powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

Arch code should use tracehook_*() helpers, as documented in include/linux/tracehook.h.

Fixes 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")

Signed-off-by Elvira Khabirova <[email protected]>

---
 arch/powerpc/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/kernel/ptrace.c +1 -1

index afb819f4ca68..d79d907f9acc 100644
@@ -3266,7 +3266,7 @@ long do_syscall_trace_enter(struct pt_regs *regs)
 	user_exit();
 
 	if (test_thread_flag(TIF_SYSCALL_EMU)) {
-		ptrace_report_syscall(regs);
+		(void) tracehook_report_syscall_entry(regs);
 		/*
 		 * Returning -1 will skip the syscall execution. We want to
 		 * avoid clobbering any register also, thus, not 'gotoing'

Signature · 1 line

2.19.1

From: Elvira Khabirova <[email protected]>

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 <[email protected]>

Fixes 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")

Signed-off-by Elvira Khabirova <[email protected]>

Signed-off-by Dmitry V. Levin <[email protected]>

---

v3: add a descriptive comment
v2: explicitly ignore tracehook_report_syscall_entry() return code

 arch/powerpc/kernel/ptrace.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

arch/powerpc/kernel/ptrace.c +0 -0

index afb819f4ca68..e84220d91bbd 100644

From: Elvira Khabirova <[email protected]>

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 <[email protected]>

Fixes 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")

Signed-off-by Elvira Khabirova <[email protected]>

Signed-off-by Dmitry V. Levin <[email protected]>

---
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(-)

Quoted 15 lines · Dmitry V. Levin

From: Elvira Khabirova <[email protected]>

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 <[email protected]>

Fixes 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")

Signed-off-by Elvira Khabirova <[email protected]>

Signed-off-by Dmitry V. Levin <[email protected]>

---
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

Quoted 2 lines

 arch/powerpc/kernel/ptrace.c | 54 +++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 19 deletions(-)

Sorry, this patch does not work, please ignore it.

However, the bug blocks PTRACE_GET_SYSCALL_INFO, so please fix it.

I'm going to use

	if (tracehook_report_syscall_entry(regs))
		return -1;
	return -1;

in the series until you have a better fix.

Quoted 16 lines · Dmitry V. Levin

Quoted 15 lines · Dmitry V. Levin

From: Elvira Khabirova <[email protected]>

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 <[email protected]>

Fixes 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")

Signed-off-by Elvira Khabirova <[email protected]>

Signed-off-by Dmitry V. Levin <[email protected]>

---
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

Quoted 4 lines

Quoted 2 lines

 arch/powerpc/kernel/ptrace.c | 54 +++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 19 deletions(-)

Sorry, this patch does not work, please ignore it.

Hmm OK. Why exactly?

I wrote more or less the same patch, although I used a temporary bool.

Quoted 1 line

However, the bug blocks PTRACE_GET_SYSCALL_INFO, so please fix it.

Sorry, didn't realise it was blocking you.

Quoted 5 lines

I'm going to use

	if (tracehook_report_syscall_entry(regs))
		return -1;
	return -1;

in the series until you have a better fix.

Yeah that's fine by me. I could send that to Linus for 4.20 if you want me to, otherwise I'm fine for you to carry it in your series.

Please make either v5 or v6 edition of this fix, or any similar fix, into v4.20.

long do_syscall_trace_enter(struct pt_regs *regs) {

arch/powerpc/kernel/ptrace.c +34 -18

+	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))

Quoted 3 lines · Dmitry V. Levin

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...

Quoted 61 lines

long do_syscall_trace_enter(struct pt_regs *regs) {

arch/powerpc/kernel/ptrace.c +34 -18

+	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))

arch/powerpc/kernel/ptrace.c long do_syscall_trace_enter(struct pt_regs *regs) · line 3312

Quoted 5 lines

arch/powerpc/kernel/ptrace.c +1 -1

@@ -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.

Invoke tracehook_report_syscall_entry once.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 arch/powerpc/kernel/ptrace.c | 54 +++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 23 deletions(-)

arch/powerpc/kernel/ptrace.c +0 -0

index 714c3480c52d..8794d32c2d9e 100644

arch/powerpc/kernel/ptrace.c +29 -23

 	user_exit();
 
-	if (test_thread_flag(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.
-		 */
-		if (tracehook_report_syscall_entry(regs))
-			;
-		return -1;
-	}
+	cached_flags = READ_ONCE(current_thread_info()->flags) &
+		       (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
 
-	/*
-	 * 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) {
+		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))

Signature · 1 line

ldv

Quoted 1 line · Dmitry V. Levin

Invoke tracehook_report_syscall_entry once.

Thanks.

Quoted 10 lines

Signed-off-by Dmitry V. Levin <[email protected]>

---
 arch/powerpc/kernel/ptrace.c | 54 +++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 23 deletions(-)

arch/powerpc/kernel/ptrace.c +0 -0

index 714c3480c52d..8794d32c2d9e 100644

arch/powerpc/kernel/ptrace.c static inline int do_seccomp(struct pt_regs *regs) { return 0; } · line 3267

Quoted 5 lines

arch/powerpc/kernel/ptrace.c +2 -0

@@ -3263,32 +3263,40 @@ static inline int do_seccomp(struct pt_regs *regs) { return 0; }
  */
 long do_syscall_trace_enter(struct pt_regs *regs)
 {
+	u32 cached_flags;
+

Do you mind if I just call it "flags", I find "cached_flags" a bit unwieldy for some reason.

I'm happy to fix it up when applying.

long do_syscall_trace_enter(struct pt_regs *regs) {

arch/powerpc/kernel/ptrace.c +21 -23

+	u32 cached_flags;
+
 	user_exit();
 
-	if (test_thread_flag(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.
-		 */
-		if (tracehook_report_syscall_entry(regs))
-			;
-		return -1;
-	}
+	cached_flags = READ_ONCE(current_thread_info()->flags) &
+		       (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
 
-	/*
-	 * 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) {
+		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;
+		}
+

Invoke tracehook_report_syscall_entry once.

Signed-off-by Dmitry V. Levin <[email protected]>

From: Elvira Khabirova <[email protected]>

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.

The patch does not look very nice, but at least it is correct and opens the way for PTRACE_GET_SYSCALL_INFO API.

Co-authored-by Dmitry V. Levin <[email protected]>

Fixes 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")

Cc Michael Ellerman <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Breno Leitao <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Elvira Khabirova <[email protected]>

Signed-off-by Dmitry V. Levin <[email protected]>

Quoted 3 lines · Michael Ellerman

Quoted 1 line

Sorry, this patch does not work, please ignore it.

Hmm OK. Why exactly?

Unfortunately, I have no idea why it doesn't work.
All I can say is it breaks strace because the kernel no longer sends syscall entry stops.

Quoted 9 lines · Dmitry V. Levin

Quoted 3 lines · Michael Ellerman

Quoted 1 line

Sorry, this patch does not work, please ignore it.

Hmm OK. Why exactly?

Unfortunately, I have no idea why it doesn't work.
All I can say is it breaks strace because the kernel no longer sends syscall entry stops.

May be because TIF_SYSCALL_EMU/etc is a bit number, not a mask? IOW, rather than

whatever & TIF_XXX

you should do

whatever & _TIF_XXX

intstead?