From 7119b8aef73957d0bc0b8dbaaef2a071eed31cdb Mon Sep 17 00:00:00 2001 From: Leding Li Date: Sat, 11 Jul 2026 02:50:54 +0800 Subject: [PATCH] vm: fix cdev pager object lifecycle and allocation race cdev_pager_allocate() previously called cdev_pg_ctor before looking up the object. Repeated allocations therefore called a non-idempotent constructor multiple times for one allocated vm_object, while the destructor ran only once. In addition, cdev_pager_allocate() could race with other threads allocating an object. This commit fixes both problems. Reserve a newly allocated object in the pager list with a NULL ops pointer while its constructor runs outside dev_pager_mtx. Concurrent lookup and allocation callers wait for construction to finish. Publish the ops pointer and wake them on success; remove and mark the object dead before waking them on failure. Use ops rather than dev as the construction sentinel because DragonFly has valid NULL-handle pager users. Derived-from: FreeBSD (commit e93404065177d6c909cd64bf7d74fe0d8df35edf) GitHub-PR: https://github.com/DragonFlyBSD/DragonFlyBSD/pull/49 --- sys/vm/device_pager.c | 120 +++++++++++++++++++++++++++++++----------- 1 file changed, 89 insertions(+), 31 deletions(-) diff --git a/sys/vm/device_pager.c b/sys/vm/device_pager.c index 7536bfea9bd..228c85551a9 100644 --- a/sys/vm/device_pager.c +++ b/sys/vm/device_pager.c @@ -93,13 +93,46 @@ static struct cdev_pager_ops old_dev_pager_ops = { .cdev_pg_fault = old_dev_pager_fault }; +static vm_object_t +cdev_pager_lookup_locked(void *handle, vm_pindex_t pindex) +{ + vm_object_t object; + +again: + object = vm_pager_object_lookup(&dev_pager_object_list, handle); + if (object != NULL) { + if (object->un_pager.devp.ops == NULL) { + /* This object is reserved during the allocation. */ + mtxsleep(&object->un_pager.devp.ops, &dev_pager_mtx, 0, + "cdplkp", 0); + mtx_unlock(&dev_pager_mtx); + vm_object_deallocate(object); + mtx_lock(&dev_pager_mtx); + goto again; + } + + if (pindex > 0) { + /* + * Called from cdev_pager_allocate() and raced with + * other thread with allocating object. + */ + vm_object_hold(object); + if (pindex > object->size) + object->size = pindex; + vm_object_drop(object); + } + } + + return (object); +} + vm_object_t cdev_pager_lookup(void *handle) { vm_object_t object; mtx_lock(&dev_pager_mtx); - object = vm_pager_object_lookup(&dev_pager_object_list, handle); + object = cdev_pager_lookup_locked(handle, 0); mtx_unlock(&dev_pager_mtx); return (object); @@ -110,9 +143,10 @@ cdev_pager_allocate(void *handle, enum obj_type tp, struct cdev_pager_ops *ops, vm_ooffset_t size, vm_prot_t prot, vm_ooffset_t foff, struct ucred *cred) { cdev_t dev; - vm_object_t object; + vm_object_t object, object1; vm_pindex_t pindex; u_short color; + int error; /* * Offset should be page aligned. @@ -123,44 +157,68 @@ cdev_pager_allocate(void *handle, enum obj_type tp, struct cdev_pager_ops *ops, size = round_page64(size); pindex = OFF_TO_IDX(foff + size); - if (ops->cdev_pg_ctor(handle, size, prot, foff, cred, &color) != 0) - return (NULL); - /* - * Look up pager, creating as necessary. + * Look up pager and handle races. */ mtx_lock(&dev_pager_mtx); - object = vm_pager_object_lookup(&dev_pager_object_list, handle); - if (object == NULL) { - /* - * Allocate object and associate it with the pager. - */ - object = vm_object_allocate_hold(tp, pindex); - object->handle = handle; - object->un_pager.devp.ops = ops; - object->un_pager.devp.dev = handle; - TAILQ_INIT(&object->un_pager.devp.devp_pglist); - - /* - * handle is only a device for old_dev_pager_ctor. - */ - if (ops->cdev_pg_ctor == old_dev_pager_ctor) { - dev = handle; - dev->si_object = object; - } + object = cdev_pager_lookup_locked(handle, pindex); + mtx_unlock(&dev_pager_mtx); + if (object != NULL) { + KASSERT(object->type == tp, + ("Inconsistent device pager type %p %d", object, tp)); + KKASSERT(object->un_pager.devp.ops == ops); + return (object); + } - TAILQ_INSERT_TAIL(&dev_pager_object_list, object, - pager_object_entry); + /* Reserve an object before calling the constructor. */ + object1 = vm_object_allocate_hold(tp, pindex); - vm_object_drop(object); - } else { - vm_object_hold(object); - if (pindex > object->size) - object->size = pindex; + mtx_lock(&dev_pager_mtx); + object = cdev_pager_lookup_locked(handle, pindex); + if (object != NULL) { + mtx_unlock(&dev_pager_mtx); + object1->type = OBJT_DEAD; + vm_object_drop(object1); + vm_object_deallocate(object1); KASSERT(object->type == tp, ("Inconsistent device pager type %p %d", object, tp)); + KKASSERT(object->un_pager.devp.ops == ops); + return (object); + } + + object = object1; + object->handle = handle; + object->un_pager.devp.dev = handle; + object->un_pager.devp.ops = NULL; /* sentinel to detect race */ + TAILQ_INIT(&object->un_pager.devp.devp_pglist); + TAILQ_INSERT_TAIL(&dev_pager_object_list, object, pager_object_entry); + vm_object_drop(object); + + /* Only call the constructor once per object. */ + mtx_unlock(&dev_pager_mtx); + error = ops->cdev_pg_ctor(handle, size, prot, foff, cred, &color); + mtx_lock(&dev_pager_mtx); + if (error != 0) { + TAILQ_REMOVE(&dev_pager_object_list, object, + pager_object_entry); + vm_object_hold(object); + object->type = OBJT_DEAD; vm_object_drop(object); + wakeup(&object->un_pager.devp.ops); + mtx_unlock(&dev_pager_mtx); + vm_object_deallocate(object); + return (NULL); + } + + vm_object_hold(object); + object->un_pager.devp.ops = ops; + /* Handle is only a device for old_dev_pager_ctor. */ + if (ops->cdev_pg_ctor == old_dev_pager_ctor) { + dev = handle; + dev->si_object = object; } + vm_object_drop(object); + wakeup(&object->un_pager.devp.ops); mtx_unlock(&dev_pager_mtx); return (object); -- 2.53.0