[PATCH] thread: make walking parent-child thread relationship safer

[Waldemar Kozaczuk]

This patch replaces _parent_link/thread_list with a simple parent_thread_id
field to make walking parent-child list safer in elf::visible() method.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
core/elf.cc | 21 +++++++++++++++++----
core/sched.cc | 8 +-------
include/osv/sched.hh | 20 +++++---------------
3 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index a25fb3c1..691c0d5e 100644
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -151,15 +151,29 @@ bool object::visible(void) const
return true;
}

+ auto current_thread = sched::thread::current();
if (level == VisibilityLevel::ThreadOnly) {
- return thread == sched::thread::current();
+ return thread == current_thread;
}

// Is current thread the same as "thread" or is it a child ?`
- for (auto t = sched::thread::current(); t != nullptr; t = t->parent()) {
- if (t == thread) {
+ if( thread == current_thread) {
+ return true;
+ }
+
+ auto thread_id = static_cast<sched::thread*>(thread)->id();
+ auto next_parent_id = current_thread->parent_id();
+ while (next_parent_id) {
+ if (next_parent_id == thread_id) {
return true;
}
+ sched::with_thread_by_id(next_parent_id, [&next_parent_id] (sched::thread *t) {
+ if (t) {
+ next_parent_id = t->parent_id();
+ } else {
+ next_parent_id = 0;
+ }
+ });
}
return false;
}
@@ -171,7 +185,6 @@ void object::set_visibility(elf::VisibilityLevel visibilityLevel)
_visibility_level.store(visibilityLevel, std::memory_order_release);
}

-
template <>
void* object::lookup(const char* symbol)
{
diff --git a/core/sched.cc b/core/sched.cc
index fde98432..06f849d1 100644
--- a/core/sched.cc
+++ b/core/sched.cc
@@ -1002,11 +1002,7 @@ thread::thread(std::function<void ()> func, attr attr, bool main, bool app)
sizeof(_attr._name) - 1);
}

- // Note that we never delete _parent_link. This is intentional as it lets us track
- // parent-child relationship safely even when given thread is destroyed.
- _parent_link = new thread_list();
- _parent_link->_self.store(this);
- _parent_link->_parent = (s_current && !main) ? s_current->_parent_link : nullptr;
+ _parent_id = s_current ? s_current->id() : 0;
}

static std::list<std::function<void ()>> exit_notifiers
@@ -1044,8 +1040,6 @@ thread::~thread()
{
cancel_this_thread_alarm();

- _parent_link->_self.store(nullptr, std::memory_order_release);
-
if (!_attr._detached) {
join();
}
diff --git a/include/osv/sched.hh b/include/osv/sched.hh
index 0c2aa143..0fb44f77 100644
--- a/include/osv/sched.hh
+++ b/include/osv/sched.hh
@@ -364,13 +364,6 @@ public:
return *this;
}
};
- // Simple single-linked list that allows to keep track of
- // "parent-child" relationship between threads. The "parent" is
- // the thread that constructs given "child" thread
- struct thread_list {
- std::atomic<thread*> _self;
- thread_list *_parent;
- };

private:
// Unlike the detached user visible attribute, those are internal to
@@ -640,7 +633,10 @@ public:
{
return static_cast<T*>(do_remote_thread_local_var(var));
}
- thread *parent();
+ unsigned int parent_id() const
+ {
+ return _parent_id;
+ }
private:
virtual void timer_fired() override;
struct detached_state;
@@ -777,7 +773,7 @@ private:
inline void cputime_estimator_get(
osv::clock::uptime::time_point &running_since,
osv::clock::uptime::duration &total_cpu_time);
- thread_list *_parent_link;
+ unsigned int _parent_id;
};

class thread_handle {
@@ -1304,12 +1300,6 @@ inline thread_handle thread::handle()
return thread_handle(*this);
}

-inline thread* thread::parent()
-{
- auto _parent = _parent_link->_parent;
- return _parent ? _parent->_self.load(std::memory_order_acquire) : nullptr;
-}
-
inline void* thread::get_tls(ulong module)
{
if (module >= _tls.size()) {
--
2.20.1

[Nadav Har'El]

Thanks. Looks mostly good, but I think there's one last potential unsafety we should fix:

What guarantees that after we calculated "thread" (by loading _visibility_thread), it remains alive long enough for this call to thread->id() to work?

I'm worried that it can't really be guaranteed. So I think it's safer not to dereference "thread" at all.

It's not hard to avoid it, actually. You just need to compare something to "thread" (not to its id). Exactly like in the old code - we iterate over a bunch of "t"'s (making sure to do it safely), comparing each one to the "thread" pointer. We only use ids to iterate safely.

It's easy to do - as follows:

 

+    auto next_parent_id = current_thread->parent_id();
+    while (next_parent_id) {
+        if (next_parent_id == thread_id) {

Instead of this check (which has the problem of needing "thread_id" as I said above), I suggest you can do the check inside the with_thread_by_id, as below:

             return true;
         }
+        sched::with_thread_by_id(next_parent_id, [&next_parent_id] (sched::thread *t) {

Here you can check if (t == thread) - replacing the above check you did above for for next_parent_id == thread_id.

In other words, inside the with_thread_by_id() you'll have code like this:

    if (t == thread) {

         return true;

    } else if (t == 0) {

         return false;

    } else {

         next_parent_id = t->parent_id();

     }

+            if (t) {

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20200112210914.24732-1-jwkozaczuk%40gmail.com.

[Waldemar Kozaczuk]

This patch replaces _parent_link/thread_list with a simple parent_thread_id
field to make walking parent-child list safer in elf::visible() method.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---

core/elf.cc | 27 +++++++++++++++++++--------

core/sched.cc | 8 +-------
include/osv/sched.hh | 20 +++++---------------

3 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index a25fb3c1..86ccc5d9 100644
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -151,17 +151,29 @@ bool object::visible(void) const

return true;
}

+ auto current_thread = sched::thread::current();
if (level == VisibilityLevel::ThreadOnly) {
- return thread == sched::thread::current();
+ return thread == current_thread;
}

- // Is current thread the same as "thread" or is it a child ?`

- for (auto t = sched::thread::current(); t != nullptr; t = t->parent()) {
- if (t == thread) {

- return true;
- }
+ // Is current thread the same as "thread" or is it a child of it?

+ if (thread == current_thread) {
+ return true;
}

- return false;
+
+ bool visible = false;

+ auto next_parent_id = current_thread->parent_id();
+ while (next_parent_id) {

+ sched::with_thread_by_id(next_parent_id, [&next_parent_id, &visible, thread] (sched::thread *t) {
+ if (t == thread) {
+ visible = true;
+ next_parent_id = 0;
+ } else {
+ next_parent_id = t ? t->parent_id() : 0;
+ }
+ });
+ }
+ return visible;

}

void object::set_visibility(elf::VisibilityLevel visibilityLevel)

@@ -171,7 +183,6 @@ void object::set_visibility(elf::VisibilityLevel visibilityLevel)

[Nadav Har'El]

Looks good. Thanks. I'll commit.

Oh, I forgot we can't "return" from inside the lambda :-(

You could have also used "break" here instead of "next_parent_id = 0". But never mind :-)

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20200112224434.13489-1-jwkozaczuk%40gmail.com.

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

thread: make walking parent-child thread relationship safer

This patch replaces _parent_link/thread_list with a simple parent_thread_id
field to make walking parent-child list safer in elf::visible() method.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

Message-Id: <20200112224434.1...@gmail.com>

---
diff --git a/core/elf.cc b/core/elf.cc

--- a/core/sched.cc
+++ b/core/sched.cc
@@ -1002,11 +1002,7 @@ thread::thread(std::function<void ()> func, attr
attr, bool main, bool app)
sizeof(_attr._name) - 1);
}

- // Note that we never delete _parent_link. This is intentional as it
lets us track
- // parent-child relationship safely even when given thread is
destroyed.
- _parent_link = new thread_list();
- _parent_link->_self.store(this);
- _parent_link->_parent = (s_current && !main) ?
s_current->_parent_link : nullptr;
+ _parent_id = s_current ? s_current->id() : 0;
}

static std::list<std::function<void ()>> exit_notifiers
@@ -1044,8 +1040,6 @@ thread::~thread()
{
cancel_this_thread_alarm();

- _parent_link->_self.store(nullptr, std::memory_order_release);
-
if (!_attr._detached) {
join();
}
diff --git a/include/osv/sched.hh b/include/osv/sched.hh