Nishanth Aravamudan <[EMAIL PROTECTED]> wrote:
>
> Description: The current sys_poll() implementation does not seem to
> handle large timeouts correctly. Any value in milliseconds (@timeout)
> which exceeds the maximum representable jiffy value
> (MAX_SCHEDULE_TIMEOUT) should result in a MAX_SCHEDULE_TIMEOUT
> schedule_timeout() request. To achieve this, convert @timeout to jiffies
> first, then compare to MAX_SCHEDULE_TIMEOUT.
The above doesn't describe the bug very well.
> Signed-off-by: Nishanth Aravamudan <[EMAIL PROTECTED]>
>
> ---
>
> fs/select.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff -urpN 2.6.13/fs/select.c 2.6.13-dev/fs/select.c
> --- 2.6.13/fs/select.c 2005-08-28 17:46:14.000000000 -0700
> +++ 2.6.13-dev/fs/select.c 2005-09-09 17:22:30.000000000 -0700
> @@ -469,13 +469,16 @@ asmlinkage long sys_poll(struct pollfd _
> if (nfds > current->files->max_fdset && nfds > OPEN_MAX)
> return -EINVAL;
>
> - if (timeout) {
> - /* Careful about overflow in the intermediate values */
> - if ((unsigned long) timeout < MAX_SCHEDULE_TIMEOUT / HZ)
This is the problem to which you're referring, yes?
We're comparing milliseconds with jiffies/HZ, yes?
> - timeout = (unsigned long)(timeout*HZ+999)/1000+1;
> - else /* Negative or overflow */
> - timeout = MAX_SCHEDULE_TIMEOUT;
> - }
> + if (timeout > 0)
> + /*
> + * Convert the value from msecs to jiffies - if overflow
> + * occurs we get a negative value, which gets handled by
> + * the next block
> + */
> + timeout = msecs_to_jiffies(timeout) + 1;
> + if (timeout < 0) /* Negative requests result in infinite timeouts */
> + timeout = MAX_SCHEDULE_TIMEOUT;
> + /* 0 case falls through */
I don't particularly like the idea of relying on msecs_to_jiffies(too much)
returning a negative value.
Why can't we do
int too_much;
/*
* 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
too_much = timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT);
#else
too_much = msecs_to_jiffies(timeout) > MAX_SCHEDULE_TIMEOUT;
#endif
if (too_much)
timeout = MAX_SCHEDULE_TIMEOUT;
And while we're there, let's stop using the same variable for two different
units - it's horrid. How about we nuke `timeout' and create timeout_msecs
and timeout_jiffies to show what units they're in?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
