Date: Thu, 16 Oct 2008 12:42:56 +0000 (UTC) From: Attilio Rao <attilio@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r183955 - head/sys/kern Message-ID: <200810161242.m9GCgu5w061909@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: attilio Date: Thu Oct 16 12:42:56 2008 New Revision: 183955 URL: http://svn.freebsd.org/changeset/base/183955 Log: - Fix a race in witness_checkorder() where, between the PCPU_GET() and PCPU_PTR() curthread can migrate on another CPU and get incorrect results. - Fix a similar race into witness_warn(). - Fix the interlock's checks bypassing by correctly using the appropriate children even when the lock_list chunk to be explored is not the first one. - Allow witness_warn() to work with spinlocks too. Bugs found by: tegge Submitted by: jhb, tegge Tested by: Giovanni Trematerra <giovanni dot trematerra at gmail dot com> Modified: head/sys/kern/subr_witness.c Modified: head/sys/kern/subr_witness.c ============================================================================== --- head/sys/kern/subr_witness.c Thu Oct 16 12:31:03 2008 (r183954) +++ head/sys/kern/subr_witness.c Thu Oct 16 12:42:56 2008 (r183955) @@ -103,6 +103,7 @@ __FBSDID("$FreeBSD$"); #include <sys/priv.h> #include <sys/proc.h> #include <sys/sbuf.h> +#include <sys/sched.h> #include <sys/stack.h> #include <sys/sysctl.h> #include <sys/systm.h> @@ -1015,7 +1016,7 @@ void witness_checkorder(struct lock_object *lock, int flags, const char *file, int line, struct lock_object *interlock) { - struct lock_list_entry **lock_list, *lle; + struct lock_list_entry *lock_list, *lle; struct lock_instance *lock1, *lock2, *plock; struct lock_class *class; struct witness *w, *w1; @@ -1046,35 +1047,34 @@ witness_checkorder(struct lock_object *l * If this is the first lock acquired then just return as * no order checking is needed. */ - if (td->td_sleeplocks == NULL) + lock_list = td->td_sleeplocks; + if (lock_list == NULL || lock_list->ll_count == 0) return; - lock_list = &td->td_sleeplocks; } else { /* * If this is the first lock, just return as no order - * checking is needed. We check this in both if clauses - * here as unifying the check would require us to use a - * critical section to ensure we don't migrate while doing - * the check. Note that if this is not the first lock, we - * are already in a critical section and are safe for the - * rest of the check. + * checking is needed. Avoid problems with thread + * migration pinning the thread while checking if + * spinlocks are held. If at least one spinlock is held + * the thread is in a safe path and it is allowed to + * unpin it. */ - if (PCPU_GET(spinlocks) == NULL) + sched_pin(); + lock_list = PCPU_GET(spinlocks); + if (lock_list == NULL || lock_list->ll_count == 0) { + sched_unpin(); return; - lock_list = PCPU_PTR(spinlocks); + } + sched_unpin(); } - /* Empty list? */ - if ((*lock_list)->ll_count == 0) - return; - /* * Check to see if we are recursing on a lock we already own. If * so, make sure that we don't mismatch exclusive and shared lock * acquires. */ - lock1 = find_instance(*lock_list, lock); + lock1 = find_instance(lock_list, lock); if (lock1 != NULL) { if ((lock1->li_flags & LI_EXCLUSIVE) != 0 && (flags & LOP_EXCLUSIVE) == 0) { @@ -1098,17 +1098,22 @@ witness_checkorder(struct lock_object *l /* * Find the previously acquired lock, but ignore interlocks. */ - plock = &(*lock_list)->ll_children[(*lock_list)->ll_count - 1]; + plock = &lock_list->ll_children[lock_list->ll_count - 1]; if (interlock != NULL && plock->li_lock == interlock) { - if ((*lock_list)->ll_count == 1) { + if (lock_list->ll_count > 1) + plock = + &lock_list->ll_children[lock_list->ll_count - 2]; + else { + lle = lock_list->ll_next; /* * The interlock is the only lock we hold, so - * nothing to do. + * simply return. */ - return; + if (lle == NULL) + return; + plock = &lle->ll_children[lle->ll_count - 1]; } - plock = &(*lock_list)->ll_children[(*lock_list)->ll_count - 2]; } /* @@ -1154,7 +1159,7 @@ witness_checkorder(struct lock_object *l if (isitmychild(w1, w)) goto out; - for (j = 0, lle = *lock_list; lle != NULL; lle = lle->ll_next) { + for (j = 0, lle = lock_list; lle != NULL; lle = lle->ll_next) { for (i = lle->ll_count - 1; i >= 0; i--, j++) { MPASS(j < WITNESS_COUNT); @@ -1582,7 +1587,7 @@ witness_thread_exit(struct thread *td) int witness_warn(int flags, struct lock_object *lock, const char *fmt, ...) { - struct lock_list_entry **lock_list, *lle; + struct lock_list_entry *lock_list, *lle; struct lock_instance *lock1; struct thread *td; va_list ap; @@ -1615,17 +1620,33 @@ witness_warn(int flags, struct lock_obje n++; witness_list_lock(lock1); } - if (PCPU_GET(spinlocks) != NULL) { - lock_list = PCPU_PTR(spinlocks); + + /* + * Pin the thread in order to avoid problems with thread migration. + * Once that all verifies are passed about spinlocks ownership, + * the thread is in a safe path and it can be unpinned. + */ + sched_pin(); + lock_list = PCPU_GET(spinlocks); + if (lock_list != NULL) { /* Empty list? */ - if ((*lock_list)->ll_count == 0) + if (lock_list->ll_count == 0) { + sched_unpin(); return (n); + } + sched_unpin(); /* - * Since we already hold a spinlock preemption is - * already blocked. + * We should only have one spinlock and as long as + * the flags cannot match for this locks class, + * check if the first spinlock is the one curthread + * should hold. */ + lock1 = &lock_list->ll_children[lock_list->ll_count - 1]; + if (lock1->li_lock == lock) + return (n); + if (n == 0) { va_start(ap, fmt); vprintf(fmt, ap); @@ -1635,8 +1656,9 @@ witness_warn(int flags, struct lock_obje printf(" non-sleepable"); printf(" locks held:\n"); } - n += witness_list_locks(PCPU_PTR(spinlocks)); - } + n += witness_list_locks(&lock_list); + } else + sched_unpin(); if (flags & WARN_PANIC && n) panic("%s", __func__); else
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200810161242.m9GCgu5w061909>