[PATCH] poll/select: avoid arithmetic overflow in __estimate_accuracy()

[PATCH] poll/select: avoid arithmetic overflow in __estimate_accuracy()

[Guillaume Knispel]

__estimate_accuracy() was prone to integer overflow, for example
if *tv == {2147, 483648000} on a 32 bit computer (or even for delays
as small as {429, 500000000} if the task is niced).

Because the result was already forced between 0 and 100ms, the effect
of the overflow was not too problematic, but the use of the hrtimer
range feature was not optimal in overflow cases.

This patch ensures that there can not be an integer overflow in this
function.

Signed-off-by: Guillaume Knispel <gknispel@proformatique.com>
---
 fs/select.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index 8084834..a201fc3 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -41,22 +41,28 @@
  * better solutions..
  */
 
+#define MAX_SLACK	(100 * NSEC_PER_MSEC)
+
 static long __estimate_accuracy(struct timespec *tv)
 {
 	long slack;
 	int divfactor = 1000;
 
+	if (tv->tv_sec < 0)
+		return 0;
+
 	if (task_nice(current) > 0)
 		divfactor = divfactor / 5;
 
+	if (tv->tv_sec > MAX_SLACK / (NSEC_PER_SEC/divfactor))
+		return MAX_SLACK;
+
 	slack = tv->tv_nsec / divfactor;
 	slack += tv->tv_sec * (NSEC_PER_SEC/divfactor);
 
-	if (slack > 100 * NSEC_PER_MSEC)
-		slack =  100 * NSEC_PER_MSEC;
+	if (slack > MAX_SLACK)
+		return MAX_SLACK;
 
-	if (slack < 0)
-		slack = 0;
 	return slack;
 }
 

Re: [PATCH] poll/select: avoid arithmetic overflow in __estimate_accuracy()

[Amerigo Wang]

On Sun, Aug 16, 2009 at 10:29:21PM +0200, Guillaume Knispel wrote:
>__estimate_accuracy() was prone to integer overflow, for example
>if *tv == {2147, 483648000} on a 32 bit computer (or even for delays
>as small as {429, 500000000} if the task is niced).
>
>Because the result was already forced between 0 and 100ms, the effect
>of the overflow was not too problematic, but the use of the hrtimer
>range feature was not optimal in overflow cases.
>
>This patch ensures that there can not be an integer overflow in this
>function.
>
>Signed-off-by: Guillaume Knispel <gknispel@proformatique.com>
>---
> fs/select.c |   14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)
>
>diff --git a/fs/select.c b/fs/select.c
>index 8084834..a201fc3 100644
>--- a/fs/select.c
>+++ b/fs/select.c
>@@ -41,22 +41,28 @@
>  * better solutions..
>  */
> 
>+#define MAX_SLACK	(100 * NSEC_PER_MSEC)
>+
> static long __estimate_accuracy(struct timespec *tv)
> {
> 	long slack;
> 	int divfactor = 1000;
> 
>+	if (tv->tv_sec < 0)
>+		return 0;
>+
> 	if (task_nice(current) > 0)
> 		divfactor = divfactor / 5;
> 
>+	if (tv->tv_sec > MAX_SLACK / (NSEC_PER_SEC/divfactor))
>+		return MAX_SLACK;
>+


Yes? Isn't MAX_SLACK also too big for 'long' on 32bit machine?

Try:

% printf "%d\n" "100*1000000000 > 0x7fffffff"
1

Am I missing something here?


Thanks.


> 	slack = tv->tv_nsec / divfactor;
> 	slack += tv->tv_sec * (NSEC_PER_SEC/divfactor);
> 
>-	if (slack > 100 * NSEC_PER_MSEC)
>-		slack =  100 * NSEC_PER_MSEC;
>+	if (slack > MAX_SLACK)
>+		return MAX_SLACK;
> 
>-	if (slack < 0)
>-		slack = 0;
> 	return slack;
> }
> 
>--
>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/

Re: [PATCH] poll/select: avoid arithmetic overflow in __estimate_accuracy()

[Guillaume Knispel]

> On Sun, Aug 16, 2009 at 10:29:21PM +0200, Guillaume Knispel wrote:
>>__estimate_accuracy() was prone to integer overflow, for example
>>if *tv == {2147, 483648000} on a 32 bit computer (or even for delays
>>as small as {429, 500000000} if the task is niced).
>>
>>Because the result was already forced between 0 and 100ms, the effect
>>of the overflow was not too problematic, but the use of the hrtimer
>>range feature was not optimal in overflow cases.
>>
>>This patch ensures that there can not be an integer overflow in this
>>function.
>>
>>Signed-off-by: Guillaume Knispel <gknispel@proformatique.com>
>>---
>> fs/select.c |   14 ++++++++++----
>> 1 files changed, 10 insertions(+), 4 deletions(-)
>>
>>diff --git a/fs/select.c b/fs/select.c
>>index 8084834..a201fc3 100644
>>--- a/fs/select.c
>>+++ b/fs/select.c
>>@@ -41,22 +41,28 @@
>>  * better solutions..
>>  */
>>
>>+#define MAX_SLACK	(100 * NSEC_PER_MSEC)
>>+
>> static long __estimate_accuracy(struct timespec *tv)
>> {
>> 	long slack;
>> 	int divfactor = 1000;
>>
>>+	if (tv->tv_sec < 0)
>>+		return 0;
>>+
>> 	if (task_nice(current) > 0)
>> 		divfactor = divfactor / 5;
>>
>>+	if (tv->tv_sec > MAX_SLACK / (NSEC_PER_SEC/divfactor))
>>+		return MAX_SLACK;
>>+
>
>
> Yes? Isn't MAX_SLACK also too big for 'long' on 32bit machine?
>
> Try:
>
> % printf "%d\n" "100*1000000000 > 0x7fffffff"
> 1
>
> Am I missing something here?

100 * NSEC_PER_MSEC  =>  100 * 1000000
               -

The exact same multiplication was already in the existing code,
and this is not the one that can overflow.


Cheers,
Guillaume Knispel