Re: [PATCH] blk-throttle: Calculate allowed value only when the throttle is enabled

[Khazhy Kumykov]

On Wed, Sep 27, 2023 at 7:05 PM <linan666@xxxxxxxxxxxxxxx> wrote:
>
> From: Li Nan <linan122@xxxxxxxxxx>
>
> When the throttle of bps is not enabled, tg_bps_limit() returns U64_MAX,
> which is be used in calculate_bytes_allowed(), and divide 0 error will
> happen.
>
> To fix it, only calculate allowed value when the throttle of bps/iops is
> enabled and the value will be used.
>
> Fixes: e8368b57c006 ("blk-throttle: use calculate_io/bytes_allowed() for throtl_trim_slice()")
> Reported-by: Changhui Zhong <czhong@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/CAGVVp+Vt6idZtxfU9jF=VSbu145Wi-d-WnAZx_hEfOL8yLZgBA@xxxxxxxxxxxxxx
> Signed-off-by: Li Nan <linan122@xxxxxxxxxx>
> ---
>  block/blk-throttle.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 38a881cf97d0..3c9a74ab9f0e 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -730,8 +730,10 @@ static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed)
>  static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
>  {
>         unsigned long time_elapsed;
> -       long long bytes_trim;
> -       int io_trim;
> +       long long bytes_trim = 0;
> +       int io_trim = 0;
> +       u64 bps_limit;
> +       u32 iops_limit;
>
>         BUG_ON(time_before(tg->slice_end[rw], tg->slice_start[rw]));
>
> @@ -758,11 +760,14 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
>         if (!time_elapsed)
>                 return;
>
> -       bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw),
> -                                            time_elapsed) +
> -                    tg->carryover_bytes[rw];
> -       io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed) +
> -                 tg->carryover_ios[rw];
> +       bps_limit = tg_bps_limit(tg, rw);
> +       iops_limit = tg_iops_limit(tg, rw);
> +       if (tg->bytes_disp[rw] > 0 && bps_limit != U64_MAX)
I don't think this change is sufficient to prevent kernel crash, as a
"clever" user could still set the bps_limit to U64_MAX - 1 (or another
large value), which probably would still result in the same crash. The
comment in mul_u64_u64_div_u64 suggests there's something we can do to
better handle the overflow case, but I'm not sure what it's referring
to. ("Will generate an #DE when the result doesn't fit u64, could fix
with an __ex_table[] entry when it becomes an issue.") Otherwise, we
probably need to remove the mul_u64_u64_div_u64 and check for
overflow/potential overflow ourselves?

Khazhy
> +               bytes_trim = calculate_bytes_allowed(bps_limit,
> +                            time_elapsed) + tg->carryover_bytes[rw];
> +       if (tg->io_disp[rw] > 0 && iops_limit != UINT_MAX)
> +               io_trim = calculate_io_allowed(iops_limit, time_elapsed) +
> +                         tg->carryover_ios[rw];
>         if (bytes_trim <= 0 && io_trim <= 0)
>                 return;
>
> --
> 2.39.2
>