Messages in this thread | | | Date | Fri, 20 Feb 2015 14:58:49 -0800 | Subject | Re: [GIT PULL] time/ntp fix | From | 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
| |