krb5 commit: Simplify iprop update locking and avoid deadlock
Greg Hudson
ghudson at MIT.EDU
Thu Feb 20 16:39:34 EST 2014
https://github.com/krb5/krb5/commit/444ef5fe9ec8d64a5db27b3a8aaf6813dd7ef0e0
commit 444ef5fe9ec8d64a5db27b3a8aaf6813dd7ef0e0
Author: Greg Hudson <ghudson at mit.edu>
Date: Wed Feb 12 19:13:43 2014 -0500
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
src/include/kdb_log.h | 2 -
src/lib/kdb/kdb5.c | 116 ++++++++-----------------------------------------
src/lib/kdb/kdb_log.c | 59 ++++++++-----------------
3 files changed, 37 insertions(+), 140 deletions(-)
diff --git a/src/include/kdb_log.h b/src/include/kdb_log.h
index 16d8af2..c61b285 100644
--- 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 0af6a75..b35d9ca 100644
--- 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 7e85143..ca40a4f 100644
--- 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
More information about the cvs-krb5
mailing list