Messages in this thread | | | Date | Mon, 21 Jul 2008 12:36:56 +0200 | From | "Michael Kerrisk" <> | Subject | Re: ADJ_OFFSET_SS_READ bug? |
| |
On Tue, Jul 1, 2008 at 3:30 AM, Michael Kerrisk <mtk.manpages@googlemail.com> wrote: > On Tue, Jul 1, 2008 at 12:07 AM, john stultz <johnstul@us.ibm.com> wrote: >> >> On Sun, 2008-06-22 at 09:32 +0200, Michael Kerrisk wrote: >>> Roman, John >>> >>> John, thanks for ADJ_OFFSET_SS_READ, which fixed my bug report >>> (http://sourceware.org/bugzilla/show_bug?id=2449, >>> http://bugzilla.kernel.org/show_bug.cgi?id=6761) >>> >>> Roman, thanks for fixing John's fix ;-) >>> >>> However, I'm wondering if there is a potential bug in the >>> implementation of this flag. Note the following definitions >>> from include/linux/timex.h: >>> >>> #define ADJ_OFFSET 0x0001 /* time offset */ >>> [...] >>> #define ADJ_OFFSET_SINGLESHOT 0x8001 /* old-fashioned adjtime */ >>> #define ADJ_OFFSET_SS_READ 0xa001 /* read-only adjtime */ >>> >>> >>> Using the the above value for ADJ_OFFSET_SS_READ, where the bits match those >>> in ADJ_OFFSET and ADJ_OFFSET_SINGLESHOT, seems unnecessary as far as I can >>> see. Why was that done? >> >> Hrm. My original fix was to use 0x2000, but from the commit Ingo changed >> it at Ulrich's suggestion. Had something to do with old glibc's doing >> the right thing? >> >>> More to the point, it looks like it creates a bug, since the "read-only >>> adjtime" triggers the code path for ADJ_OFFSET: >>> >>> if (txc->modes) { >>> ... >>> if (txc->modes & ADJ_OFFSET) { >>> if (txc->modes == ADJ_OFFSET_SINGLESHOT) >>> /* adjtime() is independent from ntp_adjtime() */ >>> time_adjust = txc->offset; >>> else >>> ntp_update_offset(txc->offset); /*XXX*/ >>> } >>> if (txc->modes & ADJ_TICK) >>> tick_usec = txc->tick; >>> >>> if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET)) >>> ntp_update_frequency(); /*XXX*/ >>> } >>> >>> Unless I misunderstood something, ADJ_OFFSET_SS_READ causes the code marked >>> XXX to be executed, but I don't think that is what is desired. Is that true? >> >> Yea. That does look like an issue. Thanks for the close inspection and >> review! > > You're welcome -- thanks for getting back to me (I was beginning to > wonder if my mail got dropped somewhere)/ > >> Sort of a quick off the cuff patch, but does the following look like the >> right fix to you? > > I haven't tested this, but given your statement about maintaining old > glibc behavior, this looks like the riht fix, so: > > Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
John,
Are you pushing this into 2.6.27-rc1?
Cheers,
Michael
>> Roman: your thoughts? >> >> >> Signed-off-by: John Stultz <johnstul@us.ibm.com> >> >> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c >> index 5125ddd..7842a8d 100644 >> --- a/kernel/time/ntp.c >> +++ b/kernel/time/ntp.c >> @@ -379,13 +379,14 @@ int do_adjtimex(struct timex *txc) >> if (txc->modes == ADJ_OFFSET_SINGLESHOT) >> /* adjtime() is independent from ntp_adjtime() */ >> time_adjust = txc->offset; >> - else >> + else if (txc->modes != ADJ_OFFSET_SS_READ) >> ntp_update_offset(txc->offset); >> } >> if (txc->modes & ADJ_TICK) >> tick_usec = txc->tick; >> >> - if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET)) >> + if ((txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET)) && >> + (txc->modes != ADJ_OFFSET_SS_READ)) >> ntp_update_frequency(); >> } >> >> >> >> > > > > -- > Michael Kerrisk > Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ > man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html > Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html >
-- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html
| |