Re: [PATCH 3/3] 2.6.14-rc2-mm1 : fixes for overflow in sys_poll()

[Willy Tarreau <willy@w.ods.org>]

Hi,

On Sat, Sep 24, 2005 at 09:52:05PM +0200, Willy Tarreau wrote:
> This patch simplifies the overflow fix which was applied to sys_poll() in
> 2.6.14-rc2-mm1 by relying on the overflow detection code added to jiffies.h
> by patch 1/3. This also removes a useless divide for HZ <= 1000.
> 
> It might be worth applying too. It's only for -mm1, and does *not* apply
> to 2.6.14-rc2 because this code has already received patches in -mm1.

I noticed that the overflow check in the original code is either useless or
buggy, so this patch is even more worth applying. Look below at original
code : if HZ <= 1000, the overflow is set only when
msecs_to_jiffies() >= MAX_SCHEDULE_TIMEOUT.

But the maximal value that msecs_to_jiffies() can return is MAX_JIFFY_OFFSET.
Guess what ?

  include/linux/jiffies.h:#define MAX_JIFFY_OFFSET ((~0UL >> 1)-1)
  include/linux/sched.h:#define    MAX_SCHEDULE_TIMEOUT    LONG_MAX
  include/linux/kernel.h:#define LONG_MAX  ((long)(~0UL>>1))

=> MAX_JIFFY_OFFSET == (MAX_SCHEDULE_TIMEOUT - 1)

=> So msecs_to_jiffies() cannot be >= MAX_SCHEDULE_TIMEOUT, and 'overflow'
   can never be set for HZ <= 1000.

Given how msecs_to_jiffies() is used everywhere, I wonder if we should not
provide a separate overflow detection function which would be used where
needed, and possibly remove the test from msecs_to_jiffies() which is mostly
called with constants or small delays which will never overflow.

Has anyone an opinion on this ?

Regards,
Willy

> diff -purN linux-2.6.14-rc2-mm1/fs/select.c linux-2.6.14-rc2-mm1-poll/fs/select.c
> --- linux-2.6.14-rc2-mm1/fs/select.c	Sat Sep 24 21:12:36 2005
> +++ linux-2.6.14-rc2-mm1-poll/fs/select.c	Sat Sep 24 21:23:21 2005
> @@ -469,7 +469,6 @@ asmlinkage long sys_poll(struct pollfd _
>  {
>  	struct poll_wqueues table;
>  	int fdcount, err;
> -	int overflow;
>   	unsigned int i;
>  	struct poll_list *head;
>   	struct poll_list *walk;
> @@ -486,22 +485,11 @@ asmlinkage long sys_poll(struct pollfd _
>  		return -EINVAL;
>  
>  	/*
> -	 * We compare HZ with 1000 to work out which side of the
> -	 * expression needs conversion.  Because we want to avoid
> -	 * converting any value to a numerically higher value, which
> -	 * could overflow.
> -	 */
> -#if HZ > 1000
> -	overflow = timeout_msecs >= jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT);
> -#else
> -	overflow = msecs_to_jiffies(timeout_msecs) >= MAX_SCHEDULE_TIMEOUT;
> -#endif
> -
> -	/*
>  	 * If we would overflow in the conversion or a negative timeout
> -	 * is requested, sleep indefinitely.
> +	 * is requested, sleep indefinitely. Note: msecs_to_jiffies checks
> +	 * for the overflow.
>  	 */
> -	if (overflow || timeout_msecs < 0)
> +	if (timeout_msecs < 0)
>  		timeout_jiffies = MAX_SCHEDULE_TIMEOUT;
>  	else
>  		timeout_jiffies = msecs_to_jiffies(timeout_msecs) + 1;
> 
>