Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758678AbYGUKhK (ORCPT ); Mon, 21 Jul 2008 06:37:10 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755925AbYGUKg6 (ORCPT ); Mon, 21 Jul 2008 06:36:58 -0400 Received: from rv-out-0506.google.com ([209.85.198.230]:40604 "EHLO rv-out-0506.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751417AbYGUKg5 (ORCPT ); Mon, 21 Jul 2008 06:36:57 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=googlemail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=AzqTc6TLnrc7qgYuDJC0A3u5VHyTj4S803bW+F8AXfBGqbxdp53nFmR4isH6sYB75x 3nvFvBZa/QdqanZG+Ovs/t+DEOTi9DHw6QQwHkL17Rkf69oCtPXYAGd5CgpPENDCGjzl AzDH+J0PMRlXVj7N+3nEbpB9hbYdVBTp7YG14= Message-ID: Date: Mon, 21 Jul 2008 12:36:56 +0200 From: "Michael Kerrisk" To: "john stultz" Subject: Re: ADJ_OFFSET_SS_READ bug? Cc: "Michael Kerrisk" , "Roman Zippel" , lkml , "Thomas Gleixner" , "Ingo Molnar" In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <485E0007.2020904@gmail.com> <1214863650.3143.19.camel@localhost.localdomain> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4580 Lines: 127 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/