Hi,
in the past weeks I've been reading through the fiasco kernel code. This is the start of a series of patches that clean up stuff or fix bugs that I think I found. Comments appreciated.
The first patch renames a method called compund to compound, more important patches follow :-)
regards Christian
diff --git a/src/kernel/fiasco/src/abi/l4_msg_item.cpp b/src/kernel/fiasco/src/abi/l4_msg_item.cpp index bbb92ef..1155a5f 100644 --- a/src/kernel/fiasco/src/abi/l4_msg_item.cpp +++ b/src/kernel/fiasco/src/abi/l4_msg_item.cpp @@ -16,7 +16,7 @@ public: };
explicit L4_msg_item(Mword raw) : _raw(raw) {} - Mword compund() const { return _raw & 1; } + Mword compound() const { return _raw & 1; } Type type() const { return Type(_raw & 8); } bool is_void() const { return _raw == 0; } bool is_small_obj() const { return _raw & 2; } diff --git a/src/kernel/fiasco/src/kern/thread-ipc.cpp b/src/kernel/fiasco/src/kern/thread-ipc.cpp index f8e1995..3288b2a 100644 --- a/src/kernel/fiasco/src/kern/thread-ipc.cpp +++ b/src/kernel/fiasco/src/kern/thread-ipc.cpp @@ -948,7 +948,7 @@ Thread::transfer_msg_items(L4_msg_tag const &tag, Thread* snd, Utcb *snd_utcb,
--items;
- if (!item->b.compund()) + if (!item->b.compound()) buf_iter->next(); }
Hi,
The constructor for L4_msg_tag looks bogus wrt. the items() bits. The argument value should be mask and then shifted and not vice versa.
There is a single caller that passes a non-NULL value that needs checking though.
regards Christian
diff --git a/src/kernel/fiasco/src/abi/l4_types.cpp b/src/kernel/fiasco/src/abi/l4_types.cpp index 6a98f2d..96360f9 100644 --- a/src/kernel/fiasco/src/abi/l4_types.cpp +++ b/src/kernel/fiasco/src/abi/l4_types.cpp @@ -399,7 +399,7 @@ bool Utcb::inherit_fpu() const PUBLIC inline L4_msg_tag::L4_msg_tag(unsigned words, unsigned items, unsigned long flags, unsigned long proto) - : _tag((words & 0x3f) | ((items << 6) & 0x3f) | flags | (proto << 16)) + : _tag((words & 0x3f) | ((items & 0x3f) << 6) | flags | (proto << 16)) {}
PUBLIC inline diff --git a/src/kernel/fiasco/src/kern/factory.cpp b/src/kernel/fiasco/src/kern/factory.cpp index 144130b..bbf8e43 100644 --- a/src/kernel/fiasco/src/kern/factory.cpp +++ b/src/kernel/fiasco/src/kern/factory.cpp @@ -104,6 +104,7 @@ Factory::map_obj(Kobject_iface *o, Mword cap, Space *c_space, }
// FIXME: reap stuff if needed + /* XXX CEH: This will generate a result tag with one item, what is it? */ return commit_result(0, 0, 1); }
Access to the _free_map bitmap in the buddy allocator is possible beyond Max_mem when checking the free bit of the buddy. Thus bit bitmap size should be aligned to Max_size not Min_size.
regards Christian
diff --git a/src/kernel/fiasco/src/kern/buddy_alloc.cpp b/src/kernel/fiasco/src/kern/buddy_alloc.cpp index 96f2821..51e1239 100644 --- a/src/kernel/fiasco/src/kern/buddy_alloc.cpp +++ b/src/kernel/fiasco/src/kern/buddy_alloc.cpp @@ -55,7 +55,7 @@ public:
private: Head *_free[Num_sizes]; - Bitmap<(Max_mem+Min_size-1)/Min_size> _free_map; + Bitmap<(Max_mem+Max_size-1)/Min_size> _free_map; };
On Tue, 2011-05-03 at 13:26 +0200, Christian Ehrhardt wrote:
Access to the _free_map bitmap in the buddy allocator is possible beyond Max_mem when checking the free bit of the buddy. Thus bit bitmap size should be aligned to Max_size not Min_size.
regards Christian
diff --git a/src/kernel/fiasco/src/kern/buddy_alloc.cpp b/src/kernel/fiasco/src/kern/buddy_alloc.cpp index 96f2821..51e1239 100644 --- a/src/kernel/fiasco/src/kern/buddy_alloc.cpp +++ b/src/kernel/fiasco/src/kern/buddy_alloc.cpp @@ -55,7 +55,7 @@ public:
private: Head *_free[Num_sizes];
- Bitmap<(Max_mem+Min_size-1)/Min_size> _free_map;
- Bitmap<(Max_mem+Max_size-1)/Min_size> _free_map;
};
Tanks, however, the fix is somehow different, we only need a single extra bit in the case when Max_mem is not a multiple of Max_size (not the case in Fiasco, 64MB and 128kB).
Anyway, good point in general.
The function Generic_io_space<SPACE>::bitmap_pde_lookup() is unused. Additionally, it passes and Address value directly to mem_space()->dir()->walk() which expects a Page frame number and not an address. Remove it.
regards Christian
diff --git a/src/kernel/fiasco/src/kern/io_space.cpp b/src/kernel/fiasco/src/kern/io_space.cpp index 87690f9..ad6f468 100644 --- a/src/kernel/fiasco/src/kern/io_space.cpp +++ b/src/kernel/fiasco/src/kern/io_space.cpp @@ -459,14 +459,6 @@ Generic_io_space<SPACE>::get_port_bit(Address const port_number) const return 1 << (port_number & 7); }
- -PRIVATE template< typename SPACE > -inline -Pt_entry *Generic_io_space<SPACE>::bitmap_pde_lookup(Address v) -{ - return (Pt_entry*)(mem_space()->dir()->walk(v/*PTV >> 12*/, Pdir::Super_level).e); -} - PUBLIC template< typename SPACE > inline static Page_number
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();
Remove unused code: - Mem_space::reset_dirty() and Space::reset_dirty() are unused. Remove. - Some Utcb-Area retrival functions are unused and are no longer useful as there can be several Ku_mem areas. Remove. - Space::is_user_memory should probably check for integer overflows or the check is not safe when performed with values provided by the user.
diff --git a/src/kernel/fiasco/src/kern/mem_space.cpp b/src/kernel/fiasco/src/kern/mem_space.cpp index 8ac2cb3..080b881 100644 --- a/src/kernel/fiasco/src/kern/mem_space.cpp +++ b/src/kernel/fiasco/src/kern/mem_space.cpp @@ -246,14 +246,6 @@ Mem_space::ram_quota() const { return _quota; }
-/// Avoid deallocation of page table upon Mem_space destruction. -PUBLIC -void -Mem_space::reset_dirty () -{ - _dir = 0; -} - PUBLIC inline Mem_space::Dir_type* Mem_space::dir () diff --git a/src/kernel/fiasco/src/kern/space.cpp b/src/kernel/fiasco/src/kern/space.cpp index 91659d2..a709da8 100644 --- a/src/kernel/fiasco/src/kern/space.cpp +++ b/src/kernel/fiasco/src/kern/space.cpp @@ -150,42 +150,6 @@ IMPLEMENTATION: // class Space //
- - -/** - * UTCB area functions. - */ -//@{ - - -/** - * Get size of UTCB area in bytes. - * - * @return the size of the UTCB area in bytes. - */ -PUBLIC inline -unsigned long -Space::utcb_area_size() const -{ return _ku_mem->size; } - -PUBLIC inline -Address -Space::kern_utcb_area() const -{ return (Address)_ku_mem->k_addr; } - -/** - * Get the start of the UTCB area in the user address-space. - * - * @return the start address of the UTCB area in trhe user address-space. - */ -PUBLIC inline -Address -Space::user_utcb_area() const -{ return (Address)_ku_mem->u_addr.get(); } - - -//@} - PUBLIC Space::Ku_mem const * Space::find_ku_mem(User<void>::Ptr p, unsigned size) @@ -193,6 +157,9 @@ Space::find_ku_mem(User<void>::Ptr p, unsigned size) if ((Address)p.get() & (sizeof(double) - 1)) return 0;
+ /* Check for integer overflows! */ + if ((Address)p.get() > (Address)((Address)p.get() + size)) + return 0; for (Ku_mem const *f = _ku_mem; f; f = f->next) { Address a = (Address)f->u_addr.get(); @@ -244,13 +211,6 @@ Ram_quota * Space::ram_quota() const { return _mem_space.get()->ram_quota(); }
-PROTECTED -void -Space::reset_dirty() -{ - _mem_space.get()->reset_dirty(); -} -
PUBLIC inline void @@ -289,6 +249,7 @@ bool Space::is_user_memory(Address address, Mword len) { return address < Mem_layout::User_max + && address <= address + len /* Check for integer overflows */ && address + len <= Mem_layout::User_max; }
Task::alloc_ku_mem_chunk() calls free_ku_mem_chunk in case of errors. However, free_ku_mem_chunk() will try to unmap the entire area which is wrong when called from Task::alloc_ku_mem_chunk().
diff --git a/src/kernel/fiasco/src/kern/task.cpp b/src/kernel/fiasco/src/kern/task.cpp index 35fef8c..4537450 100644 --- a/src/kernel/fiasco/src/kern/task.cpp +++ b/src/kernel/fiasco/src/kern/task.cpp @@ -146,18 +146,18 @@ Task::alloc_ku_mem_chunk(User<void>::Ptr u_addr, unsigned size, void **k_addr) { case Mem_space::Insert_ok: break; case Mem_space::Insert_err_nomem: - free_ku_mem_chunk(p, u_addr, size); + free_ku_mem_chunk(p, u_addr, size, i); return -L4_err::ENomem;
case Mem_space::Insert_err_exists: - free_ku_mem_chunk(p, u_addr, size); + free_ku_mem_chunk(p, u_addr, size, i); return -L4_err::EExists;
default: printf("UTCB mapping failed: va=%p, ph=%p, res=%d\n", (void*)user_va, (void*)kern_va, res); kdb_ke("BUG in utcb allocation"); - free_ku_mem_chunk(p, u_addr, size); + free_ku_mem_chunk(p, u_addr, size, i); return 0; } } @@ -208,13 +208,14 @@ PRIVATE inline NOEXPORT void Task::free_ku_mem(Ku_mem *m) { - free_ku_mem_chunk(m->k_addr, m->u_addr, m->size); + free_ku_mem_chunk(m->k_addr, m->u_addr, m->size, m->size); m->free(ram_quota()); }
PRIVATE void -Task::free_ku_mem_chunk(void *k_addr, User<void>::Ptr u_addr, unsigned size) +Task::free_ku_mem_chunk(void *k_addr, User<void>::Ptr u_addr, unsigned size, + unsigned mapped_size) {
Mapped_allocator * const alloc = Mapped_allocator::allocator(); @@ -226,7 +227,7 @@ Task::free_ku_mem_chunk(void *k_addr, User<void>::Ptr u_addr, unsigned size) if (size >= Config::SUPERPAGE_SIZE) page_size = Config::SUPERPAGE_SIZE;
- for (unsigned long i = 0; i < size; i += page_size) + for (unsigned long i = 0; i < mapped_size; i += page_size) { Address user_va = (Address)u_addr.get() + i; mem_space()->v_delete(Mem_space::Addr(user_va),
Sender::receiver() might return a stale value as the _receiver field is not cleared. AFAICS the safe way to check for presence in a sender list is in_sender_list() and not receiver(). Check both in asserts and use in_sender_list() in Thread::do_kill.
diff --git a/src/kernel/fiasco/src/kern/ipc_sender.cpp b/src/kernel/fiasco/src/kern/ipc_sender.cpp index bd4f9f8..3290255 100644 --- a/src/kernel/fiasco/src/kern/ipc_sender.cpp +++ b/src/kernel/fiasco/src/kern/ipc_sender.cpp @@ -31,7 +31,7 @@ PUBLIC virtual void Ipc_sender_base::ipc_receiver_aborted() { - assert (receiver()); + assert (receiver() && in_sender_list());
sender_dequeue(receiver()->sender_list()); receiver()->vcpu_update_state(); diff --git a/src/kernel/fiasco/src/kern/thread-ipc.cpp b/src/kernel/fiasco/src/kern/thread-ipc.cpp index f8e1995..3288b2a 100644 --- a/src/kernel/fiasco/src/kern/thread-ipc.cpp +++ b/src/kernel/fiasco/src/kern/thread-ipc.cpp @@ -111,7 +111,7 @@ PUBLIC virtual void Thread::ipc_receiver_aborted() { - assert_kdb (receiver()); + assert_kdb (receiver() && in_sender_list());
sender_dequeue(receiver()->sender_list()); receiver()->vcpu_update_state(); diff --git a/src/kernel/fiasco/src/kern/thread.cpp b/src/kernel/fiasco/src/kern/thread.cpp index 08150e8..27f2206 100644 --- a/src/kernel/fiasco/src/kern/thread.cpp +++ b/src/kernel/fiasco/src/kern/thread.cpp @@ -561,7 +561,7 @@ Thread::do_kill() }
// if engaged in IPC operation, stop it - if (receiver()) + if (in_sender_list()) sender_dequeue(receiver()->sender_list());
Context::do_kill();
If two consecutive Mem_regions are contained in a range that is given to Mem_region_map_base::sub(), the second region will not be removed. The first region will be removed and the second region will be copied to position "pos". The next loop iteration will then continue at pos+1 skipping the region just copied from pos+1 to pos by del().
diff --git a/src/kernel/fiasco/src/lib/libk/mem_region.cpp b/src/kernel/fiasco/src/lib/libk/mem_region.cpp index 6c76df2..8475d3d 100644 --- a/src/kernel/fiasco/src/lib/libk/mem_region.cpp +++ b/src/kernel/fiasco/src/lib/libk/mem_region.cpp @@ -118,7 +118,10 @@ Mem_region_map_base::sub(Mem_region const &r) if (_r[pos].overlaps(r)) { if (r.contains(_r[pos])) - del(pos, pos+1); + { + del(pos, pos+1); + pos--; + } else if (r.start <= _r[pos].start) _r[pos].start = r.end + 1; else if (r.end >= _r[pos].end)
l4-hackers@os.inf.tu-dresden.de