2018-11-16 11:18:09

by Elvira Khabirova

[permalink] [raw]
Subject: [PATCH v2] 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(-)

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



2018-11-16 12:43:25

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

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

2018-11-19 21:02:35

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH v3] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

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

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

2018-11-22 08:16:19

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v3] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

Hi Dmitry,

Thanks for the patch.

"Dmitry V. Levin" <[email protected]> writes:
> 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(-)
>
> 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

2018-12-03 03:21:17

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

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

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

2018-12-07 01:21:19

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

On Mon, Dec 03, 2018 at 06:18:23AM +0300, Dmitry V. Levin wrote:
> 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(-)

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


Attachments:
(No filename) (1.10 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-07 11:15:29

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

"Dmitry V. Levin" <[email protected]> writes:

> On Mon, Dec 03, 2018 at 06:18:23AM +0300, Dmitry V. Levin wrote:
>> 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(-)
>
> 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

2018-12-07 15:43:51

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

On Fri, Dec 07, 2018 at 10:12:49PM +1100, Michael Ellerman wrote:
> "Dmitry V. Levin" <[email protected]> writes:
> > On Mon, Dec 03, 2018 at 06:18:23AM +0300, Dmitry V. Levin wrote:
> >> 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(-)
> >
> > 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


Attachments:
(No filename) (1.99 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-07 15:57:00

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH v5] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

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

2018-12-07 16:37:04

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

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.


2018-12-07 18:45:01

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v4] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

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


Attachments:
(No filename) (773.00 B)
signature.asc (817.00 B)
Download all attachments

2018-12-07 18:54:27

by Dmitry V. Levin

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

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")
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]>
---
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

2018-12-10 14:47:01

by Oleg Nesterov

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

On 12/07, Dmitry V. Levin wrote:
>
> Please make either v5 or v6 edition of this fix, or any similar fix,
> into v4.20.

IIUC, v5 above means

[PATCH v5 23/25] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

you sent in another series...

> long do_syscall_trace_enter(struct pt_regs *regs)
> {
> + struct thread_info *ti;
> + u32 cached_flags;
> +
> user_exit();
>
> - if (test_thread_flag(TIF_SYSCALL_EMU)) {
> - ptrace_report_syscall(regs);
> - /*
> - * Returning -1 will skip the syscall execution. We want to
> - * avoid clobbering any register also, thus, not 'gotoing'
> - * skip label.
> - */
> - return -1;
> - }
> + ti = current_thread_info();
> + cached_flags = READ_ONCE(ti->flags) &
> + (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE |
> + _TIF_SYSCALL_TRACEPOINT);
>
> - /*
> - * The tracer may decide to abort the syscall, if so tracehook
> - * will return !0. Note that the tracer may also just change
> - * regs->gpr[0] to an invalid syscall number, that is handled
> - * below on the exit path.
> - */
> - if (test_thread_flag(TIF_SYSCALL_TRACE) &&
> - tracehook_report_syscall_entry(regs))
> - goto skip;
> + if (cached_flags & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> + int rc = tracehook_report_syscall_entry(regs);
> +
> + if (unlikely(cached_flags & _TIF_SYSCALL_EMU)) {
> + /*
> + * A nonzero return code from
> + * tracehook_report_syscall_entry() tells us
> + * to prevent the syscall execution, but
> + * we are not going to execute it anyway.
> + *
> + * Returning -1 will skip the syscall execution.
> + * We want to avoid clobbering any register also,
> + * thus, not 'gotoing' skip label.
> + */
> + return -1;
> + }
> +
> + if (rc) {
> + /*
> + * The tracer decided to abort the syscall.
> + * Note that the tracer may also just change
> + * regs->gpr[0] to an invalid syscall number,
> + * that is handled below on the exit path.
> + */
> + goto skip;
> + }
> + }
>
> /* Run seccomp after ptrace; allow it to set gpr[3]. */
> if (do_seccomp(regs))
> @@ -3293,7 +3309,7 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> if (regs->gpr[0] >= NR_syscalls)
> goto skip;
>
> - if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> + if (unlikely(cached_flags & _TIF_SYSCALL_TRACEPOINT))

I will leave this to maintainers, but to me this change looks good and imo it
also cleanups the code.

However I am not sure cached_flags should include _TIF_SYSCALL_TRACEPOINT. If
nothing else, the caller can sleep in ptrace_stop() unpredictably long and
TIF_SYSCALL_TRACEPOINT can be set/cleared meanwhile.

Oleg.


2018-12-10 16:07:30

by Dmitry V. Levin

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

On Mon, Dec 10, 2018 at 02:28:07PM +0100, Oleg Nesterov wrote:
> On 12/07, Dmitry V. Levin wrote:
> >
> > Please make either v5 or v6 edition of this fix, or any similar fix,
> > into v4.20.
>
> IIUC, v5 above means
>
> [PATCH v5 23/25] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
>
> you sent in another series...

They just happen to have the same v5 here and there.
In that series I included the most trivial variant of the change.

> > long do_syscall_trace_enter(struct pt_regs *regs)
> > {
> > + struct thread_info *ti;
> > + u32 cached_flags;
> > +
> > user_exit();
> >
> > - if (test_thread_flag(TIF_SYSCALL_EMU)) {
> > - ptrace_report_syscall(regs);
> > - /*
> > - * Returning -1 will skip the syscall execution. We want to
> > - * avoid clobbering any register also, thus, not 'gotoing'
> > - * skip label.
> > - */
> > - return -1;
> > - }
> > + ti = current_thread_info();
> > + cached_flags = READ_ONCE(ti->flags) &
> > + (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE |
> > + _TIF_SYSCALL_TRACEPOINT);
> >
> > - /*
> > - * The tracer may decide to abort the syscall, if so tracehook
> > - * will return !0. Note that the tracer may also just change
> > - * regs->gpr[0] to an invalid syscall number, that is handled
> > - * below on the exit path.
> > - */
> > - if (test_thread_flag(TIF_SYSCALL_TRACE) &&
> > - tracehook_report_syscall_entry(regs))
> > - goto skip;
> > + if (cached_flags & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> > + int rc = tracehook_report_syscall_entry(regs);
> > +
> > + if (unlikely(cached_flags & _TIF_SYSCALL_EMU)) {
> > + /*
> > + * A nonzero return code from
> > + * tracehook_report_syscall_entry() tells us
> > + * to prevent the syscall execution, but
> > + * we are not going to execute it anyway.
> > + *
> > + * Returning -1 will skip the syscall execution.
> > + * We want to avoid clobbering any register also,
> > + * thus, not 'gotoing' skip label.
> > + */
> > + return -1;
> > + }
> > +
> > + if (rc) {
> > + /*
> > + * The tracer decided to abort the syscall.
> > + * Note that the tracer may also just change
> > + * regs->gpr[0] to an invalid syscall number,
> > + * that is handled below on the exit path.
> > + */
> > + goto skip;
> > + }
> > + }
> >
> > /* Run seccomp after ptrace; allow it to set gpr[3]. */
> > if (do_seccomp(regs))
> > @@ -3293,7 +3309,7 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> > if (regs->gpr[0] >= NR_syscalls)
> > goto skip;
> >
> > - if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> > + if (unlikely(cached_flags & _TIF_SYSCALL_TRACEPOINT))
>
> I will leave this to maintainers, but to me this change looks good and imo it
> also cleanups the code.
>
> However I am not sure cached_flags should include _TIF_SYSCALL_TRACEPOINT. If
> nothing else, the caller can sleep in ptrace_stop() unpredictably long and
> TIF_SYSCALL_TRACEPOINT can be set/cleared meanwhile.

I agree, we shouldn't cache _TIF_SYSCALL_TRACEPOINT.


--
ldv


Attachments:
(No filename) (3.09 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-11 13:48:08

by Michael Ellerman

[permalink] [raw]
Subject: Re: [v5] powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call

On Fri, 2018-12-07 at 15:56:05 UTC, "Dmitry V. Levin" wrote:
> 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]>

Applied to powerpc fixes, thanks.

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

cheers

2018-12-16 17:32:01

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter

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

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

2018-12-17 12:22:16

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter

"Dmitry V. Levin" <[email protected]> writes:
> Invoke tracehook_report_syscall_entry once.

Thanks.

> Signed-off-by: Dmitry V. Levin <[email protected]>
> ---
> 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

2018-12-17 12:22:35

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter

Hi,

On Mon, Dec 17, 2018 at 10:20:26PM +1100, Michael Ellerman wrote:
> "Dmitry V. Levin" <[email protected]> writes:
> > Invoke tracehook_report_syscall_entry once.
>
> Thanks.
>
> > Signed-off-by: Dmitry V. Levin <[email protected]>
> > ---
> > 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


Attachments:
(No filename) (1.00 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-17 13:47:28

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [PATCH] powerpc/ptrace: cleanup do_syscall_trace_enter

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.


2018-12-23 11:02:41

by Michael Ellerman

[permalink] [raw]
Subject: Re: powerpc/ptrace: cleanup do_syscall_trace_enter

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

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/8dbdec0bcb416d0ef0bfd737620d08

cheers