Messages in this thread Patch in this message | | | Date | Mon, 18 Mar 2019 16:33:22 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook |
| |
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 /*
| |