Simplify iprop update locking and avoid deadlock

Since we are no longer treating the update log like a journal (#7552),
we don't need two-stage update logging.  In kdb5.c, add an update log
entry after each DB change in one step, without getting an explicit
lock.  In kdb_log.c, combine ulog_add_update with ulog_finish_update,
and make ulog_add_update lock the ulog internally.

This change avoids deadlock by removing the only cases where the ulog
is locked before the DB.

ticket: 7861

diff --git a/src/include/kdb_log.h b/src/include/kdb_log.h
index 16d8af2c6608c33094b193c2ce07af501c860af0..c61b285a4cf71b7212079fb1c367be80256c4668 100644 (file)
--- a/src/include/kdb_log.h
+++ b/src/include/kdb_log.h
@@ -71,8 +71,6 @@ krb5_error_code ulog_map(krb5_context context, const char *logname,
                          uint32_t entries, int caller, char **db_args);
 void ulog_init_header(krb5_context context);
 krb5_error_code ulog_add_update(krb5_context context, kdb_incr_update_t *upd);
-krb5_error_code ulog_finish_update(krb5_context context,
-                                   kdb_incr_update_t *upd);
 krb5_error_code ulog_get_entries(krb5_context context, const kdb_last_t *last,
                                  kdb_incr_result_t *ulog_handle);
 krb5_error_code ulog_replay(krb5_context context, kdb_incr_result_t *incr_ret,

diff --git a/src/lib/kdb/kdb5.c b/src/lib/kdb/kdb5.c
index 0af6a75f2fb9e876b6e68fd4eb20a0039094f216..b35d9ca5b7e04d53b6ab8784bd39657c67446997 100644 (file)
--- a/src/lib/kdb/kdb5.c
+++ b/src/lib/kdb/kdb5.c
@@ -884,23 +884,8 @@ krb5_error_code
 krb5_db_put_principal(krb5_context kcontext, krb5_db_entry *entry)
 {
     krb5_error_code status = 0;
-    kdb_vftabl *v;
-    char  **db_args = NULL;
     kdb_incr_update_t *upd = NULL;
     char *princ_name = NULL;
-    int ulog_locked = 0;
-
-    status = get_vftabl(kcontext, &v);
-    if (status)
-        return status;
-    if (v->put_principal == NULL)
-        return KRB5_PLUGIN_OP_NOTSUPP;
-
-    status = extract_db_args_from_tl_data(kcontext, &entry->tl_data,
-                                          &entry->n_tl_data,
-                                          &db_args);
-    if (status)
-        goto cleanup;
 
     if (logging(kcontext)) {
         upd = k5alloc(sizeof(*upd), &status);
@@ -909,30 +894,22 @@ krb5_db_put_principal(krb5_context kcontext, krb5_db_entry *entry)
         if ((status = ulog_conv_2logentry(kcontext, entry, upd)))
             goto cleanup;
 
-        status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
-        if (status != 0)
-            goto cleanup;
-        ulog_locked = 1;
-
         status = krb5_unparse_name(kcontext, entry->princ, &princ_name);
         if (status != 0)
             goto cleanup;
 
         upd->kdb_princ_name.utf8str_t_val = princ_name;
         upd->kdb_princ_name.utf8str_t_len = strlen(princ_name);
-
-        if ((status = ulog_add_update(kcontext, upd)) != 0)
-            goto cleanup;
     }
 
-    status = v->put_principal(kcontext, entry, db_args);
-    if (status == 0 && ulog_locked)
-        (void) ulog_finish_update(kcontext, upd);
+    status = krb5int_put_principal_no_log(kcontext, entry);
+    if (status)
+        goto cleanup;
+
+    if (logging(kcontext))
+        status = ulog_add_update(kcontext, upd);
 
 cleanup:
-    if (ulog_locked)
-        ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
-    free_db_args(kcontext, db_args);
     ulog_free_entries(upd, 1);
     return status;
 }
@@ -956,45 +933,23 @@ krb5_error_code
 krb5_db_delete_principal(krb5_context kcontext, krb5_principal search_for)
 {
     krb5_error_code status = 0;
-    kdb_vftabl *v;
     kdb_incr_update_t upd;
     char *princ_name = NULL;
-    int ulog_locked = 0;
 
-    status = get_vftabl(kcontext, &v);
-    if (status)
+    status = krb5int_delete_principal_no_log(kcontext, search_for);
+    if (status || !logging(kcontext))
         return status;
-    if (v->delete_principal == NULL)
-        return KRB5_PLUGIN_OP_NOTSUPP;
 
-    if (logging(kcontext)) {
-        status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
-        if (status)
-            return status;
-        ulog_locked = 1;
-
-        status = krb5_unparse_name(kcontext, search_for, &princ_name);
-        if (status)
-            goto cleanup;
-
-        (void) memset(&upd, 0, sizeof (kdb_incr_update_t));
-
-        upd.kdb_princ_name.utf8str_t_val = princ_name;
-        upd.kdb_princ_name.utf8str_t_len = strlen(princ_name);
-        upd.kdb_deleted = TRUE;
-
-        status = ulog_add_update(kcontext, &upd);
-        if (status)
-            goto cleanup;
-    }
+    status = krb5_unparse_name(kcontext, search_for, &princ_name);
+    if (status)
+        return status;
 
-    status = v->delete_principal(kcontext, search_for);
-    if (status == 0 && ulog_locked)
-        (void) ulog_finish_update(kcontext, &upd);
+    memset(&upd, 0, sizeof(kdb_incr_update_t));
+    upd.kdb_princ_name.utf8str_t_val = princ_name;
+    upd.kdb_princ_name.utf8str_t_len = strlen(princ_name);
+    upd.kdb_deleted = TRUE;
 
-cleanup:
-    if (ulog_locked)
-        ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
+    status = ulog_add_update(kcontext, &upd);
     free(princ_name);
     return status;
 }
@@ -2301,7 +2256,6 @@ krb5_db_create_policy(krb5_context kcontext, osa_policy_ent_t policy)
 {
     krb5_error_code status = 0;
     kdb_vftabl *v;
-    int ulog_locked = 0;
 
     status = get_vftabl(kcontext, &v);
     if (status)
@@ -2309,20 +2263,10 @@ krb5_db_create_policy(krb5_context kcontext, osa_policy_ent_t policy)
     if (v->create_policy == NULL)
         return KRB5_PLUGIN_OP_NOTSUPP;
 
-    if (logging(kcontext)) {
-        status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
-        if (status != 0)
-            return status;
-        ulog_locked = 1;
-    }
-
     status = v->create_policy(kcontext, policy);
     /* iprop does not support policy mods; force full resync. */
-    if (!status && ulog_locked)
+    if (!status && logging(kcontext))
         ulog_init_header(kcontext);
-
-    if (ulog_locked)
-        ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
     return status;
 }
 
@@ -2345,7 +2289,6 @@ krb5_db_put_policy(krb5_context kcontext, osa_policy_ent_t policy)
 {
     krb5_error_code status = 0;
     kdb_vftabl *v;
-    int ulog_locked = 0;
 
     status = get_vftabl(kcontext, &v);
     if (status)
@@ -2353,20 +2296,10 @@ krb5_db_put_policy(krb5_context kcontext, osa_policy_ent_t policy)
     if (v->put_policy == NULL)
         return KRB5_PLUGIN_OP_NOTSUPP;
 
-    if (logging(kcontext)) {
-        status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
-        if (status)
-            return status;
-        ulog_locked = 1;
-    }
-
     status = v->put_policy(kcontext, policy);
     /* iprop does not support policy mods; force full resync. */
-    if (!status && ulog_locked)
+    if (!status && logging(kcontext))
         ulog_init_header(kcontext);
-
-    if (ulog_locked)
-        ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
     return status;
 }
 
@@ -2390,7 +2323,6 @@ krb5_db_delete_policy(krb5_context kcontext, char *policy)
 {
     krb5_error_code status = 0;
     kdb_vftabl *v;
-    int ulog_locked = 0;
 
     status = get_vftabl(kcontext, &v);
     if (status)
@@ -2398,20 +2330,10 @@ krb5_db_delete_policy(krb5_context kcontext, char *policy)
     if (v->delete_policy == NULL)
         return KRB5_PLUGIN_OP_NOTSUPP;
 
-    if (logging(kcontext)) {
-        status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
-        if (status)
-            return status;
-        ulog_locked = 1;
-    }
-
     status = v->delete_policy(kcontext, policy);
     /* iprop does not support policy mods; force full resync. */
-    if (!status && ulog_locked)
+    if (!status && logging(kcontext))
         ulog_init_header(kcontext);
-
-    if (ulog_locked)
-        ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
     return status;
 }
 

diff --git a/src/lib/kdb/kdb_log.c b/src/lib/kdb/kdb_log.c
index 7e8514308b646166936a069ca5e6d12b82c78c5e..ca40a4fe52202dc58da09844f5b4099a5d1a2261 100644 (file)
--- a/src/lib/kdb/kdb_log.c
+++ b/src/lib/kdb/kdb_log.c
@@ -203,12 +203,13 @@ ulog_add_update(krb5_context context, kdb_incr_update_t *upd)
     int ulogfd;
 
     INIT_ULOG(context);
+    retval = ulog_lock(context, KRB5_LOCKMODE_EXCLUSIVE);
+    if (retval)
+        return retval;
+
     ulogentries = log_ctx->ulogentries;
     ulogfd = log_ctx->ulogfd;
 
-    if (upd == NULL)
-        return KRB5_LOG_ERROR;
-
     time_current(&ktime);
 
     upd_size = xdr_sizeof((xdrproc_t)xdr_kdb_incr_update_t, upd);
@@ -218,7 +219,7 @@ ulog_add_update(krb5_context context, kdb_incr_update_t *upd)
     if (recsize > ulog->kdb_block) {
         retval = resize(ulog, ulogentries, ulogfd, recsize);
         if (retval)
-            return retval;
+            goto cleanup;
     }
 
     /* If we have reached the last possible serial number, reinitialize the
@@ -226,30 +227,31 @@ ulog_add_update(krb5_context context, kdb_incr_update_t *upd)
     if (ulog->kdb_last_sno == (kdb_sno_t)-1)
         reset_header(ulog);
 
-    /* Get the next serial number and save it for finish_update() to index. */
-    cur_sno = ulog->kdb_last_sno + 1;
-    upd->kdb_entry_sno = cur_sno;
+    ulog->kdb_state = KDB_UNSTABLE;
 
+    /* Get the next serial number and find its update entry. */
+    cur_sno = ulog->kdb_last_sno + 1;
     i = (cur_sno - 1) % ulogentries;
     indx_log = INDEX(ulog, i);
 
     memset(indx_log, 0, ulog->kdb_block);
     indx_log->kdb_umagic = KDB_ULOG_MAGIC;
     indx_log->kdb_entry_size = upd_size;
-    indx_log->kdb_entry_sno = cur_sno;
+    indx_log->kdb_entry_sno = upd->kdb_entry_sno = cur_sno;
     indx_log->kdb_time = upd->kdb_time = ktime;
     indx_log->kdb_commit = upd->kdb_commit = FALSE;
 
-    ulog->kdb_state = KDB_UNSTABLE;
-
     xdrmem_create(&xdrs, (char *)indx_log->entry_data,
                   indx_log->kdb_entry_size, XDR_ENCODE);
-    if (!xdr_kdb_incr_update_t(&xdrs, upd))
-        return KRB5_LOG_CONV;
+    if (!xdr_kdb_incr_update_t(&xdrs, upd)) {
+        retval = KRB5_LOG_CONV;
+        goto cleanup;
+    }
 
+    indx_log->kdb_commit = TRUE;
     retval = sync_update(ulog, indx_log);
     if (retval)
-        return retval;
+        goto cleanup;
 
     if (ulog->kdb_num < ulogentries)
         ulog->kdb_num++;
@@ -269,37 +271,12 @@ ulog_add_update(krb5_context context, kdb_incr_update_t *upd)
         ulog->kdb_first_time = indx_log->kdb_time;
     }
 
-    ulog_sync_header(ulog);
-    return 0;
-}
-
-/* Mark the log entry as committed and sync the memory mapped log to file. */
-krb5_error_code
-ulog_finish_update(krb5_context context, kdb_incr_update_t *upd)
-{
-    krb5_error_code retval;
-    kdb_ent_header_t *indx_log;
-    unsigned int i;
-    kdb_log_context *log_ctx;
-    kdb_hlog_t *ulog = NULL;
-    uint32_t ulogentries;
-
-    INIT_ULOG(context);
-    ulogentries = log_ctx->ulogentries;
-
-    i = (upd->kdb_entry_sno - 1) % ulogentries;
-
-    indx_log = INDEX(ulog, i);
-    indx_log->kdb_commit = TRUE;
-
     ulog->kdb_state = KDB_STABLE;
 
-    retval = sync_update(ulog, indx_log);
-    if (retval)
-        return retval;
-
+cleanup:
     ulog_sync_header(ulog);
-    return 0;
+    ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
+    return retval;
 }
 
 /* Used by the slave to update its hash db from* the incr update log.  Must be