Concurrency protection for ntpmon using memory barrier instructions.

1 files changed, 49 insertions, 36 deletions

diff --git a/ntpshmread.c b/ntpshmread.c
index da449811..5d39dad6 100644
--- a/ntpshmread.c
+++ b/ntpshmread.c
@@ -17,6 +17,7 @@
 #include <stdint.h>
 
 #include "ntpshm.h"
+#include "compiler.h"
 
 struct shmTime *shm_get(const int unit, const bool create, const bool forall)
 /* initialize an initial segment */
@@ -53,41 +54,57 @@ char *shm_name(const int unit)
 enum segstat_t shm_query(struct shmTime *shm_in, struct shm_stat_t *shm_stat)
 /* try to grab a sample from the specified SHM segment */
 {
-    /* access order is important for lock-free SHM access; we
-    ** enforce order by treating the whole structure volatile.
-    **
-    ** IMPORTANT NOTE: This does not protect from reordering on CPU
-    ** level, and it does nothing for cache consistency and
-    ** visibility of changes by other cores. We need atomic and/or
-    ** fence instructions for that.
-    */
-    volatile struct shmTime *shm = shm_in;
+    volatile struct shmTime shmcopy, *shm = shm_in;
+    volatile int cnt;
 
     unsigned int cns_new, rns_new;
-    int cnt;
-
-    shm_stat->tvc.tv_sec = shm_stat->tvc.tv_nsec = 0;
-    clock_gettime(CLOCK_REALTIME, &shm_stat->tvc);
 
     if (shm == NULL) {
 	shm_stat->status = NO_SEGMENT;
 	return NO_SEGMENT;
     }
-    if (!shm->valid) {
+
+    shm_stat->tvc.tv_sec = shm_stat->tvc.tv_nsec = 0;
+    clock_gettime(CLOCK_REALTIME, &shm_stat->tvc);
+
+    /* relying on word access to be atomic here */
+    if (shm->valid == 0) {
 	shm_stat->status = NOT_READY;
 	return NOT_READY;
     }
 
+    cnt = shm->count;
+
+    /*
+     * This is proof against concurrency issues if either
+     * (a) the memory_barrier() call works on this host, or
+     * (b) memset compiles to an uninterruptible single-instruction bitblt.
+     */
+    memory_barrier();
+    memcpy((void *)&shmcopy, (void *)shm, sizeof(struct shmTime));
+    shm->valid = 0;
+    memory_barrier();
+
+    /* 
+     * Clash detection in case beither (a) nor (b) was true.
+     * Not supported in mode 0, and word access to the count field 
+     * must be atomic for this to work.
+     */
+    if (shmcopy.mode > 0 && cnt != shm->count) {
+	shm_stat->status = CLASH;
+	return shm_stat->status;
+    }
+
     shm_stat->status = OK;
 
-    switch (shm->mode) {
+    switch (shmcopy.mode) {
     case 0:
-	shm_stat->tvr.tv_sec	= shm->receiveTimeStampSec;
-	shm_stat->tvr.tv_nsec	= shm->receiveTimeStampUSec * 1000;
-	rns_new		= shm->receiveTimeStampNSec;
-	shm_stat->tvt.tv_sec	= shm->clockTimeStampSec;
-	shm_stat->tvt.tv_nsec	= shm->clockTimeStampUSec * 1000;
-	cns_new		= shm->clockTimeStampNSec;
+	shm_stat->tvr.tv_sec	= shmcopy.receiveTimeStampSec;
+	shm_stat->tvr.tv_nsec	= shmcopy.receiveTimeStampUSec * 1000;
+	rns_new		= shmcopy.receiveTimeStampNSec;
+	shm_stat->tvt.tv_sec	= shmcopy.clockTimeStampSec;
+	shm_stat->tvt.tv_nsec	= shmcopy.clockTimeStampUSec * 1000;
+	cns_new		= shmcopy.clockTimeStampNSec;
 
 	/* Since the following comparisons are between unsigned
 	** variables they are always well defined, and any
@@ -105,24 +122,20 @@ enum segstat_t shm_query(struct shmTime *shm_in, struct shm_stat_t *shm_stat)
 	    shm_stat->tvt.tv_nsec = cns_new;
 	    shm_stat->tvr.tv_nsec = rns_new;
 	}
-	/* At this point shm_stat->tvr and shm_stat->tvt contains valid ns-level
+	/* At this point shm_stat->tvr and shm_stat->tvt contain valid ns-level
 	** timestamps, possibly generated by extending the old
 	** us-level timestamps
 	*/
 	break;
 
     case 1:
-	cnt = shm->count;
-
-	shm_stat->tvr.tv_sec	= shm->receiveTimeStampSec;
-	shm_stat->tvr.tv_nsec	= shm->receiveTimeStampUSec * 1000;
-	rns_new		= shm->receiveTimeStampNSec;
-	shm_stat->tvt.tv_sec	= shm->clockTimeStampSec;
-	shm_stat->tvt.tv_nsec	= shm->clockTimeStampUSec * 1000;
-	cns_new		= shm->clockTimeStampNSec;
-	if (cnt != shm->count) {
-	    shm_stat->status = CLASH;
-	}
+
+	shm_stat->tvr.tv_sec	= shmcopy.receiveTimeStampSec;
+	shm_stat->tvr.tv_nsec	= shmcopy.receiveTimeStampUSec * 1000;
+	rns_new		= shmcopy.receiveTimeStampNSec;
+	shm_stat->tvt.tv_sec	= shmcopy.clockTimeStampSec;
+	shm_stat->tvt.tv_nsec	= shmcopy.clockTimeStampUSec * 1000;
+	cns_new		= shmcopy.clockTimeStampNSec;
 		
 	/* See the case above for an explanation of the
 	** following test.
@@ -142,14 +155,14 @@ enum segstat_t shm_query(struct shmTime *shm_in, struct shm_stat_t *shm_stat)
 	shm_stat->status = BAD_MODE;
 	break;
     }
+
     /*
      * leap field is not a leap offset but a leap notification code.
      * The values are magic numbers used by NTP and set by GPSD, if at all, in
      * the subframe code.
      */
-    shm_stat->leap = shm->leap;
-    shm_stat->precision = shm->precision;
-    shm->valid = 0;
+    shm_stat->leap = shmcopy.leap;
+    shm_stat->precision = shmcopy.precision;
 
     return shm_stat->status;
 }