diff mbox series

[v2,4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

Message ID 20190318104925.16600-5-sudeep.holla@arm.com (mailing list archive)
State Not Applicable
Headers show
Series ptrace: consolidate PTRACE_SYSEMU handling and add support for arm64 | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch next (de3c83c2fd2b87cf68214eda76dfa66989d78cb6)
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 56 lines checked

Commit Message

Sudeep Holla March 18, 2019, 10:49 a.m. UTC
Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in do_syscall_trace_enter.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

Comments

Dmitry V. Levin March 18, 2019, 2:26 p.m. UTC | #1
On Mon, Mar 18, 2019 at 10:49:23AM +0000, Sudeep Holla wrote:
> Now that we have a new hook ptrace_syscall_enter that can be called from
> syscall entry code and it handles PTRACE_SYSEMU in generic code, we
> can do some cleanup using the same in do_syscall_trace_enter.
> 
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 2e2183b800a8..05579a5dcb12 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>  
>  	user_exit();
>  
> -	flags = READ_ONCE(current_thread_info()->flags) &
> -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> -
> -	if (flags) {
> -		int rc = tracehook_report_syscall_entry(regs);
> +	if (unlikely(ptrace_syscall_enter(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 registers, so we don't goto the skip
> +		 * label below.
> +		 */
> +		return -1;
> +	}

This comment is out of sync with the changed code.
Sudeep Holla March 18, 2019, 2:59 p.m. UTC | #2
On Mon, Mar 18, 2019 at 05:26:18PM +0300, Dmitry V. Levin wrote:
> On Mon, Mar 18, 2019 at 10:49:23AM +0000, Sudeep Holla wrote:
> > Now that we have a new hook ptrace_syscall_enter that can be called from
> > syscall entry code and it handles PTRACE_SYSEMU in generic code, we
> > can do some cleanup using the same in do_syscall_trace_enter.
> > 
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
> >  1 file changed, 21 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > index 2e2183b800a8..05579a5dcb12 100644
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> >  
> >  	user_exit();
> >  
> > -	flags = READ_ONCE(current_thread_info()->flags) &
> > -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> > -
> > -	if (flags) {
> > -		int rc = tracehook_report_syscall_entry(regs);
> > +	if (unlikely(ptrace_syscall_enter(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 registers, so we don't goto the skip
> > +		 * label below.
> > +		 */
> > +		return -1;
> > +	}
> 
> This comment is out of sync with the changed code.

Still applicable indirectly as ptrace_syscall_enter just executes
tracehook_report_syscall_entry, but I agree needs rewording, will update.

--
Regards,
Sudeep
Oleg Nesterov March 18, 2019, 5:20 p.m. UTC | #3
On 03/18, Sudeep Holla wrote:
>
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>  
>  	user_exit();
>  
> -	flags = READ_ONCE(current_thread_info()->flags) &
> -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> -
> -	if (flags) {
> -		int rc = tracehook_report_syscall_entry(regs);
> +	if (unlikely(ptrace_syscall_enter(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 registers, so we don't goto the skip
> +		 * label below.
> +		 */
> +		return -1;
> +	}
>  
> -		if (unlikely(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 registers, so we don't goto
> -			 * the skip label below.
> -			 */
> -			return -1;
> -		}
> +	flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;

Why do we need READ_ONCE() with this change?

And now that we change a single bit "flags" doesn't look like a good name.

Again, to me this patch just makes the code look worse. Honestly, I don't
think that the new (badly named) ptrace_syscall_enter() hook makes any sense.

Oleg.
Sudeep Holla March 18, 2019, 5:24 p.m. UTC | #4
On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> On 03/18, Sudeep Holla wrote:
> >
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> >
> >  	user_exit();
> >
> > -	flags = READ_ONCE(current_thread_info()->flags) &
> > -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> > -
> > -	if (flags) {
> > -		int rc = tracehook_report_syscall_entry(regs);
> > +	if (unlikely(ptrace_syscall_enter(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 registers, so we don't goto the skip
> > +		 * label below.
> > +		 */
> > +		return -1;
> > +	}
> >
> > -		if (unlikely(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 registers, so we don't goto
> > -			 * the skip label below.
> > -			 */
> > -			return -1;
> > -		}
> > +	flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;
>
> Why do we need READ_ONCE() with this change?
>
> And now that we change a single bit "flags" doesn't look like a good name.
>
> Again, to me this patch just makes the code look worse. Honestly, I don't
> think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
>

Worse because we end up reading current_thread_info->flags twice ?

--
Regards,
Sudeep
Oleg Nesterov March 18, 2019, 5:33 p.m. UTC | #5
On 03/18, Sudeep Holla wrote:
>
> On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> >
> > Again, to me this patch just makes the code look worse. Honestly, I don't
> > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> >
>
> Worse because we end up reading current_thread_info->flags twice ?

Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
the caller's code less readable/understandable.

Sure, this is subjective.

Oleg.
Sudeep Holla March 18, 2019, 5:40 p.m. UTC | #6
On Mon, Mar 18, 2019 at 06:33:41PM +0100, Oleg Nesterov wrote:
> On 03/18, Sudeep Holla wrote:
> >
> > On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> > >
> > > Again, to me this patch just makes the code look worse. Honestly, I don't
> > > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> > >
> >
> > Worse because we end up reading current_thread_info->flags twice ?
>
> Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
> the caller's code less readable/understandable.
>
> Sure, this is subjective.
>

Based on what we have in that function today, I tend to agree. Will and
Richard were in the opinion to consolidate SYSEMU handling(in the threads
pointed in my cover letter). If there's a better way to achieve the same
I am in for it. I have just tried to put something together based on
what I could think of.

--
Regards,
Sudeep
Oleg Nesterov March 19, 2019, 5:08 p.m. UTC | #7
On 03/18, Sudeep Holla wrote:
>
> On Mon, Mar 18, 2019 at 06:33:41PM +0100, Oleg Nesterov wrote:
> > On 03/18, Sudeep Holla wrote:
> > >
> > > On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> > > >
> > > > Again, to me this patch just makes the code look worse. Honestly, I don't
> > > > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> > > >
> > >
> > > Worse because we end up reading current_thread_info->flags twice ?
> >
> > Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
> > the caller's code less readable/understandable.
> >
> > Sure, this is subjective.
> >
>
> Based on what we have in that function today, I tend to agree. Will and
> Richard were in the opinion to consolidate SYSEMU handling

Well, personally I see no point... Again, after the trivial simplification
x86 does

	if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
		ret = tracehook_report_syscall_entry(regs);
		if (ret || (work & _TIF_SYSCALL_EMU))
			return -1L;
	}

this looks simple enough for copy-and-paste.

> If there's a better way to achieve the same

I can only say that if we add a common helper, I think it should absorb
tracehook_report_syscall_entry() and handle both TIF's just like the code
above does. Not sure this makes any sense.

Oleg.
Oleg Nesterov March 19, 2019, 5:32 p.m. UTC | #8
On 03/19, Oleg Nesterov wrote:
>
> Well, personally I see no point... Again, after the trivial simplification
> x86 does
>
> 	if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> 		ret = tracehook_report_syscall_entry(regs);
> 		if (ret || (work & _TIF_SYSCALL_EMU))
> 			return -1L;
> 	}
>
> this looks simple enough for copy-and-paste.
>
> > If there's a better way to achieve the same
>
> I can only say that if we add a common helper, I think it should absorb
> tracehook_report_syscall_entry() and handle both TIF's just like the code
> above does. Not sure this makes any sense.

this won't work, looking at 6/6 I see that arm64 needs to distinguish
_TRACE and _EMU ... I don't understand this code, but it looks suspicious.
If tracehook_report_syscall_entry() returns nonzero the tracee was killed,
syscall_trace_enter() should just return.

To me this is another indication that consolidation makes no sense ;)

Oleg.
Will Deacon April 3, 2019, 4:50 p.m. UTC | #9
Hi Oleg,

On Tue, Mar 19, 2019 at 06:32:33PM +0100, Oleg Nesterov wrote:
> On 03/19, Oleg Nesterov wrote:
> >
> > Well, personally I see no point... Again, after the trivial simplification
> > x86 does
> >
> > 	if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> > 		ret = tracehook_report_syscall_entry(regs);
> > 		if (ret || (work & _TIF_SYSCALL_EMU))
> > 			return -1L;
> > 	}
> >
> > this looks simple enough for copy-and-paste.
> >
> > > If there's a better way to achieve the same
> >
> > I can only say that if we add a common helper, I think it should absorb
> > tracehook_report_syscall_entry() and handle both TIF's just like the code
> > above does. Not sure this makes any sense.
> 
> this won't work, looking at 6/6 I see that arm64 needs to distinguish
> _TRACE and _EMU ... I don't understand this code, but it looks suspicious.
> If tracehook_report_syscall_entry() returns nonzero the tracee was killed,
> syscall_trace_enter() should just return.
> 
> To me this is another indication that consolidation makes no sense ;)

The reason I'm pushing for consolidation here is because I think it's the
only sane way to maintain the tracing and debug hooks on the syscall
entry/exit paths. Having to look at all the different arch implementations
and distil the portable semantics is a nightmare and encourages gradual
divergence over time. Given that we don't support this SYSCALL_EMU stuff
on arm64 today, we have the opportunity to make this generic and allow other
architectures (e.g. riscv) to hook in the same way that we do. It clearly
shouldn't affect the behaviour of existing architectures which already
support the functionality.

However, I also agree that this patch series looks dodgy as it stands -- we
shouldn't have code paths that can result in calling
tracehook_report_syscall_entry() twice.

Will
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 2e2183b800a8..05579a5dcb12 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3278,35 +3278,29 @@  long do_syscall_trace_enter(struct pt_regs *regs)
 
 	user_exit();
 
-	flags = READ_ONCE(current_thread_info()->flags) &
-		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
-
-	if (flags) {
-		int rc = tracehook_report_syscall_entry(regs);
+	if (unlikely(ptrace_syscall_enter(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 registers, so we don't goto the skip
+		 * label below.
+		 */
+		return -1;
+	}
 
-		if (unlikely(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 registers, so we don't goto
-			 * the skip label below.
-			 */
-			return -1;
-		}
+	flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;
 
-		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;
-		}
+	if (flags && tracehook_report_syscall_entry(regs)) {
+		/*
+		 * 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]. */