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

Willy Tarreau
2005-10-01T20:45:33Z

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;
>
>