| Message ID | 20190318104925.16600-5-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ptrace: consolidate PTRACE_SYSEMU handling and add support for arm64 | expand |
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. -- ldv
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
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.
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
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.
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
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.
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.
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 --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]. */
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(-) -- 2.17.1