[v2] fix overflow in idle exit_latency

Commit Message

Bo Ye Dec. 19, 2023, 3:14 a.m. UTC

From: C Cheng <[e-mail]>

In detail:

In C language, when you perform a multiplication operation, if
both operands are of int type, the multiplication operation is
performed on the int type, and then the result is converted to
the target type. This means that if the product of int type
multiplication exceeds the range that int type can represent,
 an overflow will occur even if you store the result in a
variable of int64_t type.

For a multiplication of two int values, it is better to use
mul_u32_u32() rather than s->exit_latency_ns = s->exit_latency *
NSEC_PER_USEC to avoid potential overflow happenning.

Signed-off-by: C Cheng <[e-mail]>
Signed-off-by: Bo Ye <[e-mail]>
---
Change in v2:
-remove Change-ID
-correct patch author name
-replace multiplication of two int values with mul_u32_u32
-refine commit message
---
 drivers/cpuidle/driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno Dec. 19, 2023, 10:56 a.m. UTC | #1

Il 19/12/23 04:14, Bo Ye ha scritto:
> From: C Cheng <[e-mail]>
> 
> In detail:
> 
> In C language, when you perform a multiplication operation, if
> both operands are of int type, the multiplication operation is
> performed on the int type, and then the result is converted to
> the target type. This means that if the product of int type
> multiplication exceeds the range that int type can represent,
>   an overflow will occur even if you store the result in a
> variable of int64_t type.
> 
> For a multiplication of two int values, it is better to use
> mul_u32_u32() rather than s->exit_latency_ns = s->exit_latency *
> NSEC_PER_USEC to avoid potential overflow happenning.
> 
> Signed-off-by: C Cheng <[e-mail]>
> Signed-off-by: Bo Ye <[e-mail]>

Reviewed-by: AngeloGioacchino Del Regno <[e-mail]>

David Laight Dec. 19, 2023, 4:24 p.m. UTC | #2

From: Bo Ye 
> Sent: 19 December 2023 03:15
> 
> From: C Cheng <[e-mail]>
> 
> In detail:
> 
> In C language, when you perform a multiplication operation, if
> both operands are of int type, the multiplication operation is
> performed on the int type, and then the result is converted to
> the target type. This means that if the product of int type
> multiplication exceeds the range that int type can represent,
>  an overflow will occur even if you store the result in a
> variable of int64_t type.
> 
> For a multiplication of two int values, it is better to use
> mul_u32_u32() rather than s->exit_latency_ns = s->exit_latency *
> NSEC_PER_USEC to avoid potential overflow happenning.
> 
> Signed-off-by: C Cheng <[e-mail]>
> Signed-off-by: Bo Ye <[e-mail]>
> ---
> Change in v2:
> -remove Change-ID
> -correct patch author name
> -replace multiplication of two int values with mul_u32_u32
> -refine commit message
> ---
>  drivers/cpuidle/driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index d9cda7f6ccb9..cf5873cc45dc 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -16,6 +16,7 @@
>  #include <linux/cpumask.h>
>  #include <linux/tick.h>
>  #include <linux/cpu.h>
> +#include <linux/math64.h>
> 
>  #include "cpuidle.h"
> 
> @@ -187,7 +188,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
>  			s->target_residency = div_u64(s->target_residency_ns, NSEC_PER_USEC);
> 
>  		if (s->exit_latency > 0)
> -			s->exit_latency_ns = s->exit_latency * NSEC_PER_USEC;
> +			s->exit_latency_ns = mul_u32_u32(s->exit_latency, NSEC_PER_USEC);

Just force either side of the multiply to a 64bit unsigned type.
The compiler will then DTRT which is likely to be better code than
whatever mul_u32_u32() generates.

In any case is the 'exit_latency' ever going to be greater than 4 seconds?
In which case the 32bit multiply will never overflow.

	David

>  		else if (s->exit_latency_ns < 0)
>  			s->exit_latency_ns =  0;
>  		else
> --
> 2.18.0
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Rafael J. Wysocki Dec. 19, 2023, 4:30 p.m. UTC | #3

On Tue, Dec 19, 2023 at 5:24 PM David Laight <[e-mail]> wrote:
>
> From: Bo Ye
> > Sent: 19 December 2023 03:15
> >
> > From: C Cheng <[e-mail]>
> >
> > In detail:
> >
> > In C language, when you perform a multiplication operation, if
> > both operands are of int type, the multiplication operation is
> > performed on the int type, and then the result is converted to
> > the target type. This means that if the product of int type
> > multiplication exceeds the range that int type can represent,
> >  an overflow will occur even if you store the result in a
> > variable of int64_t type.
> >
> > For a multiplication of two int values, it is better to use
> > mul_u32_u32() rather than s->exit_latency_ns = s->exit_latency *
> > NSEC_PER_USEC to avoid potential overflow happenning.
> >
> > Signed-off-by: C Cheng <[e-mail]>
> > Signed-off-by: Bo Ye <[e-mail]>
> > ---
> > Change in v2:
> > -remove Change-ID
> > -correct patch author name
> > -replace multiplication of two int values with mul_u32_u32
> > -refine commit message
> > ---
> >  drivers/cpuidle/driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> > index d9cda7f6ccb9..cf5873cc45dc 100644
> > --- a/drivers/cpuidle/driver.c
> > +++ b/drivers/cpuidle/driver.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/cpumask.h>
> >  #include <linux/tick.h>
> >  #include <linux/cpu.h>
> > +#include <linux/math64.h>
> >
> >  #include "cpuidle.h"
> >
> > @@ -187,7 +188,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
> >                       s->target_residency = div_u64(s->target_residency_ns, NSEC_PER_USEC);
> >
> >               if (s->exit_latency > 0)
> > -                     s->exit_latency_ns = s->exit_latency * NSEC_PER_USEC;
> > +                     s->exit_latency_ns = mul_u32_u32(s->exit_latency, NSEC_PER_USEC);
>
> Just force either side of the multiply to a 64bit unsigned type.
> The compiler will then DTRT which is likely to be better code than
> whatever mul_u32_u32() generates.

So why is it there?

The default implementation of mul_u32_u32() is to cast its first
argument to u64 before the multiplication AFAICS.

> In any case is the 'exit_latency' ever going to be greater than 4 seconds?
> In which case the 32bit multiply will never overflow.

So this is more of a theoretical thing found by some static analysis
tool or similar.

> >               else if (s->exit_latency_ns < 0)
> >                       s->exit_latency_ns =  0;
> >               else
> > --

Rafael J. Wysocki Feb. 12, 2024, 4:09 p.m. UTC | #4

On Tue, Dec 19, 2023 at 11:56 AM AngeloGioacchino Del Regno
<[e-mail]> wrote:
>
> Il 19/12/23 04:14, Bo Ye ha scritto:
> > From: C Cheng <[e-mail]>
> >
> > In detail:
> >
> > In C language, when you perform a multiplication operation, if
> > both operands are of int type, the multiplication operation is
> > performed on the int type, and then the result is converted to
> > the target type. This means that if the product of int type
> > multiplication exceeds the range that int type can represent,
> >   an overflow will occur even if you store the result in a
> > variable of int64_t type.
> >
> > For a multiplication of two int values, it is better to use
> > mul_u32_u32() rather than s->exit_latency_ns = s->exit_latency *
> > NSEC_PER_USEC to avoid potential overflow happenning.
> >
> > Signed-off-by: C Cheng <[e-mail]>
> > Signed-off-by: Bo Ye <[e-mail]>
>
> Reviewed-by: AngeloGioacchino Del Regno <[e-mail]>

Applied as 6.9 material under a different subject (cpuidle: Avoid
potential overflow in integer multiplication), thanks!

Patch

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index d9cda7f6ccb9..cf5873cc45dc 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -16,6 +16,7 @@ 
 #include <linux/cpumask.h>
 #include <linux/tick.h>
 #include <linux/cpu.h>
+#include <linux/math64.h>
 
 #include "cpuidle.h"
 
@@ -187,7 +188,7 @@  static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 			s->target_residency = div_u64(s->target_residency_ns, NSEC_PER_USEC);
 
 		if (s->exit_latency > 0)
-			s->exit_latency_ns = s->exit_latency * NSEC_PER_USEC;
+			s->exit_latency_ns = mul_u32_u32(s->exit_latency, NSEC_PER_USEC);
 		else if (s->exit_latency_ns < 0)
 			s->exit_latency_ns =  0;
 		else