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;
}
On Fri, Feb 20, 2015 at 9:44 PM, Ingo Molnar <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <[email protected]> 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
On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
<[email protected]> wrote:
> On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <[email protected]> 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
On Fri, Feb 20, 2015 at 2:58 PM, John Stultz <[email protected]> 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
* John Stultz <[email protected]> wrote:
> On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
> <[email protected]> wrote:
> > On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <[email protected]> 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
Hi Ingo,
On Sat, Feb 21, 2015 at 6:49 AM, Ingo Molnar <[email protected]> wrote:
> * John Stultz <[email protected]> wrote:
>
>> On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
>> <[email protected]> wrote:
>> > On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <[email protected]> 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 -- [email protected]
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