Bugfixes and cleanups in mapping_tree.cpp: - The destructor should not stop after _count mappings as _count does not include unused mappings. This might cause leakage of mmemory wrt. quota. - The skip_subtree variable in flush() is never true. Remove it and code that depends on it. - Remove unused lookup() functions. - The m_depth variable in flush() should have the depth value of the last mapping that was not unused or a submap. Currently, its use is inconsistent and this can AFAICS lead corrupt mapping trees, e.g. in this case: 0: parent(depth = 0) 1: submap 2: child(depth = 1) 3: submap Assume a call with me_to == false and offsets that will not result in deletion of the submaps. Now look a the loop: Both the first and the second submap will be processed with m_depth=1 == p_depth+1, i.e. both submaps will potentially survive the flush. This is a bug as the tree now contains two consecutive submaps below parent.
regards Christian
diff --git a/src/kernel/fiasco/src/kern/mapping_tree.cpp b/src/kernel/fiasco/src/kern/mapping_tree.cpp index d141d24..beee8a3 100644 --- a/src/kernel/fiasco/src/kern/mapping_tree.cpp +++ b/src/kernel/fiasco/src/kern/mapping_tree.cpp @@ -327,7 +327,7 @@ PUBLIC Mapping_tree::~Mapping_tree() { // special case for copied mapping trees - for (Mapping *m = _mappings; m < _mappings+_count && !m->is_end_tag(); ++m) + for (Mapping *m = _mappings; m < end() && !m->is_end_tag(); ++m) { if (!m->submap() && !m->unused()) quota(m->space())->free(sizeof(Mapping)); @@ -730,8 +730,7 @@ Mapping_tree::flush(Mapping *parent, bool me_too, else start_of_deletions++;
- unsigned m_depth = p_depth + 1; - bool skip_subtree = false; + unsigned m_depth = p_depth;
for (Mapping* m = parent + 1; m < end() && ! m->is_end_tag(); @@ -755,18 +754,6 @@ Mapping_tree::flush(Mapping *parent, bool me_too, continue; }
- // m is a submap or a regular mapping. - if (skip_subtree) - { - if (m->depth() > p_depth + 1) // Can use m->depth() even for submaps. - { - start_of_deletions++; - continue; - } - - skip_subtree = false; - } - Space *space;
if (Treemap* submap = m->submap()) @@ -776,7 +763,7 @@ Mapping_tree::flush(Mapping *parent, bool me_too, // Check for immediate child. Note: It's a bad idea to // call m->parent() because that would iterate backwards // over already-deleted entries. - && m_depth == p_depth + 1 + && m_depth == p_depth && submap_ops.is_partial(submap, offs_begin, offs_end)) { submap_ops.flush(submap, offs_begin, offs_end); @@ -840,39 +827,6 @@ Mapping_tree::grant(Mapping* m, Space *new_space, Page_number va,
PUBLIC inline Mapping * -Mapping_tree::lookup(Space *space, Page_number page) -{ - - Mapping *m; - - for (m = mappings(); m; m = next(m)) - { - assert (!m->submap()); - if (m->space() == space && m->page() == page) - return m; - } - - return 0; -} - - -PUBLIC -Mapping * -Base_mappable::lookup(Space *space, Page_number va) -{ - // get and lock the tree. - lock.lock(); - Mapping_tree *t = tree.get(); - assert (t); - if (Mapping *m = t->lookup(space, va)) - return m; - - lock.clear(); - return 0; -} - -PUBLIC inline -Mapping * Base_mappable::insert(Mapping* parent, Space *space, Page_number page) { Mapping_tree* t = tree.get();