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
>
