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