On 08/18, Oleg Nesterov wrote:
>
> If engine is detached (has utrace_detached_ops), utrace_barrier(engine)
> spins until engine->ops becomes NULL. This is just wrong.
>
> Suppose that utrace_control(DETACH) returns -EINPROGRESS, now we should
> call utrace_barrier(). However, it is possible that -EINPROGRESS means
> we raced with sys_sleep(A_LOT) doing report_syscall_entry().
Yes,
> Or it could
> race with any callback, but another engine requested UTRACE_STOP.
No, this is not possible, utrace_stop() must never sleep with
just-detached-engine.
> if (unlikely(utrace->reap) || unlikely(!engine->ops) ||
> - unlikely(engine->ops == &utrace_detached_ops)) {
> + (!attached && unlikely(engine->ops == &utrace_detached_ops))) {
Oh, I confused attached and !attached. Noticed this only because
I hit the "new" problems with utrace during the testing ;)
Please find v2 below.
-------------------------------------------------------------------------------
[PATCH v2] utrace_barrier(detached_engine) must not sping without checking
->reporting
If engine is detached (has utrace_detached_ops), utrace_barrier(engine)
spins until engine->ops becomes NULL. This is just wrong.
Suppose that utrace_control(DETACH) returns -EINPROGRESS, now we should
call utrace_barrier(). However, it is possible that -EINPROGRESS means
we raced with, say, sys_sleep(A_LOT) doing report_syscall_entry().
If start_callback() notices utrace_detached_ops and sets report->detaches
everything is fine. But it is quite possible that this doesn't happen,
and in this case utrace_barrier() will spin "forever" waiting for the
next utrace_reset().
Change get_utrace_lock() to succeed if the caller is utrace_barrier()
and ops == &utrace_detached_ops. I do not see any reason why this case
should be special from utrace_barrier's pov. It can just check
->reporting and return 0 or do another iteration.
Note: we should also reconsider() utrace_barrier()->signal_pending()
check. Also, it is not clear why utrace_barrier() needs utrace->lock,
except to ensure it is safe to dereference target/utrace.
Signed-off-by: Oleg Nesterov <o...@redhat.com>
---
kernel/utrace.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
--- kstub/kernel/utrace.c~9_utrace_barrier_and_detached 2010-08-18
17:47:53.000000000 +0200
+++ kstub/kernel/utrace.c 2010-08-19 15:33:06.000000000 +0200
@@ -416,23 +416,21 @@ static struct utrace *get_utrace_lock(st
return ERR_PTR(-ESRCH);
}
- if (unlikely(engine->ops == &utrace_detached_ops)) {
+ if (attached && unlikely(engine->ops == &utrace_detached_ops)) {
rcu_read_unlock();
- return attached ? ERR_PTR(-ESRCH) : ERR_PTR(-ERESTARTSYS);
+ return ERR_PTR(-ESRCH);
}
utrace = task_utrace_struct(target);
spin_lock(&utrace->lock);
if (unlikely(utrace->reap) || unlikely(!engine->ops) ||
- unlikely(engine->ops == &utrace_detached_ops)) {
+ (attached && unlikely(engine->ops == &utrace_detached_ops))) {
/*
* By the time we got the utrace lock,
* it had been reaped or detached already.
*/
spin_unlock(&utrace->lock);
utrace = ERR_PTR(-ESRCH);
- if (!attached && engine->ops == &utrace_detached_ops)
- utrace = ERR_PTR(-ERESTARTSYS);
}
rcu_read_unlock();
@@ -1286,8 +1284,7 @@ int utrace_barrier(struct task_struct *t
utrace = get_utrace_lock(target, engine, false);
if (unlikely(IS_ERR(utrace))) {
ret = PTR_ERR(utrace);
- if (ret != -ERESTARTSYS)
- break;
+ break;
} else {
/*
* All engine state changes are done while
