Bug 6761 - adjtimex broken when delta is NULL
adjtimex broken when delta is NULL
Status: CLOSED CODE_FIX
Product: Timers
Classification: Unclassified
Component: Other
i386 Linux
: P2 normal
Assigned To: john stultz
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-28 06:07 UTC by Michael Kerrisk
Modified: 2007-12-11 13:26 UTC (History)
8 users (show)

See Also:
Kernel Version: 2.6.17
Tree: Mainline
Regression: ---


Attachments
Add ADJ_OFFSET_SS_READ (1002 bytes, patch)
2007-07-27 17:26 UTC, john stultz
Details | Diff

Description Michael Kerrisk 2006-06-28 06:07:18 UTC
Most recent kernel where this bug did not occur: occurs in all kernels (also in
2.6.17)
Distribution: n/a (most recently tested on SUSE 10.0)
Hardware Environment: x86 (and probably all others)
Software Environment:
Problem Description:
A long standing bug in the adjtimex() system call causes glibc's 
adjtime(3) function to deliver the wrong results if 'delta' is NULL.

The adjtime(3) manual page says 

       If olddelta is not NULL, then the buffer that it points to 
       is used to return the amount of time remaining from any 
       previous adjustment that has not yet been completed.

The FreeBSD manual pages says similar:

     If olddelta is not a null pointer, the structure pointed to 
     will contain, upon return, the number of microseconds still 
     to be corrected from the earlier call.

This information should be returned in 'olddelta' regardless of 
whether the 'delta' argument is or is not NULL.  However, Linux/glibc
only returns valid information in 'olddelta' if 'delta' is non-NULL;
in other words it is only possible to enquire about 'olddelta' if
we at the same time change 'delta'.  FreeBSD does not have this 
limitation.  

The problem lies in the implementation of adjtimex().  I earlier
suggested a fix for this problem:

--- time.c.orig	2006-03-12 11:03:10.000000000 +1300
+++ time.c	2006-03-12 11:04:26.000000000 +1300
@@ -375,7 +375,9 @@

 		result = TIME_ERROR;
 	
-	if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT)
+	if(txc->modes == 0)
+	    txc->offset    = time_adjust;
+	else if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT)
 	    txc->offset	   = save_adjust;
 	else {
 	    txc->offset = shift_right(time_offset, SHIFT_UPDATE);

(the patch line numbers are wrong for 2.6.17, but the fix is the same.)

I made that suggestion in 2002:

http://marc.theaimsgroup.com/?l=linux-kernel&m=102404614411225&w=2

Some follow up acknowledged the problem but implied that the fix is 
more complex (and put off until later):

http://marc.theaimsgroup.com/?l=linux-kernel&m=103674954619484&w=2
http://marc.theaimsgroup.com/?l=linux-kernel&m=103689681229548&w=2

But later has never come, and the problem still exists.  It should 
be fixed.  There is also a relevant glibc report that I made
to get further background on this point: 
http://sourceware.org/bugzilla/show_bug.cgi?id=2449 .

Steps to reproduce:

This problem can be demonstrated with the following program:


/* t_adjtime.c */

#define _GNU_SOURCE
#include <sys/time.h>
#include <sys/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

int
main(int argc, char *argv[])
{
    struct timeval delta, oldDelta, *deltap;

    oldDelta.tv_sec = oldDelta.tv_usec = 0;

    delta.tv_sec =  (argc > 1) ? atoi(argv[1]) : 0;
    delta.tv_usec = (argc > 2) ? atoi(argv[2]) : 0;

    if (argc > 1)
        deltap = &delta;
    else
        deltap = NULL;

    if (adjtime(deltap, &oldDelta) == -1) {
        perror("adjtime");
        exit(EXIT_FAILURE);
    } 

    printf("old delta %ld.%06ld\n", oldDelta.tv_sec, oldDelta.tv_usec);
    exit(EXIT_SUCCESS);
} /* main */

If given command-line arguments, this program uses them 
to initialise 'delta'.  If no arguments are supplied
then delta is specified as NULL.  In either case, the program
prints the information returned in 'olddelta'.

On Linux we see something like the following:

# ./t_adjtime 2
old delta 0.000000
# ./t_adjtime
old delta 0.000000

The last line of output shows the problem.  When we do similar 
on FreeBSD, we see something like the following

# ./t_adjtime 2
old delta 0.000000
# ./t_adjtime
old delta 1.990000

== END ==
Comment 1 john stultz 2007-02-09 13:47:03 UTC
Looking over the context, it does seem like the fix is a bit more complicated. 

It looks like this would require both a kernel and glibc change (this issue
appears to stem from sys_adjtimex's multiplexing of current adjtimex interface
along w/ the old adjtime interface). 

I'm worried that the patch below would fix the old adjtime interface but break
the adjtimex interface.
Comment 2 Michael Kerrisk 2007-02-09 19:35:23 UTC
Yes, for all I know the patch I gave may well be insufficient.  I don't 
understand the code there well enough.  Nevertheless, things do seem to 
be broken as they stand, and some type of fix is required...

Comment 3 Natalie Protasevich 2007-07-07 13:22:48 UTC
What is the current state of this issue, will fix be needed eventually? it seems like for portablity adjtime() should be handled properly. Are there any plans to find a solution?
Thanks.
Comment 4 Andrew Morton 2007-07-27 16:37:40 UTC
guys, that pain in your ribs is my thumb ;)
Comment 5 john stultz 2007-07-27 17:09:44 UTC
:( The issue is the adjtimex interface multiplexes both the old adjtime and new adjtimex interfaces. 

The problem being, if nothing is passed in on the modes, you get the get new adjtimex()'s offset value, not the adjtime offset.

In order to get the current adjtime offset value, you have to use the ADJ_OFFSET_SINGLESHOT mode, which overwrites the current adjtime offset (and returns the old).

The only fix I can imagine for this would be to change the adjtimex interface and introduce a new modes flag, something like:

#define ADJ_OFFSET_SINGLESHOT_READ 0x2000

And we can add a simple:
if (txc->modes == ADJ_OFFSET_SINGLESHOT_READ)
    txc->offset = saved_adjust;

Then glibc can pass it in if adjtime() is called w/ no new adjustment value.

Outside of that I'm not sure I see how to solve this.
Comment 6 john stultz 2007-07-27 17:26:09 UTC
Created attachment 12184 [details]
Add ADJ_OFFSET_SS_READ

Untested patch to add ADJ_OFFSET_SS_READ modes flag. How does this sound:

When passed in the modes field to adjtimex() we will return the current singleshot (adjtime style) adjustment in the timex.offset field without modifying the kernel's time_adjust value.

Needs glibc to use this flag in its adjtime() implementation in the case where the new offset is null and the old offset is non-null.
Comment 7 john stultz 2007-07-27 17:28:00 UTC
It should be noted that the existing behavior is documented in the man page for adjtime():
"BUGS
       Currently,  if  delta  is specified as NULL, no valid information about
       the outstanding clock adjustment is returned  in  olddelta.   (In  this
       circumstance, adjtime() should return the outstanding clock adjustment,
       without changing it.)  This is the result of a kernel limitation."
Comment 8 Michael Kerrisk 2007-07-28 05:09:14 UTC
(In reply to comment #7)
> It should be noted that the existing behavior is documented in the man page for
> adjtime():
> "BUGS
>        Currently,  if  delta  is specified as NULL, no valid information about
>        the outstanding clock adjustment is returned  in  olddelta.   (In  this
>        circumstance, adjtime() should return the outstanding clock adjustment,
>        without changing it.)  This is the result of a kernel limitation."

Yes, it's documented in the man page because as well as creating this bug report, I also wrote the man page text.

Comment 9 Natalie Protasevich 2007-07-29 02:58:38 UTC
*** Bug 7491 has been marked as a duplicate of this bug. ***
Comment 10 Natalie Protasevich 2007-07-29 03:03:10 UTC
Sorry, disregard #9, it was operator error.
Comment 11 Ingo Molnar 2007-11-18 07:52:19 UTC
Ulrich, what do you think about this new proposed ADJ_OFFSET_SINGLESHOT_READ modes flag?

glibc would use this flag in its adjtime() implementation in the case where
the new offset is null and the old offset is non-null.

old kernels just ignore unknown mode flags and return TIME_OK.

If this is OK to you then we can add this fix to the upstream kernel.
Comment 12 Ingo Molnar 2007-11-18 08:07:05 UTC
We've added this to the x86 tree's "still cooking" queue of patches. This means it will show up in -mm for testing, but it will need Ulrich's ACK before it can go upstream.
Comment 14 Ingo Molnar 2007-12-11 13:26:17 UTC
great - i'm closing it. Thanks to everyone involved.

Note You need to log in before you can comment on or make changes to this bug.