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?
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?
Cheers,
Michael
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!
Sort of a quick off the cuff patch, but does the following look like the
right fix to you?
Roman: your thoughts?
Signed-off-by: John Stultz <[email protected]>
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();
}
On Tue, Jul 1, 2008 at 12:07 AM, john stultz <[email protected]> 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 <[email protected]>
> Roman: your thoughts?
>
>
> Signed-off-by: John Stultz <[email protected]>
>
> 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
On Tue, Jul 1, 2008 at 3:30 AM, Michael Kerrisk
<[email protected]> wrote:
> On Tue, Jul 1, 2008 at 12:07 AM, john stultz <[email protected]> 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 <[email protected]>
John,
Are you pushing this into 2.6.27-rc1?
Cheers,
Michael
>> Roman: your thoughts?
>>
>>
>> Signed-off-by: John Stultz <[email protected]>
>>
>> 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
Hi,
On Mon, 21 Jul 2008, Michael Kerrisk wrote:
> >> Sort of a quick off the cuff patch, but does the following look like the
> >> right fix to you?
I would more prefer a patch like below, instead of adding tests for this
all over the place this separates the code more clearly, as this syscall
is seriously overloaded.
bye, Roman
[PATCH] fix ADJ_OFFSET_SS_READ bug and do_adjtimex cleanup
Thanks to the review by Michael Kerrisk <[email protected]> a
bug in the recent ADJ_OFFSET_SS_READ option was discovered, where the
ntp time_offset was inadvertently set by it. This fixes this by making
the adjtime code more separate from the ntp_adjtime code (both of which
really want to be separate syscalls).
Signed-off-by: Roman Zippel <[email protected]>
---
include/linux/timex.h | 9 +++++
kernel/time/ntp.c | 76 ++++++++++++++++++++++++++------------------------
2 files changed, 48 insertions(+), 37 deletions(-)
Index: linux-2.6/include/linux/timex.h
===================================================================
--- linux-2.6.orig/include/linux/timex.h
+++ linux-2.6/include/linux/timex.h
@@ -141,8 +141,15 @@ struct timex {
#define ADJ_MICRO 0x1000 /* select microsecond resolution */
#define ADJ_NANO 0x2000 /* select nanosecond resolution */
#define ADJ_TICK 0x4000 /* tick value */
+
+#ifdef __KERNEL__
+#define ADJ_ADJTIME 0x8000 /* switch between adjtime/adjtimex modes */
+#define ADJ_OFFSET_SINGLESHOT 0x0001 /* old-fashioned adjtime */
+#define ADJ_OFFSET_READONLY 0x2000 /* read-only adjtime */
+#else
#define ADJ_OFFSET_SINGLESHOT 0x8001 /* old-fashioned adjtime */
-#define ADJ_OFFSET_SS_READ 0xa001 /* read-only adjtime */
+#define ADJ_OFFSET_SS_READ 0xa001 /* read-only adjtime */
+#endif
/* xntp 3.4 compatibility names */
#define MOD_OFFSET ADJ_OFFSET
Index: linux-2.6/kernel/time/ntp.c
===================================================================
--- linux-2.6.orig/kernel/time/ntp.c
+++ linux-2.6/kernel/time/ntp.c
@@ -277,38 +277,50 @@ static inline void notify_cmos_timer(voi
int do_adjtimex(struct timex *txc)
{
struct timespec ts;
- long save_adjust, sec;
int result;
- /* In order to modify anything, you gotta be super-user! */
- if (txc->modes && !capable(CAP_SYS_TIME))
- return -EPERM;
-
- /* Now we validate the data before disabling interrupts */
-
- if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT) {
+ /* Validate the data before disabling interrupts */
+ if (txc->modes & ADJ_ADJTIME) {
/* singleshot must not be used with any other mode bits */
- if (txc->modes & ~ADJ_OFFSET_SS_READ)
+ if (!(txc->modes & ADJ_OFFSET_SINGLESHOT))
return -EINVAL;
- }
+ if (!(txc->modes & ADJ_OFFSET_READONLY) &&
+ !capable(CAP_SYS_TIME))
+ return -EPERM;
+ } else {
+ /* In order to modify anything, you gotta be super-user! */
+ if (txc->modes && !capable(CAP_SYS_TIME))
+ return -EPERM;
+
+ /* if the quartz is off by more than 10% something is VERY wrong! */
+ if (txc->modes & ADJ_TICK &&
+ (txc->tick < 900000/USER_HZ ||
+ txc->tick > 1100000/USER_HZ))
+ return -EINVAL;
- /* if the quartz is off by more than 10% something is VERY wrong ! */
- if (txc->modes & ADJ_TICK)
- if (txc->tick < 900000/USER_HZ ||
- txc->tick > 1100000/USER_HZ)
- return -EINVAL;
+ if (txc->modes & ADJ_STATUS && time_state != TIME_OK)
+ hrtimer_cancel(&leap_timer);
+ }
- if (time_state != TIME_OK && txc->modes & ADJ_STATUS)
- hrtimer_cancel(&leap_timer);
getnstimeofday(&ts);
write_seqlock_irq(&xtime_lock);
- /* Save for later - semantics of adjtime is to return old value */
- save_adjust = time_adjust;
-
/* If there are input parameters, then process them */
+ if (txc->modes & ADJ_ADJTIME) {
+ long save_adjust = time_adjust;
+
+ if (!(txc->modes & ADJ_OFFSET_READONLY)) {
+ /* adjtime() is independent from ntp_adjtime() */
+ time_adjust = txc->offset;
+ ntp_update_frequency();
+ }
+ txc->offset = save_adjust;
+ goto adj_done;
+ }
if (txc->modes) {
+ long sec;
+
if (txc->modes & ADJ_STATUS) {
if ((time_status & STA_PLL) &&
!(txc->status & STA_PLL)) {
@@ -375,13 +387,8 @@ int do_adjtimex(struct timex *txc)
if (txc->modes & ADJ_TAI && txc->constant > 0)
time_tai = txc->constant;
- 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);
- }
+ if (txc->modes & ADJ_OFFSET)
+ ntp_update_offset(txc->offset);
if (txc->modes & ADJ_TICK)
tick_usec = txc->tick;
@@ -389,19 +396,16 @@ int do_adjtimex(struct timex *txc)
ntp_update_frequency();
}
+ txc->offset = shift_right(time_offset * NTP_INTERVAL_FREQ,
+ NTP_SCALE_SHIFT);
+ if (!(time_status & STA_NANO))
+ txc->offset /= NSEC_PER_USEC;
+
+adj_done:
result = time_state; /* mostly `TIME_OK' */
if (time_status & (STA_UNSYNC|STA_CLOCKERR))
result = TIME_ERROR;
- if ((txc->modes == ADJ_OFFSET_SINGLESHOT) ||
- (txc->modes == ADJ_OFFSET_SS_READ))
- txc->offset = save_adjust;
- else {
- txc->offset = shift_right(time_offset * NTP_INTERVAL_FREQ,
- NTP_SCALE_SHIFT);
- if (!(time_status & STA_NANO))
- txc->offset /= NSEC_PER_USEC;
- }
txc->freq = shift_right((s32)(time_freq >> PPM_SCALE_INV_SHIFT) *
(s64)PPM_SCALE_INV,
NTP_SCALE_SHIFT);