[v1] dirtylimit: Fix overflow when computing MB

[PATCH] dirtylimit: Fix overflow when computing MB

Posted by huangy81@chinatelecom.cn 1 year, 9 months ago

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

Coverity points out a overflow problem when computing MB,
dirty_ring_size and TARGET_PAGE_SIZE are both 32 bits,
multiplication will be done as a 32-bit operation, which
could overflow. Simplify the formula.

Meanwhile, fix spelling mistake of variable name.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 softmmu/dirtylimit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 8d98cb7..ab62f29 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -236,14 +236,14 @@ static inline int64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
 {
     static uint64_t max_dirtyrate;
     uint32_t dirty_ring_size = kvm_dirty_ring_size();
-    uint64_t dirty_ring_size_meory_MB =
-        dirty_ring_size * TARGET_PAGE_SIZE >> 20;
+    uint32_t dirty_ring_size_memory_MB =
+        dirty_ring_size >> (20 - TARGET_PAGE_BITS);
 
     if (max_dirtyrate < dirtyrate) {
         max_dirtyrate = dirtyrate;
     }
 
-    return dirty_ring_size_meory_MB * 1000000 / max_dirtyrate;
+    return dirty_ring_size_memory_MB * 1000000 / max_dirtyrate;
 }
 
 static inline bool dirtylimit_done(uint64_t quota,
-- 
1.8.3.1

Re: [PATCH] dirtylimit: Fix overflow when computing MB

Posted by Peter Maydell 1 year, 9 months ago

On Fri, 29 Jul 2022 at 16:17, <huangy81@chinatelecom.cn> wrote:
>
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> Coverity points out a overflow problem when computing MB,
> dirty_ring_size and TARGET_PAGE_SIZE are both 32 bits,
> multiplication will be done as a 32-bit operation, which
> could overflow. Simplify the formula.
>
> Meanwhile, fix spelling mistake of variable name.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  softmmu/dirtylimit.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
> index 8d98cb7..ab62f29 100644
> --- a/softmmu/dirtylimit.c
> +++ b/softmmu/dirtylimit.c
> @@ -236,14 +236,14 @@ static inline int64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
>  {
>      static uint64_t max_dirtyrate;
>      uint32_t dirty_ring_size = kvm_dirty_ring_size();
> -    uint64_t dirty_ring_size_meory_MB =
> -        dirty_ring_size * TARGET_PAGE_SIZE >> 20;
> +    uint32_t dirty_ring_size_memory_MB =
> +        dirty_ring_size >> (20 - TARGET_PAGE_BITS);
>
>      if (max_dirtyrate < dirtyrate) {
>          max_dirtyrate = dirtyrate;
>      }
>
> -    return dirty_ring_size_meory_MB * 1000000 / max_dirtyrate;
> +    return dirty_ring_size_memory_MB * 1000000 / max_dirtyrate;

Now you've changed dirty_ring_size_memory_MB to 32 bits,
this multiplication is going to be done at 32 bit
precision and can overflow. Adding 'ULL' to the '1000000'
is one way to fix that.

thanks
-- PMM