Re: [PATCH] target/arm: Add overflow check for gt_recalc_timer

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 6 Apr 2023 at 16:16, Leonid Komarianskyi
<Leonid_Komarianskyi@epam.com> wrote:
>
> If gt_timer is enabled before cval initialization on a virtualized
> setup on QEMU, cval equals (UINT64_MAX - 1). Adding an offset value
> to this causes an overflow that sets timer into the past, which leads
> to infinite loop, because this timer fires immediately and calls
> gt_recalc_timer() once more, which in turn sets the timer into the
> past again and as a result, QEMU hangs. This patch adds check for
> overflowing of the nexttick variable.

This is https://gitlab.com/qemu-project/qemu/-/issues/60 --
thanks for sending a patch.

> Suggested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Co-Authored-By: Dmytro Firsov <dmytro_firsov@epam.com>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
> ---
>  target/arm/helper.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2297626bfb..2fbba15040 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2618,6 +2618,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>          int istatus = count - offset >= gt->cval;
>          uint64_t nexttick;
>          int irqstate;
> +        bool nexttick_overflow = false;
>
>          gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
>
> @@ -2630,6 +2631,16 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>          } else {
>              /* Next transition is when we hit cval */
>              nexttick = gt->cval + offset;
> +            if (nexttick < offset) {
> +                /*
> +                 * If gt->cval value is close to UINT64_MAX then adding
> +                 * to it offset can lead to overflow of nexttick variable.
> +                 * So, this check tests that arguments sum is less than any
> +                 * addend, and in case it is overflowed we have to mod timer
> +                 * to INT64_MAX.
> +                 */
> +                nexttick_overflow = true;
> +            }

Rather than adding in a bool, I think I prefer the version
of the patch in one of the comments to the bug report:

             /* Next transition is when we hit cval */
             nexttick = gt->cval + offset;
+            if (nexttick < gt->cval) {
+                nexttick = UINT64_MAX;
+            }

i.e. we just saturate nexttick, and then let the existing handling
of "turns out nexttick is too big" handle things.

There is also a comment or two from me in the bug report pointing
out that the handling of wraparound is also wrong in the other
half of this if(); we should look at that too.

>          }
>          /*
>           * Note that the desired next expiry time might be beyond the
> @@ -2637,7 +2648,8 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>           * set the timer for as far in the future as possible. When the
>           * timer expires we will reset the timer for any remaining period.
>           */
> -        if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
> +        if ((nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu))
> +             || nexttick_overflow) {
>              timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
>          } else {
>              timer_mod(cpu->gt_timer[timeridx], nexttick);
> --
> 2.25.1

thanks
-- PMM