LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
@ 2018-11-16 11:17 Elvira Khabirova
  2018-11-16 12:42 ` Michael Ellerman
  0 siblings, 1 reply; 15+ messages in thread
From: Elvira Khabirova @ 2018-11-16 11:17 UTC (permalink / raw)
  To: benh, paulus, mpe
  Cc: linuxppc-dev, linux-kernel, leitao, oleg, luto, ldv, esyr

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 <lineprinter@altlinux.org>
---
 arch/powerpc/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index afb819f4ca68..d79d907f9acc 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -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'
-- 
2.19.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-11-16 11:17 [PATCH v2] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call Elvira Khabirova
@ 2018-11-16 12:42 ` Michael Ellerman
  2018-11-19 21:01   ` [PATCH v3] " Dmitry V. Levin
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Ellerman @ 2018-11-16 12:42 UTC (permalink / raw)
  To: Elvira Khabirova, benh, paulus
  Cc: linuxppc-dev, linux-kernel, leitao, oleg, luto, ldv, esyr

Elvira Khabirova <lineprinter@altlinux.org> writes:

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

Thanks.

It probably also should have a comment explaining why we're ignoring the
return value and why that's OK.

cheers

> Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
> Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
> ---
>  arch/powerpc/kernel/ptrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index afb819f4ca68..d79d907f9acc 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -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'
> -- 
> 2.19.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-11-16 12:42 ` Michael Ellerman
@ 2018-11-19 21:01   ` " Dmitry V. Levin
  2018-11-21 21:17     ` Michael Ellerman
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry V. Levin @ 2018-11-19 21:01 UTC (permalink / raw)
  To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
  Cc: Oleg Nesterov, Breno Leitao, Andy Lutomirski,
	Eugene Syromyatnikov, Elvira Khabirova, linuxppc-dev,
	linux-kernel

From: Elvira Khabirova <lineprinter@altlinux.org>

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 <ldv@altlinux.org>
Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---

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

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index afb819f4ca68..e84220d91bbd 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3266,7 +3266,12 @@ long do_syscall_trace_enter(struct pt_regs *regs)
 	user_exit();
 
 	if (test_thread_flag(TIF_SYSCALL_EMU)) {
-		ptrace_report_syscall(regs);
+		/*
+		 * 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.
+		 */
+		(void) tracehook_report_syscall_entry(regs);
 		/*
 		 * Returning -1 will skip the syscall execution. We want to
 		 * avoid clobbering any register also, thus, not 'gotoing'
-- 
ldv

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-11-19 21:01   ` [PATCH v3] " Dmitry V. Levin
@ 2018-11-21 21:17     ` Michael Ellerman
  2018-12-03  3:18       ` [PATCH v4] " Dmitry V. Levin
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Ellerman @ 2018-11-21 21:17 UTC (permalink / raw)
  To: Dmitry V. Levin, Benjamin Herrenschmidt, Paul Mackerras
  Cc: Oleg Nesterov, Breno Leitao, Andy Lutomirski,
	Eugene Syromyatnikov, Elvira Khabirova, linuxppc-dev,
	linux-kernel

Hi Dmitry,

Thanks for the patch.

"Dmitry V. Levin" <ldv@altlinux.org> writes:
> From: Elvira Khabirova <lineprinter@altlinux.org>
>
> 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 <ldv@altlinux.org>
> Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
> Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
>
> 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(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index afb819f4ca68..e84220d91bbd 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3266,7 +3266,12 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>  	user_exit();
>  
>  	if (test_thread_flag(TIF_SYSCALL_EMU)) {
> -		ptrace_report_syscall(regs);
> +		/*
> +		 * 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.
> +		 */
> +		(void) tracehook_report_syscall_entry(regs);

Unfortunately the (void) cast doesn't work to suppress the must check
warning.

  arch/powerpc/kernel/ptrace.c:3274:3: error: ignoring return value of 'tracehook_report_syscall_entry', declared with attribute warn_unused_result [-Werror=unused-result]

AFAIK we don't have a way to suppress that.

I guess we should rewrite it to only call
tracehook_report_syscall_entry() once, like x86 does.

I'll try and get a patch for that done & tested.

cheers

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-11-21 21:17     ` Michael Ellerman
@ 2018-12-03  3:18       ` " Dmitry V. Levin
  2018-12-07  1:19         ` Dmitry V. Levin
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry V. Levin @ 2018-12-03  3:18 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Oleg Nesterov,
	Breno Leitao, Andy Lutomirski, Eugene Syromyatnikov,
	Elvira Khabirova, linuxppc-dev, linux-kernel

From: Elvira Khabirova <lineprinter@altlinux.org>

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 <ldv@altlinux.org>
Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
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..59c8c9a3d7ea 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-03  3:18       ` [PATCH v4] " Dmitry V. Levin
@ 2018-12-07  1:19         ` Dmitry V. Levin
  2018-12-07 11:12           ` Michael Ellerman
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry V. Levin @ 2018-12-07  1:19 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Oleg Nesterov,
	Breno Leitao, Andy Lutomirski, Eugene Syromyatnikov,
	Elvira Khabirova, linuxppc-dev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On Mon, Dec 03, 2018 at 06:18:23AM +0300, Dmitry V. Levin wrote:
> From: Elvira Khabirova <lineprinter@altlinux.org>
> 
> 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 <ldv@altlinux.org>
> Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
> Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
> 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(-)

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.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07  1:19         ` Dmitry V. Levin
@ 2018-12-07 11:12           ` Michael Ellerman
  2018-12-07 15:42             ` Dmitry V. Levin
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Ellerman @ 2018-12-07 11:12 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Oleg Nesterov,
	Breno Leitao, Andy Lutomirski, Eugene Syromyatnikov,
	Elvira Khabirova, linuxppc-dev, linux-kernel

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> On Mon, Dec 03, 2018 at 06:18:23AM +0300, Dmitry V. Levin wrote:
>> From: Elvira Khabirova <lineprinter@altlinux.org>
>> 
>> 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 <ldv@altlinux.org>
>> Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
>> Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
>> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
>> ---
>> 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(-)
>
> 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.

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

Sorry, didn't realise it was blocking you.

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

cheers

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07 11:12           ` Michael Ellerman
@ 2018-12-07 15:42             ` Dmitry V. Levin
  2018-12-07 15:56               ` [PATCH v5] " Dmitry V. Levin
  2018-12-07 16:34               ` [PATCH v4] " Oleg Nesterov
  0 siblings, 2 replies; 15+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 15:42 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Oleg Nesterov,
	Breno Leitao, Andy Lutomirski, Eugene Syromyatnikov,
	Elvira Khabirova, linuxppc-dev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2035 bytes --]

On Fri, Dec 07, 2018 at 10:12:49PM +1100, Michael Ellerman wrote:
> "Dmitry V. Levin" <ldv@altlinux.org> writes:
> > On Mon, Dec 03, 2018 at 06:18:23AM +0300, Dmitry V. Levin wrote:
> >> From: Elvira Khabirova <lineprinter@altlinux.org>
> >> 
> >> 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 <ldv@altlinux.org>
> >> Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
> >> Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
> >> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> >> ---
> >> 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(-)
> >
> > 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.

> I wrote more or less the same patch, although I used a temporary bool.
> 
> > However, the bug blocks PTRACE_GET_SYSCALL_INFO, so please fix it.
> 
> Sorry, didn't realise it was blocking you.

We are changing ptrace_report_syscall signature to implement
PTRACE_GET_SYSCALL_INFO, and this is the only place in the kernel besides
tracehook_report_syscall_*() that invokes ptrace_report_syscall() directly.

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

Yes, please.  I'll send a v5 shortly.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07 15:42             ` Dmitry V. Levin
@ 2018-12-07 15:56               ` " Dmitry V. Levin
  2018-12-07 18:52                 ` [PATCH v6] " Dmitry V. Levin
  2018-12-11 13:45                 ` [v5] " Michael Ellerman
  2018-12-07 16:34               ` [PATCH v4] " Oleg Nesterov
  1 sibling, 2 replies; 15+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 15:56 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Elvira Khabirova, Benjamin Herrenschmidt, Paul Mackerras,
	Oleg Nesterov, Breno Leitao, Andy Lutomirski,
	Eugene Syromyatnikov, linuxppc-dev, linux-kernel

From: Elvira Khabirova <lineprinter@altlinux.org>

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 <ldv@altlinux.org>
Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Breno Leitao <leitao@debian.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Eugene Syromyatnikov <esyr@redhat.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index afb819f4ca68..714c3480c52d 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3266,12 +3266,17 @@ long do_syscall_trace_enter(struct pt_regs *regs)
 	user_exit();
 
 	if (test_thread_flag(TIF_SYSCALL_EMU)) {
-		ptrace_report_syscall(regs);
 		/*
+		 * 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;
 	}
 
-- 
ldv

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07 15:42             ` Dmitry V. Levin
  2018-12-07 15:56               ` [PATCH v5] " Dmitry V. Levin
@ 2018-12-07 16:34               ` " Oleg Nesterov
  2018-12-07 18:42                 ` Dmitry V. Levin
  1 sibling, 1 reply; 15+ messages in thread
From: Oleg Nesterov @ 2018-12-07 16:34 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Breno Leitao, Andy Lutomirski, Eugene Syromyatnikov,
	Elvira Khabirova, linuxppc-dev, linux-kernel

On 12/07, Dmitry V. Levin wrote:
>
> On Fri, Dec 07, 2018 at 10:12:49PM +1100, Michael Ellerman wrote:
>
> > > 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?

Oleg.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07 16:34               ` [PATCH v4] " Oleg Nesterov
@ 2018-12-07 18:42                 ` Dmitry V. Levin
  0 siblings, 0 replies; 15+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 18:42 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Breno Leitao, Andy Lutomirski, Eugene Syromyatnikov,
	Elvira Khabirova, linuxppc-dev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Fri, Dec 07, 2018 at 05:34:10PM +0100, Oleg Nesterov wrote:
> On 12/07, Dmitry V. Levin wrote:
> > On Fri, Dec 07, 2018 at 10:12:49PM +1100, Michael Ellerman wrote:
> >
> > > > 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?

Thanks Oleg, this was exactly the reason why it didn't work.
That kind of things happens when you let userspace people hack you kernel. :)


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07 15:56               ` [PATCH v5] " Dmitry V. Levin
@ 2018-12-07 18:52                 ` " Dmitry V. Levin
  2018-12-10 13:28                   ` Oleg Nesterov
  2018-12-11 13:45                 ` [v5] " Michael Ellerman
  1 sibling, 1 reply; 15+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 18:52 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Elvira Khabirova, Benjamin Herrenschmidt, Paul Mackerras,
	Oleg Nesterov, Breno Leitao, Andy Lutomirski,
	Eugene Syromyatnikov, linuxppc-dev, linux-kernel

From: Elvira Khabirova <lineprinter@altlinux.org>

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 <ldv@altlinux.org>
Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Breno Leitao <leitao@debian.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Eugene Syromyatnikov <esyr@redhat.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07 18:52                 ` [PATCH v6] " Dmitry V. Levin
@ 2018-12-10 13:28                   ` Oleg Nesterov
  2018-12-10 13:36                     ` Dmitry V. Levin
  0 siblings, 1 reply; 15+ messages in thread
From: Oleg Nesterov @ 2018-12-10 13:28 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Michael Ellerman, Elvira Khabirova, Benjamin Herrenschmidt,
	Paul Mackerras, Breno Leitao, Andy Lutomirski,
	Eugene Syromyatnikov, linuxppc-dev, linux-kernel

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-10 13:28                   ` Oleg Nesterov
@ 2018-12-10 13:36                     ` Dmitry V. Levin
  0 siblings, 0 replies; 15+ messages in thread
From: Dmitry V. Levin @ 2018-12-10 13:36 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Michael Ellerman, Elvira Khabirova, Benjamin Herrenschmidt,
	Paul Mackerras, Breno Leitao, Andy Lutomirski,
	Eugene Syromyatnikov, linuxppc-dev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3167 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [v5] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
  2018-12-07 15:56               ` [PATCH v5] " Dmitry V. Levin
  2018-12-07 18:52                 ` [PATCH v6] " Dmitry V. Levin
@ 2018-12-11 13:45                 ` " Michael Ellerman
  1 sibling, 0 replies; 15+ messages in thread
From: Michael Ellerman @ 2018-12-11 13:45 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Oleg Nesterov, Eugene Syromyatnikov, Elvira Khabirova,
	Paul Mackerras, Andy Lutomirski, Breno Leitao, linuxppc-dev,
	linux-kernel

On Fri, 2018-12-07 at 15:56:05 UTC, "Dmitry V. Levin" wrote:
> From: Elvira Khabirova <lineprinter@altlinux.org>
> 
> 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 <ldv@altlinux.org>
> Fixes: 5521eb4bca2d ("powerpc/ptrace: Add support for PTRACE_SYSEMU")
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Breno Leitao <leitao@debian.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Eugene Syromyatnikov <esyr@redhat.com>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/a225f1567405558fb5410e9b2b9080

cheers

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-16 11:17 [PATCH v2] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call Elvira Khabirova
2018-11-16 12:42 ` Michael Ellerman
2018-11-19 21:01   ` [PATCH v3] " Dmitry V. Levin
2018-11-21 21:17     ` Michael Ellerman
2018-12-03  3:18       ` [PATCH v4] " Dmitry V. Levin
2018-12-07  1:19         ` Dmitry V. Levin
2018-12-07 11:12           ` Michael Ellerman
2018-12-07 15:42             ` Dmitry V. Levin
2018-12-07 15:56               ` [PATCH v5] " Dmitry V. Levin
2018-12-07 18:52                 ` [PATCH v6] " Dmitry V. Levin
2018-12-10 13:28                   ` Oleg Nesterov
2018-12-10 13:36                     ` Dmitry V. Levin
2018-12-11 13:45                 ` [v5] " Michael Ellerman
2018-12-07 16:34               ` [PATCH v4] " Oleg Nesterov
2018-12-07 18:42                 ` Dmitry V. Levin

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox