Date: Thu, 13 Jun 2013 09:33:22 +0000 (UTC) From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r251684 - head/sys/kern Message-ID: <201306130933.r5D9XM4j087891@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
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 {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201306130933.r5D9XM4j087891>