On Sat, Jan 11, 2020 at 8:00 PM Waldemar Kozaczuk <jwkozac...@gmail.com>
wrote:
> The commit fe37650fbbcd352806bab3e9e70949a65fadf29b to address
>
I didn't notice you committed it :-( Sorry about the slow reviews....
I thought you said it wasn't possible to keep a parent thread pointer -
because a parent thread pointer can go away at any time! I'll go review
that patch next (belatedly).
Anyway, I'll commit this patch, and if we need more fixes later we can do
them later.
the issue #1067 relaxed the elf visibility rules while being loaded
> to child threads so that they can resolve symbols if any threads
> are created by the INIT functions.
>
> Unfortunately inadvertently that patch allowed access to elf
> ebjects too early in some cases and would lead to a crash. Here is a
> concrete scenario when this would happen with the 'cli' app:
>
> 1. Thread A resolves objects of the httpserver app.
> 2. Thread A starts the httpserver app on new thread B.
> 3. Thread A continues to load objects of the cli app.
> 4. Thread B tries to resolve some symbols through elf_resolve_pltgot() for
> example
> and ends up attempting to look up symbols of cli object that is just
> being loaded
> by thread A and is not "ready" yet. However because thread B is a child
> of A
> it can do so.
>
> Clearly the example above illustrates a hole in the original patch.
> This patch fills this hole by restricting back the access to the symbols
> of the object being loaded to the thread doing it only as it used to be be.
> And it only relaxes the access to child threads for the brief time while
> object
> is being initialized.
>
> It does so by introducing the elf access visibility levels:
> - Public
> - ThreadOnly
> - ThreadAndItsChildren
>
> The Public and ThreadOnly and equivalent to the old - nullptr, non-nullptr
> visibility attribute.
> The ThreadAndItsChildren is only set to for the period the INIT functions
> are being called.
>
> References #1067
>
> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
> ---
> core/elf.cc | 31 ++++++++++++++++++++++---------
> include/osv/elf.hh | 11 +++++++++--
> 2 files changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/core/elf.cc b/core/elf.cc
> index 36466807..a25fb3c1 100644
> --- a/core/elf.cc
> +++ b/core/elf.cc
> @@ -122,7 +122,8 @@ object::object(program& prog, std::string pathname)
> , _dynamic_table(nullptr)
> , _module_index(_prog.register_dtv(this))
> , _is_executable(false)
> - , _visibility(nullptr)
> + , _visibility_thread(nullptr)
> + , _visibility_level(VisibilityLevel::Public)
> {
> elf_debug("Instantiated\n");
> }
> @@ -140,22 +141,34 @@ ulong object::module_index() const
>
> bool object::visible(void) const
> {
> - auto v = _visibility.load(std::memory_order_acquire);
> - if (v == nullptr) {
> + auto level = _visibility_level.load(std::memory_order_acquire);
> + if (level == VisibilityLevel::Public) {
> return true;
> }
> +
> + auto thread = _visibility_thread.load(std::memory_order_acquire);
> + if (!thread) { //Unlikely, but ...
> + return true;
> + }
> +
> + if (level == VisibilityLevel::ThreadOnly) {
> + return thread == sched::thread::current();
> + }
> +
> + // 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 == v) {
> + if (t == thread) {
> return true;
> }
> }
> return false;
> }
>
> -void object::setprivate(bool priv)
> +void object::set_visibility(elf::VisibilityLevel visibilityLevel)
> {
> - _visibility.store(priv ? sched::thread::current() : nullptr,
> + _visibility_thread.store(visibilityLevel == VisibilityLevel::Public ?
> nullptr : sched::thread::current(),
> std::memory_order_release);
> + _visibility_level.store(visibilityLevel, std::memory_order_release);
>
Nitpick (but we can fix this later, if at all) - I don't think you need to
use release order (and in the read - acquire) for both
fields. I think it's enough to do it for one. But since this is happens
rarely (usually you just read the level), I'll let this slide.
> }
>
>
> @@ -1323,7 +1336,7 @@ program::load_object(std::string name,
> std::vector<std::string> extra_path,
> auto ef = std::shared_ptr<object>(new file(*this, f, name),
> [=](object *obj) { remove_object(obj); });
> ef->set_base(_next_alloc);
> - ef->setprivate(true);
> + ef->set_visibility(ThreadOnly);
> // We need to push the object at the end of the list (so that the
> main
> // shared object gets searched before the shared libraries it
> uses),
> // with one exception: the kernel needs to remain at the end of
> the
> @@ -1395,13 +1408,13 @@ void program::init_library(int argc, char** argv)
> // first) and finally make the loaded objects visible in search
> order.
> auto size = loaded_objects.size();
> for (unsigned i = 0; i < size; i++) {
> - loaded_objects[i]->setprivate(true);
> + loaded_objects[i]->set_visibility(ThreadAndItsChildren);
> }
> for (int i = size - 1; i >= 0; i--) {
> loaded_objects[i]->run_init_funcs(argc, argv);
> }
> for (unsigned i = 0; i < size; i++) {
> - loaded_objects[i]->setprivate(false);
> + loaded_objects[i]->set_visibility(Public);
> }
> _loaded_objects_stack.pop();
> }
> diff --git a/include/osv/elf.hh b/include/osv/elf.hh
> index 8e37aebb..32f3eeaa 100644
> --- a/include/osv/elf.hh
> +++ b/include/osv/elf.hh
> @@ -329,6 +329,12 @@ struct [[gnu::packed]] Elf64_Shdr {
> Elf64_Xword sh_entsize; /* Size of entries, if section has table */
> };
>
> +enum VisibilityLevel {
> + Public,
> + ThreadOnly,
> + ThreadAndItsChildren
> +};
>
+
> class object: public std::enable_shared_from_this<elf::object> {
> public:
> explicit object(program& prog, std::string pathname);
> @@ -452,10 +458,11 @@ protected:
> return _static_tls_offset + get_tls_size();
> }
> private:
> - std::atomic<void*> _visibility;
> + std::atomic<void*> _visibility_thread;
> + std::atomic<VisibilityLevel> _visibility_level;
> bool visible(void) const;
> public:
> - void setprivate(bool);
> + void set_visibility(VisibilityLevel);
> };
>
> class file : public object {
> --
> 2.20.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/20200111180020.22335-1-jwkozaczuk%40gmail.com
> .
>
--
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+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/CANEVyjv-eQttQ1B38HBv-LFe1kGQ18F%2BJifYcjvSQBCPZia8WQ%40mail.gmail.com.
