tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: compat linux futex locking problem
On Tue, Jun 03, 2008 at 12:13:30PM +0100, Andrew Doran wrote:
> On Mon, Jun 02, 2008 at 10:07:55AM +0200, Nicolas Joly wrote:
>
> > While testing some threaded apps under amd64 NTPL linux emulation, i
> > discovered a locking problem with our futex implementation.
> >
> > In most cases, futex_get() is called from linux_sys_futex() without
> > lock held, but there is one extra call in futex_wake() where the futex
> > lock is already hold ... leading to a panic.
> >
> > I'm not sure about the correct way to fix it (i'm not a locking
> > specialist) and wanted to ask.
> >
> > In the mean time, i checked the corresponding code in FreeBSD which
> > added an extra argument to futex_get to record if the lock is already
> > hold. But that does not seems right to me, especially because
> > futex_get() can sleep for quite some time due to a call to
> > kmem_zalloc(x, KM_SLEEP).
> >
> > Any hint/advice ?
>
> I think easiest way is to require that futex_get/put/wake/wait are always
> called holding futex_lock.
>
> The KM_SLEEP I'm not sure about. The lazy option is to hold futex_lock
> across the call; if the lock is not held then the operation needs to
> re-check conditions after releasing the lock.
The attached patch, which solves the locking problem, does move all
the locking in linux_sys_futex(), and all other functions check that
the lock is indeed hold.
> I'm not sure how futexes work, but is it possible that there is a race
> in that code? Presumably a FUTEX_WAKE can arrive before the thread that
> we want to wake has called into the kernel and done FUTEX_WAIT?
Possible, i sometimes see processes emulating threads blocked in
`futex' state ... I'll try to have a look.
Thanks.
--
Nicolas Joly
Biological Software and Databanks.
Institut Pasteur, Paris.
Index: sys/compat/linux/common/linux_futex.c
===================================================================
RCS file: /cvsroot/src/sys/compat/linux/common/linux_futex.c,v
retrieving revision 1.14
diff -u -p -r1.14 linux_futex.c
--- sys/compat/linux/common/linux_futex.c 5 Jun 2008 10:58:16 -0000
1.14
+++ sys/compat/linux/common/linux_futex.c 6 Jun 2008 09:46:51 -0000
@@ -71,6 +71,7 @@ static kmutex_t *futex_lock = NULL;
#define FUTEX_LOCK mutex_enter(futex_lock);
#define FUTEX_UNLOCK mutex_exit(futex_lock);
+#define FUTEX_OWNED mutex_owned(futex_lock)
#ifdef DEBUG_LINUX_FUTEX
#define FUTEXPRINTF(a) printf a
@@ -142,9 +143,11 @@ linux_sys_futex(struct lwp *l, const str
SCARG(uap, uaddr), val, (long long)timeout.tv_sec,
timeout.tv_nsec));
+ FUTEX_LOCK;
f = futex_get(SCARG(uap, uaddr));
ret = futex_sleep(f, timeout_hz);
futex_put(f);
+ FUTEX_UNLOCK;
FUTEXPRINTF(("FUTEX_WAIT %d.%d: uaddr = %p, "
"ret = %d\n", l->l_proc->p_pid, l->l_lid,
@@ -179,9 +182,13 @@ linux_sys_futex(struct lwp *l, const str
FUTEXPRINTF(("FUTEX_WAKE %d.%d: uaddr = %p, val = %d\n",
l->l_proc->p_pid, l->l_lid,
SCARG(uap, uaddr), SCARG(uap, val)));
+
+ FUTEX_LOCK;
f = futex_get(SCARG(uap, uaddr));
*retval = futex_wake(f, SCARG(uap, val), NULL);
futex_put(f);
+ FUTEX_UNLOCK;
+
break;
case LINUX_FUTEX_CMP_REQUEUE:
@@ -194,11 +201,15 @@ linux_sys_futex(struct lwp *l, const str
/* FALLTHROUGH */
case LINUX_FUTEX_REQUEUE:
+
+ FUTEX_LOCK;
f = futex_get(SCARG(uap, uaddr));
newf = futex_get(SCARG(uap, uaddr2));
*retval = futex_wake(f, SCARG(uap, val), newf);
futex_put(f);
futex_put(newf);
+ FUTEX_UNLOCK;
+
break;
case LINUX_FUTEX_FD:
@@ -218,15 +229,14 @@ futex_get(void *uaddr)
{
struct futex *f, *f2;
- FUTEX_LOCK;
+ KASSERT(FUTEX_OWNED);
+
LIST_FOREACH(f, &futex_list, f_list) {
if (f->f_uaddr == uaddr) {
f->f_refcount++;
- FUTEX_UNLOCK;
return f;
}
}
- FUTEX_UNLOCK;
/* Not found, create it */
f2 = kmem_zalloc(sizeof(*f2), KM_SLEEP);
@@ -234,17 +244,7 @@ futex_get(void *uaddr)
f2->f_refcount = 1;
TAILQ_INIT(&f2->f_waiting_proc);
- FUTEX_LOCK;
- LIST_FOREACH(f, &futex_list, f_list) {
- if (f->f_uaddr == uaddr) {
- f->f_refcount++;
- FUTEX_UNLOCK;
- kmem_free(f2, sizeof(*f2));
- return f;
- }
- }
LIST_INSERT_HEAD(&futex_list, f2, f_list);
- FUTEX_UNLOCK;
return f2;
}
@@ -252,14 +252,14 @@ futex_get(void *uaddr)
static void
futex_put(struct futex *f)
{
+ KASSERT(FUTEX_OWNED);
- FUTEX_LOCK;
f->f_refcount--;
if (f->f_refcount == 0) {
+ KASSERT(TAILQ_EMPTY(&f->f_waiting_proc));
LIST_REMOVE(f, f_list);
kmem_free(f, sizeof(*f));
}
- FUTEX_UNLOCK;
return;
}
@@ -270,14 +270,14 @@ futex_sleep(struct futex *f, unsigned in
struct waiting_proc *wp;
int ret;
+ KASSERT(FUTEX_OWNED);
+
wp = kmem_zalloc(sizeof(*wp), KM_SLEEP);
cv_init(&wp->wp_futex_cv, "futex");
- FUTEX_LOCK;
TAILQ_INSERT_TAIL(&f->f_waiting_proc, wp, wp_list);
ret = cv_timedwait_sig(&wp->wp_futex_cv, futex_lock, timeout);
TAILQ_REMOVE(&f->f_waiting_proc, wp, wp_list);
- FUTEX_UNLOCK;
if ((ret == 0) && (wp->wp_new_futex != NULL)) {
ret = futex_sleep(wp->wp_new_futex, timeout);
@@ -295,7 +295,8 @@ futex_wake(struct futex *f, int n, struc
struct waiting_proc *wp;
int count = 0;
- FUTEX_LOCK;
+ KASSERT(FUTEX_OWNED);
+
TAILQ_FOREACH(wp, &f->f_waiting_proc, wp_list) {
if (count <= n) {
cv_broadcast(&wp->wp_futex_cv);
@@ -308,7 +309,6 @@ futex_wake(struct futex *f, int n, struc
cv_broadcast(&wp->wp_futex_cv);
}
}
- FUTEX_UNLOCK;
return count;
}
Home |
Main Index |
Thread Index |
Old Index