From a46de84f894685a52b54bc7be4920ac9f1b9ed6a Mon Sep 17 00:00:00 2001 From: Christian Prochaska Date: Tue, 15 Jul 2014 19:05:26 +0200 Subject: [PATCH] Fix race condition in page fault notification When a page fault cannot be resolved, the GDB monitor can get a hint about which thread faulted by evaluating the thread state object returned by 'Cpu_session::state()'. Unfortunately, with the current implementation, the signal which informs GDB monitor about the page fault is sent before the thread state object of the faulted thread has been updated, so it can happen that the faulted thread cannot be determined immediately after receiving the signal. With this commit, the thread state gets updated before the signal is sent. At least on base-nova it can also happen that the thread state is not accessible yet after receiving the page fault notification. For this reason, GDB monitor needs to retry its query until the state is accessible. Fixes #1206. --- repos/base-foc/src/base/pager/pager.cc | 2 - repos/base-hw/include/base/pager.h | 7 +++ repos/base-nova/include/base/pager.h | 9 ++++ repos/base-nova/src/base/pager/pager.cc | 2 - repos/base-nova/src/core/platform_thread.cc | 7 ++- repos/base/include/base/pager.h | 6 +++ repos/base/src/base/pager/common.cc | 6 +++ repos/base/src/core/rm_session_component.cc | 2 + .../app/gdb_monitor/gdbserver/genode-low.cc | 46 +++++++++++++++---- 9 files changed, 72 insertions(+), 15 deletions(-) diff --git a/repos/base-foc/src/base/pager/pager.cc b/repos/base-foc/src/base/pager/pager.cc index 075619d7c..5121f2738 100644 --- a/repos/base-foc/src/base/pager/pager.cc +++ b/repos/base-foc/src/base/pager/pager.cc @@ -81,8 +81,6 @@ void Pager_activation_base::entry() /* handle request */ if (obj->pager(pager)) { /* could not resolv - leave thread in pagefault */ - Lock::Guard guard(obj->state.lock); - obj->state.unresolved_page_fault = true; PDBG("Could not resolve pf=%p ip=%p", (void*)pager.fault_addr(), (void*)pager.fault_ip()); } else { diff --git a/repos/base-hw/include/base/pager.h b/repos/base-hw/include/base/pager.h index 9f952d613..544ffb651 100644 --- a/repos/base-hw/include/base/pager.h +++ b/repos/base-hw/include/base/pager.h @@ -258,6 +258,13 @@ class Genode::Pager_object : public Object_pool::Entry, void thread_cap(Thread_capability const & c); unsigned signal_context_id() const; + + + /************* + ** Dummies ** + *************/ + + void unresolved_page_fault_occurred() { PDBG("not implemented"); } }; class Genode::Pager_activation_base : public Thread_base, diff --git a/repos/base-nova/include/base/pager.h b/repos/base-nova/include/base/pager.h index 2f8f63339..4e1c5d18a 100644 --- a/repos/base-nova/include/base/pager.h +++ b/repos/base-nova/include/base/pager.h @@ -220,6 +220,15 @@ namespace Genode { Thread_capability thread_cap() { return _thread_cap; } const void thread_cap(Thread_capability cap) { _thread_cap = cap; } + /* + * Note in the thread state that an unresolved page + * fault occurred. + */ + void unresolved_page_fault_occurred() + { + _state.thread.unresolved_page_fault = true; + } + /** * Make sure nobody is in the handler anymore by doing an IPC to a * local cap pointing to same serving thread (if not running in the diff --git a/repos/base-nova/src/base/pager/pager.cc b/repos/base-nova/src/base/pager/pager.cc index 1b1b04ce4..68f9dac71 100644 --- a/repos/base-nova/src/base/pager/pager.cc +++ b/repos/base-nova/src/base/pager/pager.cc @@ -79,8 +79,6 @@ void Pager_object::_page_fault_handler() } if (ret == 1) { - obj->_state.thread.unresolved_page_fault = true; - char client_name[Context::NAME_LEN]; myself->name(client_name, sizeof(client_name)); diff --git a/repos/base-nova/src/core/platform_thread.cc b/repos/base-nova/src/core/platform_thread.cc index ce46e7ae3..fc1143a7d 100644 --- a/repos/base-nova/src/core/platform_thread.cc +++ b/repos/base-nova/src/core/platform_thread.cc @@ -226,13 +226,16 @@ Thread_state Platform_thread::state() if (!_pager) throw Cpu_session::State_access_failed(); Thread_state s; + if (_pager->copy_thread_state(&s)) return s; - if (is_worker()) + if (is_worker()) { s.sp = _pager->initial_esp(); + return s; + } - return s; + throw Cpu_session::State_access_failed(); } diff --git a/repos/base/include/base/pager.h b/repos/base/include/base/pager.h index 7c03e7ea1..6a0f4377b 100644 --- a/repos/base/include/base/pager.h +++ b/repos/base/include/base/pager.h @@ -109,6 +109,12 @@ namespace Genode { */ Thread_capability thread_cap() { return _thread_cap; } const void thread_cap(Thread_capability cap) { _thread_cap = cap; } + + /* + * Note in the thread state that an unresolved page + * fault occurred. + */ + void unresolved_page_fault_occurred(); }; /** diff --git a/repos/base/src/base/pager/common.cc b/repos/base/src/base/pager/common.cc index bf529456c..8e746c052 100644 --- a/repos/base/src/base/pager/common.cc +++ b/repos/base/src/base/pager/common.cc @@ -25,3 +25,9 @@ void Pager_object::wake_up() Ipc_client ipc_client(pager, &snd, &rcv); ipc_client << this << IPC_CALL; } + + +void Pager_object::unresolved_page_fault_occurred() +{ + state.unresolved_page_fault = true; +} diff --git a/repos/base/src/core/rm_session_component.cc b/repos/base/src/core/rm_session_component.cc index eea199e07..d1be8608a 100644 --- a/repos/base/src/core/rm_session_component.cc +++ b/repos/base/src/core/rm_session_component.cc @@ -313,6 +313,8 @@ void Rm_faulter::fault(Rm_session_component *faulting_rm_session, _faulting_rm_session = faulting_rm_session; _fault_state = fault_state; + + _pager_object->unresolved_page_fault_occurred(); } diff --git a/repos/ports/src/app/gdb_monitor/gdbserver/genode-low.cc b/repos/ports/src/app/gdb_monitor/gdbserver/genode-low.cc index 74f598dc0..2a2a0c6ff 100644 --- a/repos/ports/src/app/gdb_monitor/gdbserver/genode-low.cc +++ b/repos/ports/src/app/gdb_monitor/gdbserver/genode-low.cc @@ -181,21 +181,49 @@ void genode_continue_thread(unsigned long lwpid, int single_step) } +/* + * This function returns the first thread with a page fault that it finds. + * Multiple page-faulted threads are currently not supported. + */ + unsigned long genode_find_segfault_lwpid() { + Cpu_session_component *csc = gdb_stub_thread()->cpu_session_component(); - Thread_capability thread_cap = csc->first(); + /* + * It can happen that the thread state of the thread which caused the + * page fault is not accessible yet. In that case, we'll retry until + * it is accessible. + */ + + while (1) { + + Thread_capability thread_cap = csc->first(); + + while (thread_cap.valid()) { + + try { + + Thread_state thread_state = csc->state(thread_cap); + + if (thread_state.unresolved_page_fault) { + + /* + * On base-foc it is necessary to pause the thread before + * IP and SP are available in the thread state. + */ + csc->pause(thread_cap); + + return csc->lwpid(thread_cap); + } + + } catch (Cpu_session::State_access_failed) { } + + thread_cap = csc->next(thread_cap); + } - while (thread_cap.valid()) { - Thread_state thread_state = csc->state(thread_cap); - if (thread_state.unresolved_page_fault) - return csc->lwpid(thread_cap); - thread_cap = csc->next(thread_cap); } - - PDBG("could not determine thread which caused the page fault"); - return 1; }