ADJ_OFFSET_SS_READ bug? - Kernel

This is a discussion on ADJ_OFFSET_SS_READ bug? - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: ADJ_OFFSET_SS_READ bug?

  1. ADJ_OFFSET_SS_READ bug?

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: ADJ_OFFSET_SS_READ bug?


    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

    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();
    }



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: ADJ_OFFSET_SS_READ bug?

    On Tue, Jul 1, 2008 at 12:07 AM, john stultz 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

    > Roman: your thoughts?
    >
    >
    > Signed-off-by: John Stultz
    >
    > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: ADJ_OFFSET_SS_READ bug?

    On Tue, Jul 1, 2008 at 3:30 AM, Michael Kerrisk
    wrote:
    > On Tue, Jul 1, 2008 at 12:07 AM, john stultz 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



    John,

    Are you pushing this into 2.6.27-rc1?

    Cheers,

    Michael

    >> Roman: your thoughts?
    >>
    >>
    >> Signed-off-by: John Stultz
    >>
    >> 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: ADJ_OFFSET_SS_READ bug?

    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 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

    ---
    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);
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread