1. Patchew
  2. linux
  3. View series

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

linan666@huaweicloud.com posted 1 patch 2 years, 2 months ago 
block/blk-throttle.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
[PATCH] blk-throttle: Calculate allowed value only when the throttle is enabled
Posted by linan666@huaweicloud.com 2 years, 2 months ago 
From: Li Nan <linan122@huawei.com>

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@redhat.com>
Closes: https://lore.kernel.org/all/CAGVVp+Vt6idZtxfU9jF=VSbu145Wi-d-WnAZx_hEfOL8yLZgBA@mail.gmail.com
Signed-off-by: Li Nan <linan122@huawei.com>
---
 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)
+		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
Re: [PATCH] blk-throttle: Calculate allowed value only when the throttle is enabled
Posted by Khazhy Kumykov 2 years, 2 months ago 
On Wed, Sep 27, 2023 at 7:05 PM <linan666@huaweicloud.com> wrote:
>
> From: Li Nan <linan122@huawei.com>
>
> 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@redhat.com>
> Closes: https://lore.kernel.org/all/CAGVVp+Vt6idZtxfU9jF=VSbu145Wi-d-WnAZx_hEfOL8yLZgBA@mail.gmail.com
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>  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
>
Re: [PATCH] blk-throttle: Calculate allowed value only when the throttle is enabled
Posted by Tejun Heo 2 years, 2 months ago 
Hello,

On Thu, Sep 28, 2023 at 09:58:58AM +0800, linan666@huaweicloud.com wrote:
> From: Li Nan <linan122@huawei.com>
> 
> 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.

calculate_bytes_allowed() is just

  return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ); 

The only division is by HZ. How does divide by 0 happen?

Thanks.

-- 
tejun
Re: [PATCH] blk-throttle: Calculate allowed value only when the throttle is enabled
Posted by Khazhy Kumykov 2 years, 2 months ago 
On Wed, Oct 4, 2023 at 12:32 PM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Thu, Sep 28, 2023 at 09:58:58AM +0800, linan666@huaweicloud.com wrote:
> > From: Li Nan <linan122@huawei.com>
> >
> > 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.
>
> calculate_bytes_allowed() is just
>
>   return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
>
> The only division is by HZ. How does divide by 0 happen?

We've also noticed this - haven't looked too deeply but I don't think
it's a divide by zero, but an overflow (bps_limit * jiffy_elapsed / HZ
will overflow for jiffies > HZ). mul_u64_u64_div_u64 does say it will
throw DE if the mul overflows

>
> Thanks.
>
> --
> tejun

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.