| Message ID | 20190318104925.16600-4-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ptrace: consolidate PTRACE_SYSEMU handling and add support for arm64 | expand |
On 03/18, Sudeep Holla wrote: > > --- a/arch/x86/entry/common.c > +++ b/arch/x86/entry/common.c > @@ -70,22 +70,16 @@ static long syscall_trace_enter(struct pt_regs *regs) > > struct thread_info *ti = current_thread_info(); > unsigned long ret = 0; > - bool emulated = false; > u32 work; > > if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) > BUG_ON(regs != task_pt_regs(current)); > > - work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; > - > - if (unlikely(work & _TIF_SYSCALL_EMU)) > - emulated = true; > - > - if ((emulated || (work & _TIF_SYSCALL_TRACE)) && > - tracehook_report_syscall_entry(regs)) > + if (unlikely(ptrace_syscall_enter(regs))) > return -1L; > > - if (emulated) > + work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; > + if ((work & _TIF_SYSCALL_TRACE) && tracehook_report_syscall_entry(regs)) > return -1L; Well, I won't really argue, but to be honest I think this change doesn't make the code better... With this patch tracehook_report_syscall_entry() has 2 callers, to me this just adds some confusion. I agree that the usage of emulated/_TIF_SYSCALL_EMU looks a bit overcomplicated, I'd suggest a simple cleanup below. And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need "& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY should not include _TIF_NOHZ? Oleg. --- x/arch/x86/entry/common.c +++ x/arch/x86/entry/common.c @@ -70,23 +70,18 @@ static long syscall_trace_enter(struct pt_regs *regs) struct thread_info *ti = current_thread_info(); unsigned long ret = 0; - bool emulated = false; u32 work; if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) BUG_ON(regs != task_pt_regs(current)); - work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; + work = READ_ONCE(ti->flags); - if (unlikely(work & _TIF_SYSCALL_EMU)) - emulated = true; - - if ((emulated || (work & _TIF_SYSCALL_TRACE)) && - tracehook_report_syscall_entry(regs)) - return -1L; - - if (emulated) - return -1L; + if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) { + ret = tracehook_report_syscall_entry(regs); + if (ret || (work & _TIF_SYSCALL_EMU)) + return -1L; + } #ifdef CONFIG_SECCOMP /*
On Mon, Mar 18, 2019 at 04:33:22PM +0100, Oleg Nesterov wrote: > On 03/18, Sudeep Holla wrote: > > > > --- a/arch/x86/entry/common.c > > +++ b/arch/x86/entry/common.c > > @@ -70,22 +70,16 @@ static long syscall_trace_enter(struct pt_regs *regs) > > > > struct thread_info *ti = current_thread_info(); > > unsigned long ret = 0; > > - bool emulated = false; > > u32 work; > > > > if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) > > BUG_ON(regs != task_pt_regs(current)); > > > > - work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; > > - > > - if (unlikely(work & _TIF_SYSCALL_EMU)) > > - emulated = true; > > - > > - if ((emulated || (work & _TIF_SYSCALL_TRACE)) && > > - tracehook_report_syscall_entry(regs)) > > + if (unlikely(ptrace_syscall_enter(regs))) > > return -1L; > > > > - if (emulated) > > + work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; > > + if ((work & _TIF_SYSCALL_TRACE) && tracehook_report_syscall_entry(regs)) > > return -1L; > [...] > > And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need > "& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY > should not include _TIF_NOHZ? > I was about to post the updated version and checked this to make sure I have covered everything or not. I had missed the above comment. All architectures have _TIF_NOHZ in their mask that they check to do work. And from x86, I read "...syscall_trace_enter(). Also includes TIF_NOHZ for enter_from_user_mode()" So I don't understand why _TIF_NOHZ needs to be dropped. Also if we need to drop, we can address that separately examining all archs. I will post the cleanup as you suggested for now. -- Regards, Sudeep
On Mon, Mar 18, 2019 at 3:49 AM Sudeep Holla <sudeep.holla@arm.com> 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 syscall_trace_enter. > > Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP > in syscall_slow_exit_work seems unnecessary. Let's remove the same. > Unless the patch set contains a selftest that exercises all the interesting cases here, NAK. To be clear, there needs to be a test that passes on an unmodified kernel and still passes on a patched kernel. And that test case needs to *fail* if, for example, you force "emulated" to either true or false rather than reading out the actual value. --Andy
On 30/04/2019 17:46, Andy Lutomirski wrote: > On Mon, Mar 18, 2019 at 3:49 AM Sudeep Holla <sudeep.holla@arm.com> 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 syscall_trace_enter. >> >> Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP >> in syscall_slow_exit_work seems unnecessary. Let's remove the same. >> > > Unless the patch set contains a selftest that exercises all the > interesting cases here, NAK. To be clear, there needs to be a test > that passes on an unmodified kernel and still passes on a patched > kernel. And that test case needs to *fail* if, for example, you force > "emulated" to either true or false rather than reading out the actual > value. > Tested using tools/testing/selftests/x86/ptrace_syscall.c Also v3 doesn't change any logic or additional call to new function as in v2. It's just simple cleanup as suggested by Oleg.
On 04/30, Sudeep Holla wrote: > > On Mon, Mar 18, 2019 at 04:33:22PM +0100, Oleg Nesterov wrote: > > > > And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need > > "& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY > > should not include _TIF_NOHZ? > > > > I was about to post the updated version and checked this to make sure I have > covered everything or not. I had missed the above comment. All architectures > have _TIF_NOHZ in their mask that they check to do work. And from x86, I read > "...syscall_trace_enter(). Also includes TIF_NOHZ for enter_from_user_mode()" > So I don't understand why _TIF_NOHZ needs to be dropped. I have already forgot this discussion... But after I glanced at this code again I still think the same, and I don't understand why do you disagree. > Also if we need to drop, we can address that separately examining all archs. Sure, and I was only talking about x86. We can keep TIF_NOHZ and even set_tsk_thread_flag(TIF_NOHZ) in context_tracking_cpu_set() if some arch needs this but remove TIF_NOHZ from TIF_WORK_SYSCALL_ENTRY in arch/x86/include/asm/thread_info.h, afaics this shouldn't make any difference. And I see no reason why x86 needs to use TIF_WORK_SYSCALL_ENTRY in syscall_trace_enter(). Oleg.
On Wed, May 01, 2019 at 05:57:11PM +0200, Oleg Nesterov wrote: > On 04/30, Sudeep Holla wrote: > > > > On Mon, Mar 18, 2019 at 04:33:22PM +0100, Oleg Nesterov wrote: > > > > > > And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need > > > "& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY > > > should not include _TIF_NOHZ? > > > > > > > I was about to post the updated version and checked this to make sure I have > > covered everything or not. I had missed the above comment. All architectures > > have _TIF_NOHZ in their mask that they check to do work. And from x86, I read > > "...syscall_trace_enter(). Also includes TIF_NOHZ for enter_from_user_mode()" > > So I don't understand why _TIF_NOHZ needs to be dropped. > > I have already forgot this discussion... But after I glanced at this code again > I still think the same, and I don't understand why do you disagree. > Sorry, but I didn't have any disagreement, I just said I don't understand the usage on all architectures at that moment. > > Also if we need to drop, we can address that separately examining all archs. > > Sure, and I was only talking about x86. We can keep TIF_NOHZ and even > set_tsk_thread_flag(TIF_NOHZ) in context_tracking_cpu_set() if some arch needs > this but remove TIF_NOHZ from TIF_WORK_SYSCALL_ENTRY in arch/x86/include/asm/thread_info.h, > afaics this shouldn't make any difference. > OK, it's just x86, then I understand your point. I was looking at all the architectures, sorry for the confusion. > And I see no reason why x86 needs to use TIF_WORK_SYSCALL_ENTRY in > syscall_trace_enter(). > Agreed -- Regards, Sudeep
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c index 7bc105f47d21..5d7590994964 100644 --- a/arch/x86/entry/common.c +++ b/arch/x86/entry/common.c @@ -70,22 +70,16 @@ static long syscall_trace_enter(struct pt_regs *regs) struct thread_info *ti = current_thread_info(); unsigned long ret = 0; - bool emulated = false; u32 work; if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) BUG_ON(regs != task_pt_regs(current)); - work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; - - if (unlikely(work & _TIF_SYSCALL_EMU)) - emulated = true; - - if ((emulated || (work & _TIF_SYSCALL_TRACE)) && - tracehook_report_syscall_entry(regs)) + if (unlikely(ptrace_syscall_enter(regs))) return -1L; - if (emulated) + work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; + if ((work & _TIF_SYSCALL_TRACE) && tracehook_report_syscall_entry(regs)) return -1L; #ifdef CONFIG_SECCOMP
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 syscall_trace_enter. Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP in syscall_slow_exit_work seems unnecessary. Let's remove the same. Cc: Andy Lutomirski <luto@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- arch/x86/entry/common.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)