From eb8758bc5e1f5763ca26a700cd6ea8f2ac000840 Mon Sep 17 00:00:00 2001 From: Max Wash Date: Thu, 26 Feb 2026 19:41:40 +0000 Subject: [PATCH] vm: region: fix some cases where regions weren't being unlocked after use. --- vm/vm-region.c | 122 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 80 insertions(+), 42 deletions(-) diff --git a/vm/vm-region.c b/vm/vm-region.c index 47ed246..6358f84 100644 --- a/vm/vm-region.c +++ b/vm/vm-region.c @@ -36,6 +36,15 @@ region_find_free_area_linear(region, length) #endif +#define unlock_mapping_parent(p, root) \ + do { \ + struct vm_region *parent \ + = region_from_entry(p->m_entry.e_parent); \ + if (parent != root) { \ + vm_region_unlock(parent); \ + } \ + } while (0) + /* iterates over a range of mapped virtual memory in a region, and provides * a moving buffer through which the memory can be accessed */ struct vm_iterator { @@ -296,12 +305,13 @@ static struct vm_region *region_get_child_region_recursive( * this function should be called with `region` locked. if a mapping is found, * it will be returned with its immediate parent locked. */ static struct vm_region_mapping *region_get_mapping_recursive( - struct vm_region *region, + struct vm_region *root, off_t *offp, size_t len) { off_t offset = *offp; - region = region_get_child_region_recursive(region, &offset, len); + struct vm_region *region + = region_get_child_region_recursive(root, &offset, len); if (!region) { return NULL; } @@ -311,6 +321,14 @@ static struct vm_region_mapping *region_get_mapping_recursive( struct vm_region_entry *entry = region_get_entry(region, offset, len); *offp = offset; + if (!entry) { + if (region != root) { + vm_region_unlock(region); + } + + return NULL; + } + /* return the mapping with the parent region still locked */ return mapping_from_entry(entry); } @@ -593,7 +611,12 @@ static void vm_iterator_begin( off_t offset = base - vm_region_get_base_address(region); it->it_mapping = region_get_mapping_recursive(region, &offset, 1); - if (!it->it_mapping || (it->it_mapping->m_prot & prot) != prot) { + if (!it->it_mapping) { + return; + } + + if ((it->it_mapping->m_prot & prot) != prot) { + unlock_mapping_parent(it->it_mapping, region); return; } @@ -612,6 +635,7 @@ static void vm_iterator_begin( } if (!pg) { + unlock_mapping_parent(it->it_mapping, region); return; } @@ -643,15 +667,6 @@ static void vm_iterator_begin( static kern_status_t vm_iterator_seek(struct vm_iterator *it, size_t nr_bytes) { -#define UNLOCK_MAPPING_PARENT(p) \ - do { \ - struct vm_region *parent \ - = region_from_entry(p->m_entry.e_parent); \ - if (parent != it->it_region) { \ - vm_region_unlock(parent); \ - } \ - } while (0) - if (nr_bytes < it->it_max) { it->it_base += nr_bytes; it->it_buf = (char *)it->it_buf + nr_bytes; @@ -661,7 +676,7 @@ static kern_status_t vm_iterator_seek(struct vm_iterator *it, size_t nr_bytes) /* the parent region of it->it_mapping is locked here. if it is * different from it->it_region, it must be unlocked */ - UNLOCK_MAPPING_PARENT(it->it_mapping); + unlock_mapping_parent(it->it_mapping, it->it_region); it->it_base += nr_bytes; off_t offset = it->it_base - vm_region_get_base_address(it->it_region); @@ -674,13 +689,13 @@ static kern_status_t vm_iterator_seek(struct vm_iterator *it, size_t nr_bytes) return KERN_MEMORY_FAULT; } - /* past this point, if we encounter an error, must remember to unlock - * the parent region of next_mapping */ + /* past this point, if we encounter an error, must remember to + * unlock the parent region of next_mapping */ if ((next_mapping->m_prot & it->it_prot) != it->it_prot) { it->it_buf = NULL; it->it_max = 0; - UNLOCK_MAPPING_PARENT(next_mapping); + unlock_mapping_parent(next_mapping, it->it_region); return KERN_MEMORY_FAULT; } @@ -699,7 +714,7 @@ static kern_status_t vm_iterator_seek(struct vm_iterator *it, size_t nr_bytes) } if (!pg) { - UNLOCK_MAPPING_PARENT(next_mapping); + unlock_mapping_parent(next_mapping, it->it_region); return KERN_NO_MEMORY; } @@ -730,9 +745,20 @@ static kern_status_t vm_iterator_seek(struct vm_iterator *it, size_t nr_bytes) return KERN_OK; } -/* this function must be called with `root` locked. `root` will be the first - * entry visited by the iterator. from there, child entries are visited in - * depth-first order. */ +/* this function must be called when you are finished with a + * vm_iterator, to ensure that all held locks are released. */ +static void vm_iterator_finish(struct vm_iterator *it) +{ + if (it->it_mapping) { + unlock_mapping_parent(it->it_mapping, it->it_region); + } + + memset(it, 0x0, sizeof *it); +} + +/* this function must be called with `root` locked. `root` will be the + * first entry visited by the iterator. from there, child entries are + * visited in depth-first order. */ static void entry_iterator_begin( struct entry_iterator *it, struct vm_region *root) @@ -742,8 +768,8 @@ static void entry_iterator_begin( it->it_entry = &root->vr_entry; } -/* this function must be called when you are finished with an entry_iterator, - * to ensure that all held locks are released. */ +/* this function must be called when you are finished with an + * entry_iterator, to ensure that all held locks are released. */ static void entry_iterator_finish(struct entry_iterator *it) { struct vm_region_entry *cur = it->it_entry; @@ -771,10 +797,10 @@ static void entry_iterator_finish(struct entry_iterator *it) /* move to the next entry in the traversal order. * when this function returns: * 1. if the visited entry is a region, it will be locked. - * 2. if the visited entry is a mapping, its parent region will be locked. - * a region will remain locked until all of its children and n-grand-children - * have been visited. once iteration is finished, only `it->it_root` will be - * locked. + * 2. if the visited entry is a mapping, its parent region will be + * locked. a region will remain locked until all of its children and + * n-grand-children have been visited. once iteration is finished, only + * `it->it_root` will be locked. */ static void entry_iterator_move_next(struct entry_iterator *it) { @@ -791,8 +817,9 @@ static void entry_iterator_move_next(struct entry_iterator *it) if (entry->e_type == VM_REGION_ENTRY_REGION) { struct vm_region *child_region = region_from_entry(entry); - /* since `region` is locked, interrupts are already - * disabled, so don't use lock_irq() here */ + /* since `region` is locked, interrupts are + * already disabled, so don't use lock_irq() + * here */ vm_region_lock(child_region); } @@ -839,8 +866,9 @@ static void entry_iterator_move_next(struct entry_iterator *it) } } -/* erase the current entry and move to the next entry in the traversal order. - * the current entry MUST be a mapping, otherwise nothing will happen. +/* erase the current entry and move to the next entry in the traversal + * order. the current entry MUST be a mapping, otherwise nothing will + * happen. */ static void entry_iterator_erase(struct entry_iterator *it) { @@ -999,7 +1027,8 @@ struct vm_region *vm_region_cast(struct object *obj) return VM_REGION_CAST(obj); } -/* this function should be called with `parent` locked (if parent is non-NULL) +/* this function should be called with `parent` locked (if parent is + * non-NULL) */ kern_status_t vm_region_create( struct vm_region *parent, @@ -1088,9 +1117,9 @@ kern_status_t vm_region_kill( = region_from_entry(region->vr_entry.e_parent); region->vr_entry.e_parent = NULL; - /* locks must be acquired in parent->child order. since we're - * going backwards here, unlock `region` before locking its - * parent */ + /* locks must be acquired in parent->child order. since + * we're going backwards here, unlock `region` before + * locking its parent */ vm_region_unlock_irqrestore(region, *lock_flags); vm_region_lock_irqsave(parent, lock_flags); btree_delete(&parent->vr_entries, ®ion->vr_entry.e_node); @@ -1175,8 +1204,8 @@ kern_status_t vm_region_map_object( root, ®ion_offset, length); - /* if `region` != `root`, it will need to be unlocked at the end - * of the function */ + /* if `region` != `root`, it will need to be unlocked at + * the end of the function */ } if (region->vr_status != VM_REGION_ONLINE) { @@ -1257,8 +1286,8 @@ kern_status_t vm_region_map_object( return KERN_OK; } -/* unmap some pages in the middle of a mapping, splitting it into two separate - * mappings */ +/* unmap some pages in the middle of a mapping, splitting it into two + * separate mappings */ static kern_status_t split_mapping( struct vm_region_mapping *mapping, struct vm_region *root, @@ -1488,7 +1517,8 @@ kern_status_t vm_region_unmap( unmap_area_offset, unmap_area_limit); } else { - panic("don't know what to do with this mapping"); + panic("don't know what to do with this " + "mapping"); } if (delete) { @@ -1557,15 +1587,18 @@ bool vm_region_validate_access( return false; } - if ((mapping->m_prot & prot) != prot) { - return false; - } + bool ok = (mapping->m_prot & prot) == prot; struct vm_region *parent = region_from_entry(mapping->m_entry.e_parent); + if (parent != region) { vm_region_unlock(parent); } + + if (!ok) { + return false; + } } return true; @@ -1671,6 +1704,8 @@ kern_status_t vm_region_read_kernel( dest += to_move; } + vm_iterator_finish(&src); + if (nr_read) { *nr_read = r; } @@ -1727,6 +1762,9 @@ kern_status_t vm_region_memmove( r += to_move; } + vm_iterator_finish(&src); + vm_iterator_finish(&dest); + if (nr_moved) { *nr_moved = r; }