[GIT PULL] time/ntp fix

[GIT PULL] time/ntp fix

[Ingo Molnar]

Linus,

Please pull the latest timers-urgent-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-for-linus

   # HEAD: 29183a70b0b828500816bd794b3fe192fce89f73 ntp: Fixup adjtimex freq validation on 32-bit systems

An adjtimex interface regression fix for 32-bit systems.

 Thanks,

	Ingo

------------------>
John Stultz (1):
      ntp: Fixup adjtimex freq validation on 32-bit systems


 kernel/time/ntp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 4b585e0fdd22..0f60b08a4f07 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -633,10 +633,14 @@ int ntp_validate_timex(struct timex *txc)
 	if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME)))
 		return -EPERM;
 
-	if (txc->modes & ADJ_FREQUENCY) {
-		if (LONG_MIN / PPM_SCALE > txc->freq)
+	/*
+	 * Check for potential multiplication overflows that can
+	 * only happen on 64-bit systems:
+	 */
+	if ((txc->modes & ADJ_FREQUENCY) && (BITS_PER_LONG == 64)) {
+		if (LLONG_MIN / PPM_SCALE > txc->freq)
 			return -EINVAL;
-		if (LONG_MAX / PPM_SCALE < txc->freq)
+		if (LLONG_MAX / PPM_SCALE < txc->freq)
 			return -EINVAL;
 	}
 

Re: [GIT PULL] time/ntp fix

[Dongsheng Song]

On Fri, Feb 20, 2015 at 9:44 PM, Ingo Molnar <mingo@kernel.org> wrote:
> Linus,
>
> Please pull the latest timers-urgent-for-linus git tree from:
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-for-linus
>
>    # HEAD: 29183a70b0b828500816bd794b3fe192fce89f73 ntp: Fixup adjtimex freq validation on 32-bit systems
>
> An adjtimex interface regression fix for 32-bit systems.
>
>  Thanks,
>
>         Ingo
>
> ------------------>
> John Stultz (1):
>       ntp: Fixup adjtimex freq validation on 32-bit systems
>
>
>  kernel/time/ntp.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 4b585e0fdd22..0f60b08a4f07 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -633,10 +633,14 @@ int ntp_validate_timex(struct timex *txc)
>         if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME)))
>                 return -EPERM;
>
> -       if (txc->modes & ADJ_FREQUENCY) {
> -               if (LONG_MIN / PPM_SCALE > txc->freq)
> +       /*
> +        * Check for potential multiplication overflows that can
> +        * only happen on 64-bit systems:

Typo ?

> +        */
> +       if ((txc->modes & ADJ_FREQUENCY) && (BITS_PER_LONG == 64)) {
> +               if (LLONG_MIN / PPM_SCALE > txc->freq)
>                         return -EINVAL;
> -               if (LONG_MAX / PPM_SCALE < txc->freq)
> +               if (LLONG_MAX / PPM_SCALE < txc->freq)
>                         return -EINVAL;
>         }
>
> --
> 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/

Re: [GIT PULL] time/ntp fix

[Linus Torvalds]

On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> John Stultz (1):
>       ntp: Fixup adjtimex freq validation on 32-bit systems

This is confusing. 32-bit?

> +       /*
> +        * Check for potential multiplication overflows that can
> +        * only happen on 64-bit systems:

64-bit?

> +       if ((txc->modes & ADJ_FREQUENCY) && (BITS_PER_LONG == 64)) {

Hmm. The code clearly says "&& (BITS_PER_LONG == 64)"

But:

> +               if (LLONG_MIN / PPM_SCALE > txc->freq)
>                         return -EINVAL;
> -               if (LONG_MAX / PPM_SCALE < txc->freq)
> +               if (LLONG_MAX / PPM_SCALE < txc->freq)
>                         return -EINVAL;

The difference between LONG_MAX and LLONG_MAX only matters if
BITS_PER_LONG would be 32.

So the changes are confusing to begin with and the commit log
description doesn't match them anyway.

I'm not pulling this without clarifications.

                           Linus

Re: [GIT PULL] time/ntp fix

[John Stultz]

On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> John Stultz (1):
>>       ntp: Fixup adjtimex freq validation on 32-bit systems
>
> This is confusing. 32-bit?

Right, so the check that was added in a previous commit is really only
a concern for 64bit systems, but was applied to both 32 and 64bit
systems, which results in breaking 32bit systems.

Thus the "fix" here is to make the check only apply to 64bit systems.


>> +       /*
>> +        * Check for potential multiplication overflows that can
>> +        * only happen on 64-bit systems:
>
> 64-bit?
>
>> +       if ((txc->modes & ADJ_FREQUENCY) && (BITS_PER_LONG == 64)) {
>
> Hmm. The code clearly says "&& (BITS_PER_LONG == 64)"
>
> But:
>
>> +               if (LLONG_MIN / PPM_SCALE > txc->freq)
>>                         return -EINVAL;
>> -               if (LONG_MAX / PPM_SCALE < txc->freq)
>> +               if (LLONG_MAX / PPM_SCALE < txc->freq)
>>                         return -EINVAL;
>
> The difference between LONG_MAX and LLONG_MAX only matters if
> BITS_PER_LONG would be 32.

Right. So v1 of this fix just changed the LONG_MIX/MAX to be
LLONG_MIN/MAX. This resolves the issue, but with some compiler checks
we get warnings since LLONG_MIN/MAX is always smaller/larger then a
32bit value. Thus the extra BITS_PER_LONG check (which I think is a
bit ugly, but I didn't get any better suggestions) was added to avoid
the build warnings as well.

So the fix is somewhat redundant, but I do think the LLONG specifier
is more exact.

> So the changes are confusing to begin with and the commit log
> description doesn't match them anyway.
>
> I'm not pulling this without clarifications.

Is the above clarification sufficient? Or would you like me to reword
the commit message as well?

thanks
-john

Re: [GIT PULL] time/ntp fix

[Linus Torvalds]

On Fri, Feb 20, 2015 at 2:58 PM, John Stultz <john.stultz@linaro.org> wrote:
>
> Is the above clarification sufficient? Or would you like me to reword
> the commit message as well?

It's ok. And I'll add it to the merge message so that it's mentioned
somewhere at least.

Thanks,
                   Linus

Re: [GIT PULL] time/ntp fix

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >>
> >> John Stultz (1):
> >>       ntp: Fixup adjtimex freq validation on 32-bit systems
> >
> > This is confusing. 32-bit?
> 
> Right, so the check that was added in a previous commit 
> is really only a concern for 64bit systems, but was 
> applied to both 32 and 64bit systems, which results in 
> breaking 32bit systems.
> 
> Thus the "fix" here is to make the check only apply to 
> 64bit systems.

Yeah, perhaps a better commit title would have been to 
write:

     time/ntp: Fix adjtimex freq validation code build warning on 32-bit systems

To make it clear that the problem fixed is a 32-bit 
warning, and that the fix for that is to only check on 
64-bit systems.

I agreed with your BITS_PER_LONG check when I reviewed your 
patch, people usually do an ugly #ifdef, I think this 
in line check form is nicer.

Thanks,

	Ingo

Re: [GIT PULL] time/ntp fix

[Geert Uytterhoeven]

Hi Ingo,

On Sat, Feb 21, 2015 at 6:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
> * John Stultz <john.stultz@linaro.org> wrote:
>
>> On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>> > On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >>
>> >> John Stultz (1):
>> >>       ntp: Fixup adjtimex freq validation on 32-bit systems
>> >
>> > This is confusing. 32-bit?
>>
>> Right, so the check that was added in a previous commit
>> is really only a concern for 64bit systems, but was
>> applied to both 32 and 64bit systems, which results in
>> breaking 32bit systems.
>>
>> Thus the "fix" here is to make the check only apply to
>> 64bit systems.
>
> Yeah, perhaps a better commit title would have been to
> write:
>
>      time/ntp: Fix adjtimex freq validation code build warning on 32-bit systems
>
> To make it clear that the problem fixed is a 32-bit
> warning, and that the fix for that is to only check on
> 64-bit systems.
>
> I agreed with your BITS_PER_LONG check when I reviewed your
> patch, people usually do an ugly #ifdef, I think this
> in line check form is nicer.

Unfortunately it doesn't help with all compiler versions.
With gcc 4.1.2 for m68k:

kernel/time/ntp.c: In function ‘ntp_validate_timex’:
kernel/time/ntp.c:641: warning: comparison is always false due to
limited range of data type
kernel/time/ntp.c:643: warning: comparison is always false due to
limited range of data type

Gcc 4.6.3 and 4.9.0 are OK (for m68k).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[GIT PULL] time/ntp fix

[Ingo Molnar]

Linus,

Please pull the latest timers-urgent-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-for-linus

   HEAD: 6b1859dba01c7d512b72d77e3fd7da8354235189 ntp: Fix STA_INS/DEL clearing bug

 Thanks,

	Ingo

------------------>
John Stultz (1):
      ntp: Fix STA_INS/DEL clearing bug


 kernel/time/ntp.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 70b33ab..b7fbadc 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -409,7 +409,9 @@ int second_overflow(unsigned long secs)
 			time_state = TIME_DEL;
 		break;
 	case TIME_INS:
-		if (secs % 86400 == 0) {
+		if (!(time_status & STA_INS))
+			time_state = TIME_OK;
+		else if (secs % 86400 == 0) {
 			leap = -1;
 			time_state = TIME_OOP;
 			time_tai++;
@@ -418,7 +420,9 @@ int second_overflow(unsigned long secs)
 		}
 		break;
 	case TIME_DEL:
-		if ((secs + 1) % 86400 == 0) {
+		if (!(time_status & STA_DEL))
+			time_state = TIME_OK;
+		else if ((secs + 1) % 86400 == 0) {
 			leap = 1;
 			time_tai--;
 			time_state = TIME_WAIT;