* [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; 20+ messages in thread
From: Elvira Khabirova @ 2018-11-16 11:17 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: esyr, linux-kernel, ldv, luto, oleg, leitao, linuxppc-dev
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 related [flat|nested] 20+ 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; 20+ messages in thread
From: Michael Ellerman @ 2018-11-16 12:42 UTC (permalink / raw)
To: Elvira Khabirova, benh, paulus
Cc: esyr, linux-kernel, ldv, luto, oleg, leitao, linuxppc-dev
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] 20+ 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; 20+ messages in thread
From: Dmitry V. Levin @ 2018-11-19 21:01 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-kernel, Oleg Nesterov, Eugene Syromyatnikov,
Elvira Khabirova, Andy Lutomirski, Breno Leitao, linuxppc-dev
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 related [flat|nested] 20+ 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; 20+ messages in thread
From: Michael Ellerman @ 2018-11-21 21:17 UTC (permalink / raw)
To: Dmitry V. Levin, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-kernel, Oleg Nesterov, Eugene Syromyatnikov,
Elvira Khabirova, Andy Lutomirski, Breno Leitao, linuxppc-dev
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] 20+ 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; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-03 3:18 UTC (permalink / raw)
To: Michael Ellerman
Cc: Oleg Nesterov, Eugene Syromyatnikov, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, 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 related [flat|nested] 20+ 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; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 1:19 UTC (permalink / raw)
To: Michael Ellerman
Cc: Oleg Nesterov, Eugene Syromyatnikov, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, 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] 20+ 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; 20+ messages in thread
From: Michael Ellerman @ 2018-12-07 11:12 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, Eugene Syromyatnikov, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, 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] 20+ 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; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 15:42 UTC (permalink / raw)
To: Michael Ellerman
Cc: Oleg Nesterov, Eugene Syromyatnikov, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, 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] 20+ 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] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call Michael Ellerman
2018-12-07 16:34 ` [PATCH v4] " Oleg Nesterov
1 sibling, 2 replies; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 15:56 UTC (permalink / raw)
To: Michael Ellerman
Cc: Oleg Nesterov, Eugene Syromyatnikov, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, 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 related [flat|nested] 20+ 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; 20+ messages in thread
From: Oleg Nesterov @ 2018-12-07 16:34 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Eugene Syromyatnikov, linux-kernel, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, linuxppc-dev
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] 20+ 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; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 18:42 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eugene Syromyatnikov, linux-kernel, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, linuxppc-dev
[-- 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] 20+ 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] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call Michael Ellerman
1 sibling, 1 reply; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-07 18:52 UTC (permalink / raw)
To: Michael Ellerman
Cc: Oleg Nesterov, Eugene Syromyatnikov, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, 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 related [flat|nested] 20+ 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; 20+ messages in thread
From: Oleg Nesterov @ 2018-12-10 13:28 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Eugene Syromyatnikov, linux-kernel, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, linuxppc-dev
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] 20+ 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
2018-12-16 17:28 ` [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter Dmitry V. Levin
0 siblings, 1 reply; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-10 13:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eugene Syromyatnikov, linux-kernel, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, Breno Leitao, linuxppc-dev
[-- 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] 20+ 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; 20+ messages in thread
From: Michael Ellerman @ 2018-12-11 13:45 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: linux-kernel, Oleg Nesterov, Eugene Syromyatnikov,
Elvira Khabirova, Paul Mackerras, Andy Lutomirski, Breno Leitao,
linuxppc-dev
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] 20+ messages in thread
* [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter
2018-12-10 13:36 ` Dmitry V. Levin
@ 2018-12-16 17:28 ` Dmitry V. Levin
2018-12-17 11:20 ` Michael Ellerman
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-16 17:28 UTC (permalink / raw)
To: Michael Ellerman, Oleg Nesterov; +Cc: linuxppc-dev, linux-kernel
Invoke tracehook_report_syscall_entry once.
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
arch/powerpc/kernel/ptrace.c | 54 +++++++++++++++++++++---------------
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 714c3480c52d..8794d32c2d9e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -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;
+
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))
--
ldv
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter
2018-12-16 17:28 ` [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter Dmitry V. Levin
@ 2018-12-17 11:20 ` Michael Ellerman
2018-12-17 11:23 ` Dmitry V. Levin
2018-12-17 11:27 ` Oleg Nesterov
2018-12-22 9:54 ` Michael Ellerman
2 siblings, 1 reply; 20+ messages in thread
From: Michael Ellerman @ 2018-12-17 11:20 UTC (permalink / raw)
To: Dmitry V. Levin, Oleg Nesterov; +Cc: linuxppc-dev, linux-kernel
"Dmitry V. Levin" <ldv@altlinux.org> writes:
> Invoke tracehook_report_syscall_entry once.
Thanks.
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
> arch/powerpc/kernel/ptrace.c | 54 +++++++++++++++++++++---------------
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 714c3480c52d..8794d32c2d9e 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -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.
cheers
> 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))
> --
> ldv
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter
2018-12-17 11:20 ` Michael Ellerman
@ 2018-12-17 11:23 ` Dmitry V. Levin
0 siblings, 0 replies; 20+ messages in thread
From: Dmitry V. Levin @ 2018-12-17 11:23 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Oleg Nesterov, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]
Hi,
On Mon, Dec 17, 2018 at 10:20:26PM +1100, Michael Ellerman wrote:
> "Dmitry V. Levin" <ldv@altlinux.org> writes:
> > Invoke tracehook_report_syscall_entry once.
>
> Thanks.
>
> > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> > ---
> > arch/powerpc/kernel/ptrace.c | 54 +++++++++++++++++++++---------------
> > 1 file changed, 31 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > index 714c3480c52d..8794d32c2d9e 100644
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -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.
No problem, feel free to call it whatever you like. Thanks,
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter
2018-12-16 17:28 ` [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter Dmitry V. Levin
2018-12-17 11:20 ` Michael Ellerman
@ 2018-12-17 11:27 ` Oleg Nesterov
2018-12-22 9:54 ` Michael Ellerman
2 siblings, 0 replies; 20+ messages in thread
From: Oleg Nesterov @ 2018-12-17 11:27 UTC (permalink / raw)
To: Dmitry V. Levin; +Cc: linuxppc-dev, linux-kernel
On 12/16, Dmitry V. Levin wrote:
>
> long do_syscall_trace_enter(struct pt_regs *regs)
> {
> + 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;
> + }
> +
> + 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;
> + }
> + }
Looks good to me,
Oleg.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: powerpc/ptrace: cleanup do_syscall_trace_enter
2018-12-16 17:28 ` [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter Dmitry V. Levin
2018-12-17 11:20 ` Michael Ellerman
2018-12-17 11:27 ` Oleg Nesterov
@ 2018-12-22 9:54 ` Michael Ellerman
2 siblings, 0 replies; 20+ messages in thread
From: Michael Ellerman @ 2018-12-22 9:54 UTC (permalink / raw)
To: Dmitry V. Levin, Oleg Nesterov; +Cc: linuxppc-dev, linux-kernel
On Sun, 2018-12-16 at 17:28:28 UTC, "Dmitry V. Levin" wrote:
> Invoke tracehook_report_syscall_entry once.
>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/8dbdec0bcb416d0ef0bfd737620d08
cheers
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-12-22 10:37 UTC | newest]
Thread overview: 20+ 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-16 17:28 ` [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter Dmitry V. Levin
2018-12-17 11:20 ` Michael Ellerman
2018-12-17 11:23 ` Dmitry V. Levin
2018-12-17 11:27 ` Oleg Nesterov
2018-12-22 9:54 ` Michael Ellerman
2018-12-11 13:45 ` [v5] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call Michael Ellerman
2018-12-07 16:34 ` [PATCH v4] " Oleg Nesterov
2018-12-07 18:42 ` Dmitry V. Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).