#73 ptpd v.1.1.0 offset filtering bug

[Fran Horan]

Version: 1.1.0

Summary: Offset test in 'updateClock()' operates on a signed value and a magnitude. The correction is to convert the signed value to a magnitude before the test operation.

Details:

The -O runtime option activates an offset filter on the slave side. The option help says:

-O do not reset the clock if offset is more than NUMBER nanoseconds

Assumption: The value entered via -O is a magnitude and not a signed value.

Note: The value entered with -O is then stored in 'maxReset'.

Looking in src/dep/servo.c you will find on line #153 a test operation on maxReset (a magnitude) and offset_from_master.nanoseconds (a signed value). If the Assumption above is correct, then line #153 should take the absolute value of offset_from_master.nanoseconds before performing the test operation.

The bug's impact is that a large negative offset is not filtered out as desired.

138 void
139 updateClock(RunTimeOpts * rtOpts, PtpClock * ptpClock)
140 {
141     Integer32 adj;
142     TimeInternal timeTmp;
143
144     DBGV("updateClock\n");
145
146     if (rtOpts->maxReset) { /* If maxReset is 0 then it's OFF */
147         if (ptpClock->offset_from_master.seconds) {
148             INFO("updateClock aborted, offset greater than 1"
149                  " second.");
150             goto display;
151         }
152
153         if (ptpClock->offset_from_master.nanoseconds >
154             rtOpts->maxReset) {
155             INFO("updateClock aborted, offset %d greater than "
156                  "administratively set maximum %d\n",
157                  ptpClock->offset_from_master.nanoseconds,
158                  rtOpts->maxReset);
159             goto display;
160         }
161     }

A runtime test confirms this finding.

Next, I looked in ptpd version 2.3.1 to see what is going on in this area. I find that the -O option has been re-assigned to a new purpose. The new option for offset filtering is servo:max_offset:

./src/ptpd2 -e servo:max_offset
setting: servo:max_offset (--servo:max_offset)
   type: INT (min: 0, max: 999999999)
  usage: Do not reset the clock if offset from master is greater
        than this value (nanoseconds). 0 = not used.
default: 0

This help printout defines a non-negative number that is supportive of the Assumption made in v1.1.0.

The value entered with the 'servo:max_offset' option is written into the 'maxOffset' global, and the comparable test in v2.3.1 is now using magnitudes on line #766. From src/dep/servo.c:

 766     if (rtOpts->maxOffset && ((abs(ptpClock->offsetFromMaster.nanoseconds) > abs(rtOpts->maxOffset)) ||
 767         ptpClock->offsetFromMaster.seconds)) {
 768         INFO("Offset %d.%09d greater than "
 769              "administratively set maximum %d\n. Will not update clock",
 770         ptpClock->offsetFromMaster.seconds,
 771         ptpClock->offsetFromMaster.nanoseconds, rtOpts->maxOffset);
 772         return;
 773     }

This matches the recommended correction being made for v1.1.0.

Fran Horan
JHU/APL