Re: [PATCH] fix mark_engine_detached() vs start_callback() race
On 08/19, Roland McGrath wrote: I think in the longer term mark_engine_detached() should not change engine-flags at all but add QUIESCE to -utrace_flags. However, this breaks utrace_maybe_reap(reap = true) and we should avoid the race with finish_callback() which clears -reporting after report_quiesce(). What's the benefit to adding QUIESCE? If any utrace code gets entered at all, then any callback run will be able to do the special-case ops check for detached engines. Well yes, but otoh it could be the last engine. We shouldn't miss, say, start_callback() from utrace_resume() (although currently -spurious helps). Anyway, this needs more thinking, and probably you are right and QUIESCE is not needed. Oleg.
Re: [PATCH] fix mark_engine_detached() vs start_callback() race
This is the minimal temporary ugly fix for now, we should certainly cleanup and simplify this logic. The barriers in mark_engine_detached() and in start_callback() can't help and should be removed. If we ignore utrace_get_signal() we do not even need utrace_detached_quiesce(), start_callback() could just do Agreed. I applied the patch for now. I think in the longer term mark_engine_detached() should not change engine-flags at all but add QUIESCE to -utrace_flags. However, this breaks utrace_maybe_reap(reap = true) and we should avoid the race with finish_callback() which clears -reporting after report_quiesce(). What's the benefit to adding QUIESCE? If any utrace code gets entered at all, then any callback run will be able to do the special-case ops check for detached engines. A bit off-topic, but I don't think finish_callback() should check engine-ops == utrace_detached_ops before return. Instead we should change finish_callback_report() to return the boolean. We shouldn't return true without setting report-detaches. Ok. Thanks, Roland
[PATCH] fix mark_engine_detached() vs start_callback() race
Suppose that engine-flags == UTRACE_EVENT(EXEC), QUIESCE bit is not set.
1. start_callback() reads want = engine-flags (== EXEC)
2. mark_engine_detached() sets engine-ops = utrace_detached_ops
3. start_callback() gets ops = utrace_detached_ops
After that start_callback() skips if (want UTRACE_EVENT(QUIESCE))
block and returns utrace_detached_ops, then -report_exec == NULL
will be called.
This is the minimal temporary ugly fix for now, we should certainly
cleanup and simplify this logic. The barriers in mark_engine_detached()
and in start_callback() can't help and should be removed. If we ignore
utrace_get_signal() we do not even need utrace_detached_quiesce(),
start_callback() could just do
ops = engine-ops;
if (ops == utrace_detached_ops) {
report-detaches = true;
return NULL;
}
I think in the longer term mark_engine_detached() should not change
engine-flags at all but add QUIESCE to -utrace_flags. However, this
breaks utrace_maybe_reap(reap = true) and we should avoid the race
with finish_callback() which clears -reporting after report_quiesce().
A bit off-topic, but I don't think finish_callback() should check
engine-ops == utrace_detached_ops before return. Instead we should
change finish_callback_report() to return the boolean. We shouldn't
return true without setting report-detaches.
Signed-off-by: Oleg Nesterov o...@redhat.com
---
kernel/utrace.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- kstub/kernel/utrace.c~8_fix_mark_detached_without_quiesce 2010-08-18
16:46:08.0 +0200
+++ kstub/kernel/utrace.c 2010-08-18 17:47:53.0 +0200
@@ -1522,7 +1522,7 @@ static const struct utrace_engine_ops *s
smp_rmb();
ops = engine-ops;
- if (want UTRACE_EVENT(QUIESCE)) {
+ if ((want UTRACE_EVENT(QUIESCE)) || ops == utrace_detached_ops) {
if (finish_callback(task, utrace, report, engine,
(*ops-report_quiesce)(report-action,
engine, event)))
