Messages in this thread | | | Date | Sun, 19 Mar 2006 22:06:28 -0800 | From | Andrew Morton <> | Subject | Re: [patch 1/2] sys_alarm() unsigned signed conversion fixup |
| |
Thomas Gleixner <tglx@linutronix.de> wrote: > > > alarm() calls the kernel with an unsigend int timeout in seconds. > The value is stored in the tv_sec field of a struct timeval to > setup the itimer. The tv_sec field of struct timeval is of type long, > which causes the tv_sec value to be negative on 32 bit machines if > seconds > INT_MAX. > > Before the hrtimer merge (pre 2.6.16) such a negative value was > converted to the maximum jiffies timeout by the timeval_to_jiffies > conversion. It's not clear whether this was intended or just happened > to be done by the timeval_to_jiffies code. > > hrtimers expect a timeval in canonical form and treat a negative > timeout as already expired. This breaks the legitimate usage of > alarm() with a timeout value > INT_MAX seconds. > > For 32 bit machines it is therefor necessary to limit the internal > seconds value to avoid API breakage. Instead of doing this in all > implementations of sys_alarm the duplicated sys_alarm code is moved > into a common function in itimer.c > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > arch/ia64/ia32/sys_ia32.c | 14 +------------- > arch/mips/kernel/sysirix.c | 22 +--------------------- > arch/x86_64/ia32/sys_ia32.c | 16 ++-------------- > include/linux/time.h | 1 + > kernel/itimer.c | 37 +++++++++++++++++++++++++++++++++++++ > kernel/timer.c | 14 +------------- > 6 files changed, 43 insertions(+), 61 deletions(-)
It would have been much better if you'd avoided the temptation to make sweeping cleanups. We're trying to get a product out here and there are good reasons for preferring minimal fixes at this stage.
The non-MIPS changes eyeball out OK. The MIPS changes are quite unobvious.
A patch which applies cleanups like that would have been rejected out of hand as a post-2.6.16-rc6 candidate, so why on earth go mixing it up with important bugfixes?
Sigh.
> +unsigned int alarm_setitimer(unsigned int seconds) > +{ > + struct itimerval it_new, it_old; > + > +#if BITS_PER_LONG < 64 > + if (seconds > (INT_MAX >> 1)) > + seconds = (INT_MAX >> 1); > +#endif
Why clamp it at INT_MAX/2? INT_MAX is positive.
- 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/
| |