Fix a minor race in kdb5_util load

If a kdb5_util load gets killed between rename()ing the new KDB file
into place and resetting the iprop ulog then the ulog can reflect the
pre-load state, which will almost certainly be incorrect.

This matters because we want to impose a timeout on full resyncs in
kpropd when iprop dictates that a full resync is needed, and the
simplest timeout scheme involves signaling the kdb5_util load process.
But also, we want no such races in general.

The fix is simple: re-initialize the ulog before renaming the new KDB
file into place, then proceed as usual.  If the ulog is not properly
updated at the end of the load it will at least always result in
subsequent iprop get updates operations always indicating that a full
resync is required.

ticket: 7399

diff --git a/src/include/kdb_log.h b/src/include/kdb_log.h
index c8d028888ac8ab4f383759b6147f4f2dbc5ba1e3..14dbb25659f98ed54ec3cf296990e70f4af73589 100644 (file)
--- a/src/include/kdb_log.h
+++ b/src/include/kdb_log.h
@@ -49,6 +49,7 @@ extern "C" {
 #define FKPROPLOG       2
 #define FKPROPD         3
 #define FKCOMMAND       4       /* Includes kadmin.local and kdb5_util */
+#define FKLOAD          5       /* kdb5_util load */
 
 /*
  * Default ulog file attributes

diff --git a/src/kadmin/dbutil/dump.c b/src/kadmin/dbutil/dump.c
index 63f48f31020a69e6a0f2d6a73d0a589692b9d41d..b15e116f37e37e05d3c666c0fdd6fc77cbd8194a 100644 (file)
--- a/src/kadmin/dbutil/dump.c
+++ b/src/kadmin/dbutil/dump.c
@@ -2687,7 +2687,7 @@ load_db(argc, argv)
             if (log_ctx && log_ctx->iproprole) {
                 load = &iprop_version;
                 add_update = FALSE;
-                caller = FKPROPD;
+                caller = FKLOAD;
             } else {
                 fprintf(stderr, _("Iprop not enabled\n"));
                 exit_status++;

diff --git a/src/lib/kdb/kdb_log.c b/src/lib/kdb/kdb_log.c
index b800fa6fe21db434863c42e6df0e1ee4dcc78cda..4ecc34057cd3ff62be5dfe0271ba8d4dd3d766f2 100644 (file)
--- a/src/lib/kdb/kdb_log.c
+++ b/src/lib/kdb/kdb_log.c
@@ -536,12 +536,50 @@ error:
     return (retval);
 }
 
+static void
+ulog_reset(kdb_hlog_t *ulog)
+{
+    (void) memset(ulog, 0, sizeof (*ulog));
+    ulog->kdb_hmagic = KDB_ULOG_HDR_MAGIC;
+    ulog->db_version_num = KDB_VERSION;
+    ulog->kdb_state = KDB_STABLE;
+    ulog->kdb_block = ULOG_BLOCK;
+}
+
 /*
  * Map the log file to memory for performance and simplicity.
  *
  * Called by: if iprop_enabled then ulog_map();
  * Assumes that the caller will terminate on ulog_map, hence munmap and
  * closing of the fd are implicitly performed by the caller.
+ *
+ * Semantics for various values of caller:
+ *
+ *  - FKPROPLOG
+ *
+ *    Don't create if it doesn't exist, map as MAP_PRIVATE.
+ *
+ *  - FKPROPD
+ *
+ *    Create and initialize if need be, map as MAP_SHARED.
+ *
+ *  - FKLOAD
+ *
+ *    Create if need be, initialize (even if the ulog was already present), map
+ *    as MAP_SHARED.  (Intended for kdb5_util load of iprop dump.)
+ *
+ *  - FKCOMMAND
+ *
+ *    Create and [re-]initialize if need be, size appropriately, map as
+ *    MAP_SHARED.  (Intended for kdb5_util create and kdb5_util load of
+ *    non-iprop dump.)
+ *
+ *  - FKADMIN
+ *
+ *    Create and [re-]initialize if need be, size appropriately, map as
+ *    MAP_SHARED, and check consistency and recover as necessary.  (Intended
+ *    for kadmind and kadmin.local.)
+ *
  * Returns 0 on success else failure.
  */
 krb5_error_code
@@ -566,7 +604,8 @@ ulog_map(krb5_context context, const char *logname, uint32_t ulogentries,
             return (errno);
         }
 
-        if ((ulogfd = open(logname, O_RDWR+O_CREAT, 0600)) == -1) {
+        ulogfd = open(logname, O_RDWR | O_CREAT, 0600);
+        if (ulogfd == -1) {
             return (errno);
         }
 
@@ -625,28 +664,30 @@ ulog_map(krb5_context context, const char *logname, uint32_t ulogentries,
     log_ctx->ulogentries = ulogentries;
     log_ctx->ulogfd = ulogfd;
 
-    if (ulog->kdb_hmagic != KDB_ULOG_HDR_MAGIC) {
-        if (ulog->kdb_hmagic == 0) {
-            /*
-             * New update log
-             */
-            (void) memset(ulog, 0, sizeof (kdb_hlog_t));
+    retval = ulog_lock(context, KRB5_LOCKMODE_EXCLUSIVE);
+    if (retval)
+        return retval;
 
-            ulog->kdb_hmagic = KDB_ULOG_HDR_MAGIC;
-            ulog->db_version_num = KDB_VERSION;
-            ulog->kdb_state = KDB_STABLE;
-            ulog->kdb_block = ULOG_BLOCK;
-            if (!(caller == FKPROPLOG))
-                ulog_sync_header(ulog);
-        } else {
-            return (KRB5_LOG_CORRUPT);
-        }
+    if (ulog->kdb_hmagic != KDB_ULOG_HDR_MAGIC && ulog->kdb_hmagic != 0) {
+        ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
+        return (KRB5_LOG_CORRUPT);
+    }
+
+    if (ulog->kdb_hmagic != KDB_ULOG_HDR_MAGIC || caller == FKLOAD) {
+        ulog_reset(ulog);
+        if (caller != FKPROPLOG)
+            ulog_sync_header(ulog);
+        ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
+        return (0);
+    }
+
+    if ((caller == FKPROPLOG) || (caller == FKPROPD)) {
+        /* kproplog and kpropd don't need to do anything else. */
+        ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
+        return (0);
     }
 
     if (caller == FKADMIND) {
-        retval = ulog_lock(context, KRB5_LOCKMODE_EXCLUSIVE);
-        if (retval)
-            return retval;
         switch (ulog->kdb_state) {
         case KDB_STABLE:
         case KDB_UNSTABLE:
@@ -655,9 +696,8 @@ ulog_map(krb5_context context, const char *logname, uint32_t ulogentries,
              */
             retval = ulog_check(context, ulog, db_args);
             ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
-            if (retval == KRB5_LOG_CORRUPT) {
+            if (retval)
                 return (retval);
-            }
             break;
         case KDB_CORRUPT:
             ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
@@ -669,32 +709,19 @@ ulog_map(krb5_context context, const char *logname, uint32_t ulogentries,
             ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
             return (KRB5_LOG_ERROR);
         }
-    } else if ((caller == FKPROPLOG) || (caller == FKPROPD)) {
-        /*
-         * kproplog and kpropd don't need to do anything else
-         */
-        return (0);
     }
+    assert(caller == FKADMIND || caller == FKCOMMAND);
 
     /*
      * Reinit ulog if the log is being truncated or expanded after
      * we have circled.
      */
-    retval = ulog_lock(context, KRB5_LOCKMODE_EXCLUSIVE);
-    if (retval)
-        return retval;
     if (ulog->kdb_num != ulogentries) {
         if ((ulog->kdb_num != 0) &&
             ((ulog->kdb_last_sno > ulog->kdb_num) ||
              (ulog->kdb_num > ulogentries))) {
 
-            (void) memset(ulog, 0, sizeof (kdb_hlog_t));
-
-            ulog->kdb_hmagic = KDB_ULOG_HDR_MAGIC;
-            ulog->db_version_num = KDB_VERSION;
-            ulog->kdb_state = KDB_STABLE;
-            ulog->kdb_block = ULOG_BLOCK;
-
+            ulog_reset(ulog);
             ulog_sync_header(ulog);
         }