From a1f1ba2d5660f8201e2fb4b1c9bed80049ed3261 Mon Sep 17 00:00:00 2001 From: alc Date: Sun, 5 Aug 2007 21:04:32 +0000 Subject: [PATCH] Consider a scenario in which one processor, call it Pt, is performing vm_object_terminate() on a device-backed object at the same time that another processor, call it Pa, is performing dev_pager_alloc() on the same device. The problem is that vm_pager_object_lookup() should not be allowed to return a doomed object, i.e., an object with OBJ_DEAD set, but it does. In detail, the unfortunate sequence of events is: Pt in vm_object_terminate() holds the doomed object's lock and sets OBJ_DEAD on the object. Pa in dev_pager_alloc() holds dev_pager_sx and calls vm_pager_object_lookup(), which returns the doomed object. Next, Pa calls vm_object_reference(), which requires the doomed object's lock, so Pa waits for Pt to release the doomed object's lock. Pt proceeds to the point in vm_object_terminate() where it releases the doomed object's lock. Pa is now able to complete vm_object_reference() because it can now complete the acquisition of the doomed object's lock. So, now the doomed object has a reference count of one! Pa releases dev_pager_sx and returns the doomed object from dev_pager_alloc(). Pt now acquires dev_pager_mtx, removes the doomed object from dev_pager_object_list, releases dev_pager_mtx, and finally calls uma_zfree with the doomed object. However, the doomed object is still in use by Pa. Repeating my key point, vm_pager_object_lookup() must not return a doomed object. Moreover, the test for the object's state, i.e., doomed or not, and the increment of the object's reference count should be carried out atomically. Reviewed by: kib Approved by: re (kensmith) MFC after: 3 weeks --- sys/vm/device_pager.c | 4 ---- sys/vm/phys_pager.c | 4 ---- sys/vm/swap_pager.c | 4 +--- sys/vm/vm_pager.c | 26 +++++++++++++++++++------- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/sys/vm/device_pager.c b/sys/vm/device_pager.c index e9b9e737f6d0..bbca5cc9ee08 100644 --- a/sys/vm/device_pager.c +++ b/sys/vm/device_pager.c @@ -160,10 +160,6 @@ dev_pager_alloc(void *handle, vm_ooffset_t size, vm_prot_t prot, vm_ooffset_t fo TAILQ_INSERT_TAIL(&dev_pager_object_list, object, pager_object_list); mtx_unlock(&dev_pager_mtx); } else { - /* - * Gain a reference to the object. - */ - vm_object_reference(object); if (pindex > object->size) object->size = pindex; } diff --git a/sys/vm/phys_pager.c b/sys/vm/phys_pager.c index 8f3fd3af5a79..626e888f42c9 100644 --- a/sys/vm/phys_pager.c +++ b/sys/vm/phys_pager.c @@ -101,10 +101,6 @@ phys_pager_alloc(void *handle, vm_ooffset_t size, vm_prot_t prot, pager_object_list); mtx_unlock(&phys_pager_mtx); } else { - /* - * Gain a reference to the object. - */ - vm_object_reference(object); if (pindex > object->size) object->size = pindex; } diff --git a/sys/vm/swap_pager.c b/sys/vm/swap_pager.c index 8a3db43ce20e..eb825795fa8a 100644 --- a/sys/vm/swap_pager.c +++ b/sys/vm/swap_pager.c @@ -459,9 +459,7 @@ swap_pager_alloc(void *handle, vm_ooffset_t size, vm_prot_t prot, sx_xlock(&sw_alloc_sx); object = vm_pager_object_lookup(NOBJLIST(handle), handle); - if (object != NULL) { - vm_object_reference(object); - } else { + if (object == NULL) { object = vm_object_allocate(OBJT_DEFAULT, pindex); object->handle = handle; diff --git a/sys/vm/vm_pager.c b/sys/vm/vm_pager.c index 28bd1358f699..f03b951715ea 100644 --- a/sys/vm/vm_pager.c +++ b/sys/vm/vm_pager.c @@ -261,17 +261,29 @@ vm_pager_deallocate(object) * vm_pager_has_page() - inline, see vm/vm_pager.h */ +/* + * Search the specified pager object list for an object with the + * specified handle. If an object with the specified handle is found, + * increase its reference count and return it. Otherwise, return NULL. + * + * The pager object list must be locked. + */ vm_object_t -vm_pager_object_lookup(pg_list, handle) - struct pagerlst *pg_list; - void *handle; +vm_pager_object_lookup(struct pagerlst *pg_list, void *handle) { vm_object_t object; - TAILQ_FOREACH(object, pg_list, pager_object_list) - if (object->handle == handle) - return (object); - return (NULL); + TAILQ_FOREACH(object, pg_list, pager_object_list) { + VM_OBJECT_LOCK(object); + if (object->handle == handle && + (object->flags & OBJ_DEAD) == 0) { + vm_object_reference_locked(object); + VM_OBJECT_UNLOCK(object); + break; + } + VM_OBJECT_UNLOCK(object); + } + return (object); } /*