Author: kib
Date: Thu Jun 13 09:33:22 2013
New Revision: 251684
URL: http://svnweb.freebsd.org/changeset/base/251684
Log:
Fix two issues with the spin loops in the umtx(2) implementation.
- When looping, check for the pending suspension. Otherwise, other
usermode thread which races with the looping one, could try to
prevent the process from stopping or exiting.
- Add missed checks for the faults from casuword*(). The code is
structured in a way which makes the loops exit if the specified
address is invalid, since both fuword() and casuword() return -1 on
the fault. But if the address is mapped readonly, the typical value
read by fuword() is different from -1, while casuword() returns -1.
Absent the checks for casuword() faults, this is interpreted as the
race with other thread and causes non-interruptible spinning in the
kernel.
Reported and tested by: pho
Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks
Modified:
head/sys/kern/kern_umtx.c
Modified: head/sys/kern/kern_umtx.c
==============================================================================
--- head/sys/kern/kern_umtx.c Thu Jun 13 08:34:23 2013 (r251683)
+++ head/sys/kern/kern_umtx.c Thu Jun 13 09:33:22 2013 (r251684)
@@ -628,6 +628,32 @@ umtxq_count_pi(struct umtx_key *key, str
return (0);
}
+static int
+umtxq_check_susp(struct thread *td)
+{
+ struct proc *p;
+ int error;
+
+ /*
+ * The check for TDF_NEEDSUSPCHK is racy, but it is enough to
+ * eventually break the lockstep loop.
+ */
+ if ((td->td_flags & TDF_NEEDSUSPCHK) == 0)
+ return (0);
+ error = 0;
+ p = td->td_proc;
+ PROC_LOCK(p);
+ if (P_SHOULDSTOP(p) ||
+ ((p->p_flag & P_TRACED) && (td->td_dbgflags & TDB_SUSPEND))) {
+ if (p->p_flag & P_SINGLE_EXIT)
+ error = EINTR;
+ else
+ error = ERESTART;
+ }
+ PROC_UNLOCK(p);
+ return (error);
+}
+
/*
* Wake up threads waiting on an userland object.
*/
@@ -858,6 +884,10 @@ do_lock_umtx(struct thread *td, struct u
if (owner == -1)
return (EFAULT);
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
+
/* If this failed the lock has changed, restart. */
continue;
}
@@ -908,6 +938,9 @@ do_lock_umtx(struct thread *td, struct u
umtxq_remove(uq);
umtxq_unlock(&uq->uq_key);
umtx_key_release(&uq->uq_key);
+
+ if (error == 0)
+ error = umtxq_check_susp(td);
}
if (timeout == NULL) {
@@ -1032,6 +1065,10 @@ do_lock_umtx32(struct thread *td, uint32
if (owner == -1)
return (EFAULT);
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
+
/* If this failed the lock has changed, restart. */
continue;
}
@@ -1082,6 +1119,9 @@ do_lock_umtx32(struct thread *td, uint32
umtxq_remove(uq);
umtxq_unlock(&uq->uq_key);
umtx_key_release(&uq->uq_key);
+
+ if (error == 0)
+ error = umtxq_check_susp(td);
}
if (timeout == NULL) {
@@ -1272,6 +1312,10 @@ do_lock_normal(struct thread *td, struct
if (owner == -1)
return (EFAULT);
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ return (error);
+
/* If this failed the lock has changed,
restart. */
continue;
}
@@ -1331,6 +1375,9 @@ do_lock_normal(struct thread *td, struct
umtxq_remove(uq);
umtxq_unlock(&uq->uq_key);
umtx_key_release(&uq->uq_key);
+
+ if (error == 0)
+ error = umtxq_check_susp(td);
}
return (0);
@@ -1487,6 +1534,11 @@ do_wake2_umutex(struct thread *td, struc
if (old == owner)
break;
owner = old;
+ if (old == -1)
+ break;
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
}
} else if (count == 1) {
owner = fuword32(__DEVOLATILE(uint32_t *, &m->m_owner));
@@ -1497,6 +1549,11 @@ do_wake2_umutex(struct thread *td, struc
if (old == owner)
break;
owner = old;
+ if (old == -1)
+ break;
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
}
}
umtxq_lock(&key);
@@ -1961,6 +2018,10 @@ do_lock_pi(struct thread *td, struct umu
break;
}
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
+
/* If this failed the lock has changed, restart. */
continue;
}
@@ -2017,6 +2078,10 @@ do_lock_pi(struct thread *td, struct umu
umtxq_unbusy(&uq->uq_key);
umtxq_unlock(&uq->uq_key);
}
+
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
}
umtxq_lock(&uq->uq_key);
@@ -2663,10 +2728,17 @@ do_rw_rdlock(struct thread *td, struct u
return (EAGAIN);
}
oldstate = casuword32(&rwlock->rw_state, state, state +
1);
+ if (oldstate == -1) {
+ umtx_key_release(&uq->uq_key);
+ return (EFAULT);
+ }
if (oldstate == state) {
umtx_key_release(&uq->uq_key);
return (0);
}
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
state = oldstate;
}
@@ -2687,9 +2759,22 @@ do_rw_rdlock(struct thread *td, struct u
/* set read contention bit */
while ((state & wrflags) && !(state & URWLOCK_READ_WAITERS)) {
oldstate = casuword32(&rwlock->rw_state, state, state |
URWLOCK_READ_WAITERS);
+ if (oldstate == -1) {
+ error = EFAULT;
+ break;
+ }
if (oldstate == state)
goto sleep;
state = oldstate;
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
+ }
+ if (error != 0) {
+ umtxq_lock(&uq->uq_key);
+ umtxq_unbusy(&uq->uq_key);
+ umtxq_unlock(&uq->uq_key);
+ break;
}
/* state is changed while setting flags, restart */
@@ -2697,6 +2782,9 @@ do_rw_rdlock(struct thread *td, struct u
umtxq_lock(&uq->uq_key);
umtxq_unbusy(&uq->uq_key);
umtxq_unlock(&uq->uq_key);
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
continue;
}
@@ -2729,15 +2817,24 @@ sleep:
for (;;) {
oldstate = casuword32(&rwlock->rw_state, state,
state & ~URWLOCK_READ_WAITERS);
+ if (oldstate == -1) {
+ error = EFAULT;
+ break;
+ }
if (oldstate == state)
break;
state = oldstate;
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
}
}
umtxq_lock(&uq->uq_key);
umtxq_unbusy(&uq->uq_key);
umtxq_unlock(&uq->uq_key);
+ if (error != 0)
+ break;
}
umtx_key_release(&uq->uq_key);
if (error == ERESTART)
@@ -2770,11 +2867,18 @@ do_rw_wrlock(struct thread *td, struct u
state = fuword32(__DEVOLATILE(int32_t *, &rwlock->rw_state));
while (!(state & URWLOCK_WRITE_OWNER) &&
URWLOCK_READER_COUNT(state) == 0) {
oldstate = casuword32(&rwlock->rw_state, state, state |
URWLOCK_WRITE_OWNER);
+ if (oldstate == -1) {
+ umtx_key_release(&uq->uq_key);
+ return (EFAULT);
+ }
if (oldstate == state) {
umtx_key_release(&uq->uq_key);
return (0);
}
state = oldstate;
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
}
if (error) {
@@ -2804,15 +2908,31 @@ do_rw_wrlock(struct thread *td, struct u
while (((state & URWLOCK_WRITE_OWNER) ||
URWLOCK_READER_COUNT(state) != 0) &&
(state & URWLOCK_WRITE_WAITERS) == 0) {
oldstate = casuword32(&rwlock->rw_state, state, state |
URWLOCK_WRITE_WAITERS);
+ if (oldstate == -1) {
+ error = EFAULT;
+ break;
+ }
if (oldstate == state)
goto sleep;
state = oldstate;
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
+ }
+ if (error != 0) {
+ umtxq_lock(&uq->uq_key);
+ umtxq_unbusy(&uq->uq_key);
+ umtxq_unlock(&uq->uq_key);
+ break;
}
if (!(state & URWLOCK_WRITE_OWNER) &&
URWLOCK_READER_COUNT(state) == 0) {
umtxq_lock(&uq->uq_key);
umtxq_unbusy(&uq->uq_key);
umtxq_unlock(&uq->uq_key);
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ break;
continue;
}
sleep:
@@ -2842,9 +2962,21 @@ sleep:
for (;;) {
oldstate = casuword32(&rwlock->rw_state, state,
state & ~URWLOCK_WRITE_WAITERS);
+ if (oldstate == -1) {
+ error = EFAULT;
+ break;
+ }
if (oldstate == state)
break;
state = oldstate;
+ error = umtxq_check_susp(td);
+ /*
+ * We are leaving the URWLOCK_WRITE_WAITERS
+ * behind, but this should not harm the
+ * correctness.
+ */
+ if (error != 0)
+ break;
}
blocked_readers = fuword32(&rwlock->rw_blocked_readers);
} else
@@ -2880,12 +3012,19 @@ do_rw_unlock(struct thread *td, struct u
for (;;) {
oldstate = casuword32(&rwlock->rw_state, state,
state & ~URWLOCK_WRITE_OWNER);
+ if (oldstate == -1) {
+ error = EFAULT;
+ goto out;
+ }
if (oldstate != state) {
state = oldstate;
if (!(oldstate & URWLOCK_WRITE_OWNER)) {
error = EPERM;
goto out;
}
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ goto out;
} else
break;
}
@@ -2893,14 +3032,20 @@ do_rw_unlock(struct thread *td, struct u
for (;;) {
oldstate = casuword32(&rwlock->rw_state, state,
state - 1);
+ if (oldstate == -1) {
+ error = EFAULT;
+ goto out;
+ }
if (oldstate != state) {
state = oldstate;
if (URWLOCK_READER_COUNT(oldstate) == 0) {
error = EPERM;
goto out;
}
- }
- else
+ error = umtxq_check_susp(td);
+ if (error != 0)
+ goto out;
+ } else
break;
}
} else {
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
