Re: [GIT PULL] time/ntp fix

[John Stultz]

On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <mingo@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/