* ADJ_OFFSET_SS_READ bug?
@ 2008-06-22 7:32 Michael Kerrisk
2008-06-30 22:07 ` john stultz
0 siblings, 1 reply; 5+ messages in thread
From: Michael Kerrisk @ 2008-06-22 7:32 UTC (permalink / raw)
To: Roman Zippel; +Cc: lkml, john stultz, Thomas Gleixner, Ingo Molnar
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ADJ_OFFSET_SS_READ bug?
2008-06-22 7:32 ADJ_OFFSET_SS_READ bug? Michael Kerrisk
@ 2008-06-30 22:07 ` john stultz
2008-07-01 1:30 ` Michael Kerrisk
0 siblings, 1 reply; 5+ messages in thread
From: john stultz @ 2008-06-30 22:07 UTC (permalink / raw)
To: Michael Kerrisk; +Cc: Roman Zippel, lkml, Thomas Gleixner, Ingo Molnar
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 <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();
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: ADJ_OFFSET_SS_READ bug?
2008-06-30 22:07 ` john stultz
@ 2008-07-01 1:30 ` Michael Kerrisk
2008-07-21 10:36 ` Michael Kerrisk
0 siblings, 1 reply; 5+ messages in thread
From: Michael Kerrisk @ 2008-07-01 1:30 UTC (permalink / raw)
To: john stultz
Cc: Michael Kerrisk, Roman Zippel, lkml, Thomas Gleixner, Ingo Molnar
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>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ADJ_OFFSET_SS_READ bug?
2008-07-01 1:30 ` Michael Kerrisk
@ 2008-07-21 10:36 ` Michael Kerrisk
2008-07-21 19:59 ` Roman Zippel
0 siblings, 1 reply; 5+ messages in thread
From: Michael Kerrisk @ 2008-07-21 10:36 UTC (permalink / raw)
To: john stultz
Cc: Michael Kerrisk, Roman Zippel, lkml, Thomas Gleixner, Ingo Molnar
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ADJ_OFFSET_SS_READ bug?
2008-07-21 10:36 ` Michael Kerrisk
@ 2008-07-21 19:59 ` Roman Zippel
0 siblings, 0 replies; 5+ messages in thread
From: Roman Zippel @ 2008-07-21 19:59 UTC (permalink / raw)
To: Michael Kerrisk
Cc: john stultz, lkml, Thomas Gleixner, Ingo Molnar, Andrew Morton
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 <mtk.manpages@googlemail.com> 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 <zippel@linux-m68k.org>
---
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);
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-07-21 20:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-22 7:32 ADJ_OFFSET_SS_READ bug? Michael Kerrisk
2008-06-30 22:07 ` john stultz
2008-07-01 1:30 ` Michael Kerrisk
2008-07-21 10:36 ` Michael Kerrisk
2008-07-21 19:59 ` Roman Zippel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox