8308387: CLD created and unloading list sharing _next node pointer leads to concurrent YC missing CLD roots

Reviewed-by: stefank, coleenp, dholmes, eosterlund

Author: xmas92

src/hotspot/share/classfile/classLoaderData.cpp

@@ -148,6 +148,7 @@ ClassLoaderData::ClassLoaderData(Handle h_class_loader, bool has_class_mirror_ho
   _jmethod_ids(nullptr),
   _deallocate_list(nullptr),
   _next(nullptr),
+  _unloading_next(nullptr),
   _class_loader_klass(nullptr), _name(nullptr), _name_and_id(nullptr) {
 
   if (!h_class_loader.is_null()) {

src/hotspot/share/classfile/classLoaderData.hpp

@@ -153,15 +153,41 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   GrowableArray<Metadata*>*      _deallocate_list;
 
   // Support for walking class loader data objects
-  ClassLoaderData* _next; /// Next loader_datas created
+  //
+  // The ClassLoaderDataGraph maintains two lists to keep track of CLDs.
+  //
+  // The first list [_head, _next] is where new CLDs are registered. The CLDs
+  // are only inserted at the _head, and the _next pointers are only rewritten
+  // from unlink_next() which unlinks one unloading CLD by setting _next to
+  // _next->_next. This allows GCs to concurrently walk the list while the CLDs
+  // are being concurrently unlinked.
+  //
+  // The second list [_unloading_head, _unloading_next] is where dead CLDs get
+  // moved to during class unloading. See: ClassLoaderDataGraph::do_unloading().
+  // This list is never modified while other threads are iterating over it.
+  //
+  // After all dead CLDs have been moved to the unloading list, there's a
+  // synchronization point (handshake) to ensure that all threads reading these
+  // CLDs finish their work. This ensures that we don't have a use-after-free
+  // when we later delete the CLDs.
+  //
+  // And finally, when no threads are using the unloading CLDs anymore, we
+  // remove them from the class unloading list and delete them. See:
+  // ClassLoaderDataGraph::purge();
+  ClassLoaderData* _next;
+  ClassLoaderData* _unloading_next;
 
   Klass*  _class_loader_klass;
   Symbol* _name;
   Symbol* _name_and_id;
   JFR_ONLY(DEFINE_TRACE_ID_FIELD;)
 
-  void set_next(ClassLoaderData* next) { Atomic::store(&_next, next); }
-  ClassLoaderData* next() const        { return Atomic::load(&_next); }
+  void set_next(ClassLoaderData* next);
+  ClassLoaderData* next() const;
+  void unlink_next();
+
+  void set_unloading_next(ClassLoaderData* unloading_next);
+  ClassLoaderData* unloading_next() const;
 
   ClassLoaderData(Handle h_class_loader, bool has_class_mirror_holder);
   ~ClassLoaderData();

src/hotspot/share/classfile/classLoaderData.inline.hpp

@@ -33,6 +33,29 @@
 #include "oops/oopHandle.inline.hpp"
 #include "oops/weakHandle.inline.hpp"
 
+inline void ClassLoaderData::set_next(ClassLoaderData* next) {
+  assert(this->next() == nullptr, "only link once");
+  Atomic::store(&_next, next);
+}
+
+inline ClassLoaderData* ClassLoaderData::next() const {
+  return Atomic::load(&_next);
+}
+
+inline void ClassLoaderData::unlink_next() {
+  assert(next()->is_unloading(), "only remove unloading clds");
+  Atomic::store(&_next, _next->_next);
+}
+
+inline void ClassLoaderData::set_unloading_next(ClassLoaderData* unloading_next) {
+  assert(this->unloading_next() == nullptr, "only link once");
+  _unloading_next = unloading_next;
+}
+
+inline ClassLoaderData* ClassLoaderData::unloading_next() const {
+  return _unloading_next;
+}
+
 inline oop ClassLoaderData::class_loader() const {
   assert(!_unloading, "This oop is not available to unloading class loader data");
   assert(_holder.is_null() || holder_no_keepalive() != nullptr , "This class loader data holder must be alive");

src/hotspot/share/classfile/classLoaderDataGraph.cpp

@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "classfile/classLoaderData.inline.hpp"
 #include "classfile/classLoaderDataGraph.inline.hpp"
 #include "classfile/dictionary.hpp"
 #include "classfile/javaClasses.hpp"
@@ -200,9 +201,9 @@ void ClassLoaderDataGraph::walk_metadata_and_clean_metaspaces() {
   clean_deallocate_lists(walk_all_metadata);
 }
 
-// GC root of class loader data created.
+// List head of all class loader data.
 ClassLoaderData* volatile ClassLoaderDataGraph::_head = nullptr;
-ClassLoaderData* ClassLoaderDataGraph::_unloading = nullptr;
+ClassLoaderData* ClassLoaderDataGraph::_unloading_head = nullptr;
 
 bool ClassLoaderDataGraph::_should_clean_deallocate_lists = false;
 bool ClassLoaderDataGraph::_safepoint_cleanup_needed = false;
@@ -268,16 +269,7 @@ inline void assert_is_safepoint_or_gc() {
          "Must be called by safepoint or GC");
 }
 
-void ClassLoaderDataGraph::cld_unloading_do(CLDClosure* cl) {
-  assert_is_safepoint_or_gc();
-  for (ClassLoaderData* cld = _unloading; cld != nullptr; cld = cld->next()) {
-    assert(cld->is_unloading(), "invariant");
-    cl->do_cld(cld);
-  }
-}
-
-// These are functions called by the GC, which require all of the CLDs, including the
-// unloading ones.
+// These are functions called by the GC, which require all of the CLDs, including not yet unlinked CLDs.
 void ClassLoaderDataGraph::cld_do(CLDClosure* cl) {
   assert_is_safepoint_or_gc();
   for (ClassLoaderData* cld = Atomic::load_acquire(&_head);  cld != nullptr; cld = cld->next()) {
@@ -430,7 +422,7 @@ void ClassLoaderDataGraph::loaded_classes_do(KlassClosure* klass_closure) {
 
 void ClassLoaderDataGraph::classes_unloading_do(void f(Klass* const)) {
   assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
-  for (ClassLoaderData* cld = _unloading; cld != nullptr; cld = cld->next()) {
+  for (ClassLoaderData* cld = _unloading_head; cld != nullptr; cld = cld->unloading_next()) {
     assert(cld->is_unloading(), "invariant");
     cld->classes_do(f);
   }
@@ -501,37 +493,32 @@ bool ClassLoaderDataGraph::is_valid(ClassLoaderData* loader_data) {
 bool ClassLoaderDataGraph::do_unloading() {
   assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
 
-  ClassLoaderData* data = _head;
   ClassLoaderData* prev = nullptr;
   bool seen_dead_loader = false;
   uint loaders_processed = 0;
   uint loaders_removed = 0;
 
-  data = _head;
-  while (data != nullptr) {
+  for (ClassLoaderData* data = _head; data != nullptr; data = data->next()) {
     if (data->is_alive()) {
       prev = data;
-      data = data->next();
       loaders_processed++;
-      continue;
-    }
-    seen_dead_loader = true;
-    loaders_removed++;
-    ClassLoaderData* dead = data;
-    dead->unload();
-    data = data->next();
-    // Remove from loader list.
-    // This class loader data will no longer be found
-    // in the ClassLoaderDataGraph.
-    if (prev != nullptr) {
-      prev->set_next(data);
     } else {
-      assert(dead == _head, "sanity check");
-      // The GC might be walking this concurrently
-      Atomic::store(&_head, data);
+      // Found dead CLD.
+      loaders_removed++;
+      seen_dead_loader = true;
+      data->unload();
+
+      // Move dead CLD to unloading list.
+      if (prev != nullptr) {
+        prev->unlink_next();
+      } else {
+        assert(data == _head, "sanity check");
+        // The GC might be walking this concurrently
+        Atomic::store(&_head, data->next());
+      }
+      data->set_unloading_next(_unloading_head);
+      _unloading_head = data;
     }
-    dead->set_next(_unloading);
-    _unloading = dead;
   }
 
   log_debug(class, loader, data)("do_unloading: loaders processed %u, loaders removed %u", loaders_processed, loaders_removed);
@@ -563,13 +550,13 @@ void ClassLoaderDataGraph::clean_module_and_package_info() {
 }
 
 void ClassLoaderDataGraph::purge(bool at_safepoint) {
-  ClassLoaderData* list = _unloading;
-  _unloading = nullptr;
+  ClassLoaderData* list = _unloading_head;
+  _unloading_head = nullptr;
   ClassLoaderData* next = list;
   bool classes_unloaded = false;
   while (next != nullptr) {
     ClassLoaderData* purge_me = next;
-    next = purge_me->next();
+    next = purge_me->unloading_next();
     delete purge_me;
     classes_unloaded = true;
   }

src/hotspot/share/classfile/classLoaderDataGraph.hpp

@@ -41,9 +41,10 @@ class ClassLoaderDataGraph : public AllStatic {
   friend class ClassLoaderDataGraphIteratorBase;
   friend class VMStructs;
  private:
-  // All CLDs (except the null CLD) can be reached by walking _head->_next->...
+  // All CLDs (except unlinked CLDs) can be reached by walking _head->_next->...
   static ClassLoaderData* volatile _head;
-  static ClassLoaderData* _unloading;
+  // All unlinked CLDs
+  static ClassLoaderData* _unloading_head;
 
   // Set if there's anything to purge in the deallocate lists or previous versions
   // during a safepoint after class unloading in a full GC.
@@ -67,9 +68,8 @@ class ClassLoaderDataGraph : public AllStatic {
   static void clear_claimed_marks();
   static void clear_claimed_marks(int claim);
   static void verify_claimed_marks_cleared(int claim);
-  // Iteration through CLDG inside a safepoint; GC support
+  // Iteration through CLDG; GC support
   static void cld_do(CLDClosure* cl);
-  static void cld_unloading_do(CLDClosure* cl);
   static void roots_cld_do(CLDClosure* strong, CLDClosure* weak);
   static void always_strong_cld_do(CLDClosure* cl);
   // Iteration through CLDG not by GC.