Re: [RFC][PATCH] muptiple bugs in PI futexes

[Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>]

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);