Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Wed, 23 May 2007 15:51:59 +0400 | From | Alexey Kuznetsov <> | Subject | Re: [RFC][PATCH] muptiple bugs in PI futexes |
| |
Hello!
> #2 crash be explained via any of the bugs you fixed? (i.e. memory > corruption?)
Yes, I found the reason, it is really fixed by taking tasklist_lock. This happens after task struct with not cleared pi_state_list is freed and the list of futex_pi_state's is corrupted.
Meanwhile... two more bugs were found.
The first chunk: results in self-inflicted deadlock inside glibc. Sometimes futex_lock_pi returns -ESRCH, when it is not expected and glibc enters to for(;;) sleep() to simulate deadlock. This problem is quite obvious and I think the patch is right. Though it looks like each "if" in futex_lock_pi() got some stupid special case "else if". :-)
The second chunk: sometimes futex_lock_pi() returns -EDEADLK, when nobody has the lock. The reason is also obvious (see comment in the patch), but correct fix is far beyond my comprehension. I guess someone already saw this, the chunk:
if (rt_mutex_trylock(&q.pi_state->pi_mutex)) ret = 0;
is obviously from the same opera. But it does not work, because the rtmutex is really taken at this point: wake_futex_pi() of previous owner reassigned it to us. My fix works. But it looks very stupid. I would think about removal of shift of ownership in wake_futex_pi() and making all the work in context of process taking lock.
Both bugs show up when running glibc's tst-robustpi8 long enough.
Yes, all this about pre May 8 futexes. Seems, updates did not change anything in logic, but I am not sure.
--- kernel/futex.c.intermediate 2007-05-23 14:48:27.000000000 +0400 +++ kernel/futex.c 2007-05-23 14:58:06.000000000 +0400 @@ -1244,8 +1244,21 @@ static int futex_lock_pi(u32 __user *uad if (unlikely(curval != uval)) goto retry_locked; ret = 0; - } - goto out_unlock_release_sem; + } else if (ret == -ESRCH) { + /* Process could exit right now, so that robust list + * was processed after we got uval. Retry. */ + pagefault_disable(); + curval = futex_atomic_cmpxchg_inatomic(uaddr, + uval, uval); + pagefault_enable(); + if (unlikely(curval == -EFAULT)) + goto uaddr_faulted; + if (unlikely(curval != uval)) { + printk("RETRY %x %x %x\n", current->pid, uval, curval); + goto retry_locked; + } + } + goto out_unlock_release_sem; } /* @@ -1361,6 +1374,22 @@ static int futex_lock_pi(u32 __user *uad if (ret && q.pi_state->owner == curr) { if (rt_mutex_trylock(&q.pi_state->pi_mutex)) ret = 0; + /* Holy crap... Now futex lock returns -EDEADLK + * sometimes, because ownership was passed to us while + * unlock of previous owner. Who wrote this? + * Please, fix this correctly. For now: + */ + if (ret == -EDEADLK) { + pagefault_disable(); + uval = futex_atomic_cmpxchg_inatomic(uaddr, + 0, 0); + pagefault_enable(); + if (uval != -EFAULT && + (uval&FUTEX_TID_MASK) == current->pid) { + printk("ALERT1 %x\n", uval); + ret = 0; + } + } } /* Unqueue and drop the lock */ unqueue_me_pi(&q, hb); - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| ![\](/images/icornerr.gif) |