gpsd bug 15348, ntpshm interlocks

[Håkan Johansson-5]

I believe the bug report 15348 and fix to be correct.  What one may append
to the discussion is that the variable 'valid' is dual-use:

- make sure that the same measurement is not used twice, ensured as ntpd
   clears it after each use.

- make sure that ntpd cannot read values while gpsd is making an update.
   (this is new)

A sequence of events which realistically (but with low probability) could
trigger such a thing:

- NTPD makes a full read, sets valid = 0.
   (shortly thereafter:)
- GPSD writes a new value, finishing by setting valid = 1
   (almost 1 s passes...)
   (then by chance, GPSD makes it before NTPD:)
- GPSD starts to write values, increases count, valid is still 1
   (during the write operation, the write is interrupted:)
- NTPD makes a full read.  it can, as valid is 1 and count does not change
   sets valid = 0.
- GPSD gets back, and finishes the write, increases count, sets valid to 1

and from this point, it can actually continue, goto 'almost 1 s passes'.
ntpd could thus get a continous stream of bad, but correlated, data.

Patch works for me.

Cheers,
Håkan
[shmntp_locking.diff]

---

Index: ntpshm.c
===================================================================
--- ntpshm.c (revision 6417)
+++ ntpshm.c (arbetskopia)
@@ -128,6 +128,7 @@
     (void)gettimeofday(&tv,NULL);
     microseconds = 1000000.0 * modf(fixtime,&seconds);
 
+    shmTime->valid = 0;
     shmTime->count++;
     shmTime->clockTimeStampSec = (time_t)seconds;
     shmTime->clockTimeStampUSec = (int)microseconds;
@@ -195,6 +196,7 @@
  }
     }
 
+    shmTimeP->valid = 0;
     shmTimeP->count++;
     shmTimeP->clockTimeStampSec = seconds;
     shmTimeP->clockTimeStampUSec = 0;

---

_______________________________________________
Gpsd-dev mailing list
Gpsd-dev@...
https://lists.berlios.de/mailman/listinfo/gpsd-dev

[Håkan Johansson-5]

applied, then reverted, with statement

"This patch is invalid.  There are two possible modes to shmTime, mode 0
and mode 1.  We use mode 1, while the patch thinks we use mode 0.

Reference the ntpd source code, refclock_shm.c, line 70."

Which I presume would be the following:

         int    mode; /* 0 - if valid set
                       *       use values,
                       *       clear valid
                       * 1 - if valid set
                       *       if count before and after read of values is equal,
                       *         use values
                       *       clear valid
                       */

What is wrong?

Håkan


On Thu, 5 Nov 2009, Håkan Johansson wrote:

>
> I believe the bug report 15348 and fix to be correct.  What one may append
> to the discussion is that the variable 'valid' is dual-use:
>
> - make sure that the same measurement is not used twice, ensured as ntpd
>   clears it after each use.
>
> - make sure that ntpd cannot read values while gpsd is making an update.
>   (this is new)
>
> A sequence of events which realistically (but with low probability) could
> trigger such a thing:
>
> - NTPD makes a full read, sets valid = 0.
>   (shortly thereafter:)
> - GPSD writes a new value, finishing by setting valid = 1
>   (almost 1 s passes...)
>   (then by chance, GPSD makes it before NTPD:)
> - GPSD starts to write values, increases count, valid is still 1
>   (during the write operation, the write is interrupted:)
> - NTPD makes a full read.  it can, as valid is 1 and count does not change
>   sets valid = 0.
> - GPSD gets back, and finishes the write, increases count, sets valid to 1
>
> and from this point, it can actually continue, goto 'almost 1 s passes'.
> ntpd could thus get a continous stream of bad, but correlated, data.
>
> Patch works for me.
>
> Cheers,
> Håkan

_______________________________________________
Gpsd-dev mailing list
Gpsd-dev@...
https://lists.berlios.de/mailman/listinfo/gpsd-dev

[Gary E. Miller]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Yo Håkan!

On Thu, 12 Nov 2009, Håkan Johansson wrote:

Not reverted, yet.  Spent all day reverting things other people have
done (work and gpsd).  If people would just slow down they would end
up going a lot faster.

> What is wrong?

Read this:

>                       * 1 - if valid set
>                       *       if count before and after read of values is
> equal,
>                       *         use values
>                       *       clear valid
>                       */

Then read how the code works, knowing we use mode 1.  The read your
patch comment.  Think about it and you will see why your reasoning is
invalid.

RGDS
GARY
- ---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
        gem@...  Tel:[phone]

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)

iD8DBQFK+1BIBmnRqz71OvMRAqS2AJ9YMgJ2xV34p+zuBdutOhWEhU7wwQCfYpne
yppABLSIn8vHfzyKIhiEVt4=
=ciGP
-----END PGP SIGNATURE-----

_______________________________________________
Gpsd-dev mailing list
Gpsd-dev@...
https://lists.berlios.de/mailman/listinfo/gpsd-dev

[Gary E. Miller]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Yo Håkan!

I take it back.

On further study I can see no harm in your valid patch and it just may
prevent a once in several billion seconds race condition if the CPU is
horribly overloaded as to be otherwise useless for time keeping.  So I
added some comments on how the protocol works.

RGDS
GARY
- ---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
        gem@...  Tel:[phone]

On Wed, 11 Nov 2009, Gary E. Miller wrote:

> Date: Wed, 11 Nov 2009 16:01:08 -0800 (PST)
> From: Gary E. Miller <gem@...>
> To: "gpsd-dev@..." <gpsd-dev@...>
> Subject: Re: [Gpsd-dev] gpsd bug 15348, ntpshm interlocks
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Yo Håkan!
>
> On Thu, 12 Nov 2009, Håkan Johansson wrote:
>
> Not reverted, yet.  Spent all day reverting things other people have
> done (work and gpsd).  If people would just slow down they would end
> up going a lot faster.
>
> > What is wrong?
>
> Read this:
>
> >                       * 1 - if valid set
> >                       *       if count before and after read of values is
> > equal,
> >                       *         use values
> >                       *       clear valid
> >                       */
>
> Then read how the code works, knowing we use mode 1.  The read your
> patch comment.  Think about it and you will see why your reasoning is
> invalid.
>
> RGDS
> GARY
> - ---------------------------------------------------------------------------
> Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
> gem@...  Tel:[phone]
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.3 (GNU/Linux)
>
> iD8DBQFK+1BIBmnRqz71OvMRAqS2AJ9YMgJ2xV34p+zuBdutOhWEhU7wwQCfYpne
> yppABLSIn8vHfzyKIhiEVt4=
> =ciGP
> -----END PGP SIGNATURE-----
>

-----BEGIN PGP SIGNATURE-----

Version: GnuPG v1.4.3 (GNU/Linux)

iD8DBQFK+1kQBmnRqz71OvMRAuftAJ48zducaa7C3YhHkOOgSc0OV1+KJQCgq+ib
9Uvq6uwkjExj9Cr8Qr2ACnw=
=2xww
-----END PGP SIGNATURE-----

_______________________________________________
Gpsd-dev mailing list
Gpsd-dev@...
https://lists.berlios.de/mailman/listinfo/gpsd-dev