From 28e3a979dbe3aa1d08ae32056b772a6a59fae581 Mon Sep 17 00:00:00 2001

From: Thierry Bordaz <tbordaz@redhat.com>

Date: Tue, 5 May 2020 17:44:01 +0200

Subject: [PATCH 1/2] Ticket 51068 - deadlock when updating the schema

Bug Description:

        It exists a 3 threads deadlock scenario. It involves state change plugins when it

        calls schema_changed_callback. So the trigger is a change of schema (direct or via

        replication). The scenario is

      MOD(cn=schema)    hold StateChange lock   wait for vattr lock

      SRCH              hold vattr lock         wait for DB page

      MOD               hold DB page            wait for StateChange lock

Fix Description:

        Statechange lock protects the list of registered callbacks.

        lock is a mutex where actually registration of callback is only done

        at startup. Later the list is only lookup.

        Making statechange lock a rwlock suppresses the deadlock scenario

        as MODs will only acquire in read StateChange lock.

        It should also improve performance as at the moment all MODs are serialized

        on that lock

	In order to prevent writer starvation a new slapi_new_rwlock_prio

        create rwlock with priority to writers.

https://pagure.io/389-ds-base/issue/51068

Reviewed by: Mark Reynolds, William Brown

Platforms tested: 30

Flag Day: no

Doc impact: no

---

 .../servers/plugins/statechange/statechange.c | 24 ++++++++++--------

 ldap/servers/slapd/slapi-plugin.h             | 16 ++++++++++++

 ldap/servers/slapd/slapi2nspr.c               | 25 +++++++++++++++++++

 ldap/servers/slapd/vattr.c                    |  2 +-

 4 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/ldap/servers/plugins/statechange/statechange.c b/ldap/servers/plugins/statechange/statechange.c

index f89b394c6..0a3838b5e 100644

--- a/ldap/servers/plugins/statechange/statechange.c

+++ b/ldap/servers/plugins/statechange/statechange.c

@@ -40,7 +40,7 @@ static SCNotify *head; /* a place to start in the list */

 #define SCN_PLUGIN_SUBSYSTEM "statechange-plugin" /* used for logging */

 static void *api[5];

-static Slapi_Mutex *buffer_lock = 0;

+static Slapi_RWLock *buffer_lock = 0;

 static PRUint64 g_plugin_started = 0;

 /*

@@ -140,7 +140,7 @@ statechange_start(Slapi_PBlock *pb __attribute__((unused)))

     api[3] = (void *)_statechange_unregister_all;

     api[4] = (void *)_statechange_vattr_cache_invalidator_callback;

-    if (0 == (buffer_lock = slapi_new_mutex())) /* we never free this mutex */

+    if (0 == (buffer_lock = slapi_new_rwlock()))

     {

         /* badness */

         slapi_log_err(SLAPI_LOG_ERR, SCN_PLUGIN_SUBSYSTEM, "statechange_start - Failed to create lock\n");

@@ -180,7 +180,9 @@ statechange_close(Slapi_PBlock *pb __attribute__((unused)))

     slapi_counter_destroy(&op_counter);

     slapi_apib_unregister(StateChange_v1_0_GUID);

-    slapi_destroy_mutex(buffer_lock);

+    if (buffer_lock) {

+        slapi_destroy_rwlock(buffer_lock);

+    }

     buffer_lock = NULL;

     slapi_log_err(SLAPI_LOG_TRACE, SCN_PLUGIN_SUBSYSTEM, "<-- statechange_close\n");

@@ -240,7 +242,7 @@ statechange_post_op(Slapi_PBlock *pb, int modtype)

     slapi_log_err(SLAPI_LOG_TRACE, SCN_PLUGIN_SUBSYSTEM, "--> statechange_post_op\n");

     /* evaluate this operation against the notification entries */

-    slapi_lock_mutex(buffer_lock);

+    slapi_rwlock_rdlock(buffer_lock);

     if (head) {

         slapi_pblock_get(pb, SLAPI_TARGET_SDN, &sdn;;

         if (NULL == sdn) {

@@ -290,7 +292,7 @@ statechange_post_op(Slapi_PBlock *pb, int modtype)

         } while (notify && notify != head);

     }

 bail:

-    slapi_unlock_mutex(buffer_lock);

+    slapi_rwlock_unlock(buffer_lock);

     slapi_log_err(SLAPI_LOG_TRACE, SCN_PLUGIN_SUBSYSTEM, "<-- statechange_post_op\n");

     return SLAPI_PLUGIN_SUCCESS; /* always succeed */

@@ -338,7 +340,7 @@ _statechange_register(char *caller_id, char *dn, char *filter, void *caller_data

         }

         item->func = func;

-        slapi_lock_mutex(buffer_lock);

+        slapi_rwlock_wrlock(buffer_lock);

         if (head == NULL) {

             head = item;

             head->next = head;

@@ -349,7 +351,7 @@ _statechange_register(char *caller_id, char *dn, char *filter, void *caller_data

             head->prev = item;

             item->prev->next = item;

         }

-        slapi_unlock_mutex(buffer_lock);

+        slapi_rwlock_unlock(buffer_lock);

         slapi_ch_free_string(&writable_filter);

         ret = SLAPI_PLUGIN_SUCCESS;

@@ -371,7 +373,7 @@ _statechange_unregister(char *dn, char *filter, notify_callback thefunc)

         return ret;

     }

-    slapi_lock_mutex(buffer_lock);

+    slapi_rwlock_wrlock(buffer_lock);

     if ((func = statechange_find_notify(dn, filter, thefunc))) {

         func->prev->next = func->next;

@@ -392,7 +394,7 @@ _statechange_unregister(char *dn, char *filter, notify_callback thefunc)

         slapi_ch_free((void **)&func);

     }

-    slapi_unlock_mutex(buffer_lock);

+    slapi_rwlock_unlock(buffer_lock);

     slapi_counter_decrement(op_counter);

     return ret;

@@ -410,7 +412,7 @@ _statechange_unregister_all(char *caller_id, caller_data_free_callback callback)

         return;

     }

-    slapi_lock_mutex(buffer_lock);

+    slapi_rwlock_wrlock(buffer_lock);

     if (notify) {

         do {

@@ -441,7 +443,7 @@ _statechange_unregister_all(char *caller_id, caller_data_free_callback callback)

         } while (notify != start_notify && notify != NULL);

     }

-    slapi_unlock_mutex(buffer_lock);

+    slapi_rwlock_unlock(buffer_lock);

     slapi_counter_decrement(op_counter);

 }

diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h

index 679bdbb5c..865d83b9b 100644

--- a/ldap/servers/slapd/slapi-plugin.h

+++ b/ldap/servers/slapd/slapi-plugin.h

@@ -6126,6 +6126,22 @@ void slapi_destroy_condvar(Slapi_CondVar *cvar);

 int slapi_wait_condvar(Slapi_CondVar *cvar, struct timeval *timeout);

 int slapi_notify_condvar(Slapi_CondVar *cvar, int notify_all);

+/**

+ * Creates a new read/write lock

+ * If prio_writer the rwlock gives priority on writers

+ * else it give priority on readers (default)

+ *

+ * \return A pointer to a \c Slapi_RWLock

+ *

+ * \note Free the returned lock by calling slapi_destroy_rwlock() when finished

+ *

+ * \see slapi_destroy_rwlock()

+ * \see slapi_rwlock_rdlock()

+ * \see slapi_rwlock_wrlock()

+ * \see slapi_rwlock_unlock()

+ */

+Slapi_RWLock *slapi_new_rwlock_prio(int32_t prio_writer);

+

 /**

  * Creates a new read/write lock.

  *

diff --git a/ldap/servers/slapd/slapi2nspr.c b/ldap/servers/slapd/slapi2nspr.c

index b3e6d94c2..232d1599e 100644

--- a/ldap/servers/slapd/slapi2nspr.c

+++ b/ldap/servers/slapd/slapi2nspr.c

@@ -181,6 +181,31 @@ slapi_notify_condvar(Slapi_CondVar *cvar, int notify_all)

     return (prrc == PR_SUCCESS ? 1 : 0);

 }

+Slapi_RWLock *

+slapi_new_rwlock_prio(int32_t prio_writer)

+{

+#ifdef USE_POSIX_RWLOCKS

+    pthread_rwlock_t *rwlock = NULL;

+    pthread_rwlockattr_t attr;

+

+    pthread_rwlockattr_init(&attr);

+    if (prio_writer) {

+        pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);

+    } else {

+        pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_READER_NP);

+    }

+

+    rwlock = (pthread_rwlock_t *)slapi_ch_malloc(sizeof(pthread_rwlock_t));

+    if (rwlock) {

+        pthread_rwlock_init(rwlock, &attr);

+    }

+

+    return ((Slapi_RWLock *)rwlock);

+#else

+    return ((Slapi_RWLock *)PR_NewRWLock(PR_RWLOCK_RANK_NONE, "slapi_rwlock"));

+#endif

+}

+

 Slapi_RWLock *

 slapi_new_rwlock(void)

 {

diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c

index 852a887ce..eef444270 100644

--- a/ldap/servers/slapd/vattr.c

+++ b/ldap/servers/slapd/vattr.c

@@ -1996,7 +1996,7 @@ vattr_map_create(void)

         return ENOMEM;

     }

-    the_map->lock = slapi_new_rwlock();

+    the_map->lock = slapi_new_rwlock_prio(1 /* priority on writers */);

     if (NULL == the_map) {

         slapd_nasty(sourcefile, 3, 0);

         return ENOMEM;

-- 

2.25.4