diff mbox series

[v2,3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

Message ID 20190318104925.16600-4-sudeep.holla@arm.com
State New
Headers show
Series ptrace: consolidate PTRACE_SYSEMU handling and add support for arm64 | expand

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 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(-)

-- 
2.17.1

Comments

Oleg Nesterov March 18, 2019, 3:33 p.m. UTC | #1 
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
 	/*
Sudeep Holla April 30, 2019, 4:44 p.m. UTC | #2 
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
Andy Lutomirski April 30, 2019, 4:46 p.m. UTC | #3 
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
Sudeep Holla April 30, 2019, 5:09 p.m. UTC | #4 
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.

-- 
Regards,
Sudeep
Oleg Nesterov May 1, 2019, 3:57 p.m. UTC | #5 
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.
Sudeep Holla May 1, 2019, 4:51 p.m. UTC | #6 
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 mbox series

Patch

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