2009-03-27 09:34:27

by Metzger, Markus T

[permalink] [raw]
Subject: [patch 1/14] x86, ptrace: add arch_ptrace_report_exit

Add an arch_ variant to report a task exit.
Move the ptrace report exit code from tracehook.h to ptrace.h.


Signed-off-by: Markus Metzger <[email protected]>
---

Index: git-tip/include/linux/ptrace.h
===================================================================
--- git-tip.orig/include/linux/ptrace.h 2009-03-23 10:45:30.000000000 +0100
+++ git-tip/include/linux/ptrace.h 2009-03-23 11:31:21.000000000 +0100
@@ -335,6 +335,23 @@ static inline void user_enable_block_ste
#define arch_ptrace_fork(child, clone_flags) do { } while (0)
#endif

+#ifndef arch_ptrace_report_exit
+/**
+ * arch_ptrace_report_exit - Do machine specific work for exiting ptraced tasks
+ * @exit_code: current->exit_code value
+ *
+ * This is called early from do_exit() with no locks held after we notified the
+ * ptracer.
+ */
+#define arch_ptrace_report_exit(exit_code) do { } while (0)
+#endif
+
+static inline void ptrace_report_exit(long *exit_code)
+{
+ ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
+ arch_ptrace_report_exit(*exit_code);
+}
+
extern int task_current_syscall(struct task_struct *target, long *callno,
unsigned long args[6], unsigned int maxargs,
unsigned long *sp, unsigned long *pc);
Index: git-tip/include/linux/tracehook.h
===================================================================
--- git-tip.orig/include/linux/tracehook.h 2009-03-23 10:45:30.000000000 +0100
+++ git-tip/include/linux/tracehook.h 2009-03-23 11:31:21.000000000 +0100
@@ -211,7 +211,7 @@ static inline void tracehook_report_exec
*/
static inline void tracehook_report_exit(long *exit_code)
{
- ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
+ ptrace_report_exit(exit_code);
}

/**
---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.


2009-03-27 14:23:26

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [patch 1/14] x86, ptrace: add arch_ptrace_report_exit

On 03/27, Markus Metzger wrote:
>
> --- git-tip.orig/include/linux/ptrace.h 2009-03-23 10:45:30.000000000 +0100
> +++ git-tip/include/linux/ptrace.h 2009-03-23 11:31:21.000000000 +0100
> @@ -335,6 +335,23 @@ static inline void user_enable_block_ste
> #define arch_ptrace_fork(child, clone_flags) do { } while (0)
> #endif
>
> +#ifndef arch_ptrace_report_exit
> +/**
> + * arch_ptrace_report_exit - Do machine specific work for exiting ptraced tasks
> + * @exit_code: current->exit_code value
> + *
> + * This is called early from do_exit() with no locks held after we notified the
> + * ptracer.
> + */
> +#define arch_ptrace_report_exit(exit_code) do { } while (0)
> +#endif
> +
> +static inline void ptrace_report_exit(long *exit_code)
> +{
> + ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
> + arch_ptrace_report_exit(*exit_code);
> +}
> +
> extern int task_current_syscall(struct task_struct *target, long *callno,
> unsigned long args[6], unsigned int maxargs,
> unsigned long *sp, unsigned long *pc);
> Index: git-tip/include/linux/tracehook.h
> ===================================================================
> --- git-tip.orig/include/linux/tracehook.h 2009-03-23 10:45:30.000000000 +0100
> +++ git-tip/include/linux/tracehook.h 2009-03-23 11:31:21.000000000 +0100
> @@ -211,7 +211,7 @@ static inline void tracehook_report_exec
> */
> static inline void tracehook_report_exit(long *exit_code)
> {
> - ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
> + ptrace_report_exit(exit_code);
> }

This needs Rolan'd review.

But I'd say this has nothing to do with tracehooks. And why do
you pass *exit_code to arch_ptrace_report_exit() ?

Just add arch_ptrace_report_exit(void) into do_exit() ?

>From the 3/14 patch:

#define arch_ptrace_report_exit(code) x86_ptrace_report_exit(code)

void x86_ptrace_report_exit(long exit_code)
{
ptrace_bts_exit();
}

This is a bit strange. Why do we need 2 functions, ptrace_bts_exit() and
x86_ptrace_report_exit() which just calls the first one?

Oleg.

2009-03-27 15:13:11

by Metzger, Markus T

[permalink] [raw]
Subject: RE: [patch 1/14] x86, ptrace: add arch_ptrace_report_exit

>-----Original Message-----
>From: Oleg Nesterov [mailto:[email protected]]
>Sent: Friday, March 27, 2009 3:20 PM
>To: Metzger, Markus T
>Cc: [email protected]; [email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]; Villacis, Juan;
>[email protected]
>Subject: Re: [patch 1/14] x86, ptrace: add arch_ptrace_report_exit
>
>On 03/27, Markus Metzger wrote:
>>
>> --- git-tip.orig/include/linux/ptrace.h 2009-03-23 10:45:30.000000000 +0100
>> +++ git-tip/include/linux/ptrace.h 2009-03-23 11:31:21.000000000 +0100
>> @@ -335,6 +335,23 @@ static inline void user_enable_block_ste
>> #define arch_ptrace_fork(child, clone_flags) do { } while (0)
>> #endif
>>
>> +#ifndef arch_ptrace_report_exit
>> +/**
>> + * arch_ptrace_report_exit - Do machine specific work for exiting ptraced tasks
>> + * @exit_code: current->exit_code value
>> + *
>> + * This is called early from do_exit() with no locks held after we notified the
>> + * ptracer.
>> + */
>> +#define arch_ptrace_report_exit(exit_code) do { } while (0)
>> +#endif
>> +
>> +static inline void ptrace_report_exit(long *exit_code)
>> +{
>> + ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
>> + arch_ptrace_report_exit(*exit_code);
>> +}
>> +
>> extern int task_current_syscall(struct task_struct *target, long *callno,
>> unsigned long args[6], unsigned int maxargs,
>> unsigned long *sp, unsigned long *pc);
>> Index: git-tip/include/linux/tracehook.h
>> ===================================================================
>> --- git-tip.orig/include/linux/tracehook.h 2009-03-23 10:45:30.000000000 +0100
>> +++ git-tip/include/linux/tracehook.h 2009-03-23 11:31:21.000000000 +0100
>> @@ -211,7 +211,7 @@ static inline void tracehook_report_exec
>> */
>> static inline void tracehook_report_exit(long *exit_code)
>> {
>> - ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
>> + ptrace_report_exit(exit_code);
>> }
>
>This needs Rolan'd review.
>
>But I'd say this has nothing to do with tracehooks. And why do
>you pass *exit_code to arch_ptrace_report_exit() ?
>
>Just add arch_ptrace_report_exit(void) into do_exit() ?
>
>From the 3/14 patch:
>
> #define arch_ptrace_report_exit(code) x86_ptrace_report_exit(code)
>
> void x86_ptrace_report_exit(long exit_code)
> {
> ptrace_bts_exit();
> }
>
>This is a bit strange. Why do we need 2 functions, ptrace_bts_exit() and
>x86_ptrace_report_exit() which just calls the first one?

I did not want to take any shortcuts. I try to maintain the structure
general_function()->ptrace_report()->arch_ptrace_report().

Recently, tracehook_report_whatever() calls were added which either do the
ptrace work directly or call a ptrace function. I try to use those calls, where possible.

It does not clutter the general code with x86 specific, or worse x86 branch trace
recording specific calls.


If people don't like these ptrace_whatever() and arch_ptrace_whatever() calls I add,
I can remove them and add my calls directly where I need them.

regards,
markus.

---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

2009-03-27 17:11:35

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [patch 1/14] x86, ptrace: add arch_ptrace_report_exit

On 03/27, Metzger, Markus T wrote:
>
> >-----Original Message-----
> >From: Oleg Nesterov [mailto:[email protected]]
> >
> >This needs Rolan'd review.
> >
> >But I'd say this has nothing to do with tracehooks. And why do
> >you pass *exit_code to arch_ptrace_report_exit() ?
> >
> >Just add arch_ptrace_report_exit(void) into do_exit() ?
> >
> >From the 3/14 patch:
> >
> > #define arch_ptrace_report_exit(code) x86_ptrace_report_exit(code)
> >
> > void x86_ptrace_report_exit(long exit_code)
> > {
> > ptrace_bts_exit();
> > }
> >
> >This is a bit strange. Why do we need 2 functions, ptrace_bts_exit() and
> >x86_ptrace_report_exit() which just calls the first one?
>
> I did not want to take any shortcuts. I try to maintain the structure
> general_function()->ptrace_report()->arch_ptrace_report().

I see. And honestly, this doesn't look good to me. Yes, this is subjective.

Say, Regardless of CONFIG_X86_PTRACE_BTS we have the non-empty and non-inline
x86_ptrace_untrace() which just calls ptrace_bts_untrace(). And ptrace_bts_untrace()
depends on CONFIG_X86_PTRACE_BTS.

But this is minor.

> Recently, tracehook_report_whatever() calls were added which either do the
> ptrace work directly or call a ptrace function. I try to use those calls, where possible.

Up to Roland, but I still think tracehook_report_whatever() is not the
good place for this stuff. And tracehooks will be changed soon by utrace.

In any case I don't understand why you added yet another helper, you could
just add arch_ptrace_report_exit() into tracehook_report_exit().

Oleg.

2009-03-27 17:37:50

by Markus Metzger

[permalink] [raw]
Subject: Re: [patch 1/14] x86, ptrace: add arch_ptrace_report_exit

On Fri, 2009-03-27 at 18:07 +0100, Oleg Nesterov wrote:
> On 03/27, Metzger, Markus T wrote:
> >
> > >-----Original Message-----
> > >From: Oleg Nesterov [mailto:[email protected]]
> > >
> > >This needs Rolan'd review.
> > >
> > >But I'd say this has nothing to do with tracehooks. And why do
> > >you pass *exit_code to arch_ptrace_report_exit() ?
> > >
> > >Just add arch_ptrace_report_exit(void) into do_exit() ?
> > >
> > >From the 3/14 patch:
> > >
> > > #define arch_ptrace_report_exit(code) x86_ptrace_report_exit(code)
> > >
> > > void x86_ptrace_report_exit(long exit_code)
> > > {
> > > ptrace_bts_exit();
> > > }
> > >
> > >This is a bit strange. Why do we need 2 functions, ptrace_bts_exit() and
> > >x86_ptrace_report_exit() which just calls the first one?
> >
> > I did not want to take any shortcuts. I try to maintain the structure
> > general_function()->ptrace_report()->arch_ptrace_report().
>
> I see. And honestly, this doesn't look good to me. Yes, this is subjective.
>
> Say, Regardless of CONFIG_X86_PTRACE_BTS we have the non-empty and non-inline
> x86_ptrace_untrace() which just calls ptrace_bts_untrace(). And ptrace_bts_untrace()
> depends on CONFIG_X86_PTRACE_BTS.
>
> But this is minor.
>
> > Recently, tracehook_report_whatever() calls were added which either do the
> > ptrace work directly or call a ptrace function. I try to use those calls, where possible.
>
> Up to Roland, but I still think tracehook_report_whatever() is not the
> good place for this stuff. And tracehooks will be changed soon by utrace.
>
> In any case I don't understand why you added yet another helper, you could
> just add arch_ptrace_report_exit() into tracehook_report_exit().

Fine with me.
I did not want to add some arch_ptrace stuff in tracehook, but I can
change that.

regards,
markus.