Dear Roland, dear utrace developers,
This is an updated patch. It solves the race condition + it gives a quick (a
bit dirty)
solution to issues 3&4.
3- Nesting, is it really useful to run all the reports in a row and
(eventually) stop and the end waiting for all the engines?
The patch waits for each engine to resume before notifying the next registered
engine.
4- report_syscall_entry engines evaluation order should be reversed
REPORT macros have an extra "reverse" argument. The macros append this string
to the
list_for_each_entry_safe function name. All the macro calls skip this argument
except
the one in report_syscall_entry where it is set to _reverse.
With this patch it is possible to run nested kmview machines and ptrace works
inside
the virtual machines.
This patch is "a bit dirty" because variables and sections of code needed to
count and test
the stopped engines are useless here: a task can be kept stopped for at most
one engine at
a time.
This patch is a proof-of concept to show what I meant in my previous message.
For what concerns 1&2 (not included in this patch):
1- Virtual Machines may need to change the system call
THis is just to simplify the implementation of arch. independent virtual
machine.
I have kept the definition of missing functions in the kmview module code.
2- UTRACE_SYSCALL_ABORT: is it really useful as a return value for
report_syscall_entry?
It is useless for kmview as the decision of aborting the system call is taken
while
the process is stopped, I am currently setting the syscall number to -1 to skip
the syscall.
For the sake of completeness there is another way to implement the partial
virtual machine
stuff by introducing another "quiescence" state inside the report upcalls.
I mean: when utrace calls a report function (say for example
report_syscall_entry), the function
in the module puts the process in a stopped state (maybe its TASK_TRACED and
calls the schedule).
>From utrace's point of view the report function does not return until all the
>changes in
the task state have been completed and the decision
UTRACE_RESUME/UTRACE_SYSCALL_ABORT has been taken.
In this way UTRACE_STOP is never used because the module has to implement
another feature
similar to UTRACE_STOP on its own. So what is UTRACE_STOP for?
ciao
renzo
----
--- linux-2.6.29-rc4-utrace/kernel/utrace.c.mcgrath 2009-02-13
18:28:25.000000000 +0100
+++ linux-2.6.29-rc4-utrace/kernel/utrace.c 2009-02-14 09:17:31.000000000
+0100
@@ -491,6 +491,13 @@
#define DEAD_FLAGS_MASK (UTRACE_EVENT(REAP))
#define LIVE_FLAGS_MASK (~0UL)
+static void mark_engine_wants_stop(struct utrace_attached_engine *engine);
+static void clear_engine_wants_stop(struct utrace_attached_engine *engine);
+static bool engine_wants_stop(struct utrace_attached_engine *engine);
+static void mark_engine_wants_resume(struct utrace_attached_engine *engine);
+static void clear_engine_wants_resume(struct utrace_attached_engine *engine);
+static bool engine_wants_resume(struct utrace_attached_engine *engine);
+
/*
* Perform %UTRACE_STOP, i.e. block in TASK_TRACED until woken up.
* @task == current, @utrace == current->utrace, which is not locked.
@@ -500,6 +507,7 @@
static bool utrace_stop(struct task_struct *task, struct utrace *utrace)
{
bool killed;
+ struct utrace_attached_engine *engine, *next;
/*
* @utrace->stopped is the flag that says we are safely
@@ -521,6 +529,23 @@
return true;
}
+ /* final check: is really needed to stop? */
+ list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
+ if ((engine->ops != &utrace_detached_ops) &&
engine_wants_stop(engine)) {
+ if (engine_wants_resume(engine)) {
+ clear_engine_wants_stop(engine);
+ clear_engine_wants_resume(engine);
+ }
+ else
+ utrace->stopped = 1;
+ }
+ }
+ if (unlikely(!utrace->stopped)) {
+ spin_unlock_irq(&task->sighand->siglock);
+ spin_unlock(&utrace->lock);
+ return false;
+ }
+
utrace->stopped = 1;
__set_current_state(TASK_TRACED);
@@ -784,6 +809,7 @@
* to record whether the engine is keeping the target thread stopped.
*/
#define ENGINE_STOP (1UL << _UTRACE_NEVENTS)
+#define ENGINE_RESUME (1UL << (_UTRACE_NEVENTS+1))
static void mark_engine_wants_stop(struct utrace_attached_engine *engine)
{
@@ -800,6 +826,21 @@
return (engine->flags & ENGINE_STOP) != 0;
}
+static void mark_engine_wants_resume(struct utrace_attached_engine *engine)
+{
+ engine->flags |= ENGINE_RESUME;
+}
+
+static void clear_engine_wants_resume(struct utrace_attached_engine *engine)
+{
+ engine->flags &= ~ENGINE_RESUME;
+}
+
+static bool engine_wants_resume(struct utrace_attached_engine *engine)
+{
+ return (engine->flags & ENGINE_RESUME) != 0;
+}
+
/**
* utrace_set_events - choose which event reports a tracing engine gets
* @target: thread to affect
@@ -1050,6 +1091,10 @@
list_move(&engine->entry, &detached);
} else {
flags |= engine->flags | UTRACE_EVENT(REAP);
+ if (engine_wants_resume(engine)) {
+ clear_engine_wants_stop(engine);
+ clear_engine_wants_resume(engine);
+ }
wake = wake && !engine_wants_stop(engine);
}
}
@@ -1282,6 +1327,7 @@
* There might not be another report before it just
* resumes, so make sure single-step is not left set.
*/
+ mark_engine_wants_resume(engine);
if (likely(resume))
user_disable_single_step(target);
break;
@@ -1497,6 +1543,7 @@
static bool finish_callback(struct utrace *utrace,
struct utrace_report *report,
struct utrace_attached_engine *engine,
+ struct task_struct *task,
u32 ret)
{
enum utrace_resume_action action = utrace_resume_action(ret);
@@ -1529,6 +1576,7 @@
spin_lock(&utrace->lock);
mark_engine_wants_stop(engine);
spin_unlock(&utrace->lock);
+ utrace_stop(task, utrace);
}
} else if (engine_wants_stop(engine)) {
spin_lock(&utrace->lock);
@@ -1567,7 +1615,7 @@
ops = engine->ops;
if (want & UTRACE_EVENT(QUIESCE)) {
- if (finish_callback(utrace, report, engine,
+ if (finish_callback(utrace, report, engine, task,
(*ops->report_quiesce)(report->action,
engine, task,
event)))
@@ -1596,25 +1644,25 @@
* @callback is the name of the member in the ops vector, and remaining
* args are the extras it takes after the standard three args.
*/
-#define REPORT(task, utrace, report, event, callback, ...) \
+#define REPORT(reverse, task, utrace, report, event, callback, ...)
\
do { \
start_report(utrace); \
- REPORT_CALLBACKS(task, utrace, report, event, callback, \
+ REPORT_CALLBACKS(reverse, task, utrace, report, event,
callback, \
(report)->action, engine, current, \
## __VA_ARGS__); \
finish_report(report, task, utrace); \
} while (0)
-#define REPORT_CALLBACKS(task, utrace, report, event, callback, ...) \
+#define REPORT_CALLBACKS(reverse, task, utrace, report, event, callback, ...)
\
do { \
struct utrace_attached_engine *engine, *next; \
const struct utrace_engine_ops *ops; \
- list_for_each_entry_safe(engine, next, \
+ list_for_each_entry_safe ## reverse(engine, next,
\
&utrace->attached, entry) { \
ops = start_callback(utrace, report, engine, task, \
event); \
if (!ops) \
continue; \
- finish_callback(utrace, report, engine, \
+ finish_callback(utrace, report, engine, task,
\
(*ops->callback)(__VA_ARGS__)); \
} \
} while (0)
@@ -1629,7 +1677,7 @@
struct utrace *utrace = task_utrace_struct(task);
INIT_REPORT(report);
- REPORT(task, utrace, &report, UTRACE_EVENT(EXEC),
+ REPORT(,task, utrace, &report, UTRACE_EVENT(EXEC),
report_exec, fmt, bprm, regs);
}
@@ -1644,7 +1692,7 @@
INIT_REPORT(report);
start_report(utrace);
- REPORT_CALLBACKS(task, utrace, &report, UTRACE_EVENT(SYSCALL_ENTRY),
+ REPORT_CALLBACKS(_reverse,task, utrace, &report,
UTRACE_EVENT(SYSCALL_ENTRY),
report_syscall_entry, report.result | report.action,
engine, current, regs);
finish_report(&report, task, utrace);
@@ -1686,7 +1734,7 @@
struct utrace *utrace = task_utrace_struct(task);
INIT_REPORT(report);
- REPORT(task, utrace, &report, UTRACE_EVENT(SYSCALL_EXIT),
+ REPORT(,task, utrace, &report, UTRACE_EVENT(SYSCALL_EXIT),
report_syscall_exit, regs);
}
@@ -1711,7 +1759,7 @@
start_report(utrace);
utrace->u.live.cloning = child;
- REPORT_CALLBACKS(task, utrace, &report,
+ REPORT_CALLBACKS(,task, utrace, &report,
UTRACE_EVENT(CLONE), report_clone,
report.action, engine, task, clone_flags, child);
@@ -1791,7 +1839,7 @@
spin_unlock(&utrace->lock);
rcu_read_unlock();
- REPORT(task, utrace, &report, UTRACE_EVENT(JCTL),
+ REPORT(,task, utrace, &report, UTRACE_EVENT(JCTL),
report_jctl, was_stopped ? CLD_STOPPED : CLD_CONTINUED, what);
if (was_stopped && !task_is_stopped(task)) {
@@ -1828,7 +1876,7 @@
INIT_REPORT(report);
long orig_code = *exit_code;
- REPORT(task, utrace, &report, UTRACE_EVENT(EXIT),
+ REPORT(,task, utrace, &report, UTRACE_EVENT(EXIT),
report_exit, orig_code, exit_code);
if (report.action == UTRACE_STOP)
@@ -1867,7 +1915,7 @@
utrace->interrupt = 0;
spin_unlock(&utrace->lock);
- REPORT_CALLBACKS(task, utrace, &report, UTRACE_EVENT(DEATH),
+ REPORT_CALLBACKS(,task, utrace, &report, UTRACE_EVENT(DEATH),
report_death, engine, task, group_dead, signal);
spin_lock(&utrace->lock);
@@ -2259,7 +2307,7 @@
break;
}
- finish_callback(utrace, &report, engine, ret);
+ finish_callback(utrace, &report, engine, task, ret);
}
/*
