lkml.org 
[lkml]   [2006]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2006-03-20 07:12    [W:0.059 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site