[RH7] vziolimit: fix race by putting throttle.speed check under ub_lock

Submitted by Pavel Tikhomirov on June 28, 2019, 11:47 a.m.

Details

Message ID 20190628114701.13075-1-ptikhomirov@virtuozzo.com
State New
Series "vziolimit: fix race by putting throttle.speed check under ub_lock"
Headers show

Commit Message

Pavel Tikhomirov June 28, 2019, 11:47 a.m.
We had a crash on these stack:

iolimit_balance_dirty
  if (!th->speed) # <- th->speed is non-zero here
    return;
  ...
  throttle_charge(th, (long long)dirty << PAGE_SHIFT);
    ...
    if (do_div(step, th->speed)) # <- th->speed is zero here, crash

We don't change th->speed in between, throttle.speed is changed from:

1) iolimit_cgroup_write_u64 (cgroup beancounter.iolimit.speed)
2) and iolimit_ioctl->throttle_setup

Both are under spin_lock_irq(&ub->ub_lock); so putting the check under
the same lock should fix the race.

https://jira.sw.ru/browse/PSBM-95815
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 kernel/ve/vziolimit.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/kernel/ve/vziolimit.c b/kernel/ve/vziolimit.c
index d1cd687ab1f1..d6bbc8c1dd89 100644
--- a/kernel/ve/vziolimit.c
+++ b/kernel/ve/vziolimit.c
@@ -138,9 +138,6 @@  static void iolimit_balance_dirty(struct iolimit *iolimit,
 	struct throttle *th = &iolimit->throttle;
 	unsigned long flags, dirty, state;
 
-	if (!th->speed)
-		return;
-
 	/* can be non-atomic on i386, but ok. this just hint. */
 	state = th->state >> PAGE_SHIFT;
 	dirty = ub_stat_get(ub, dirty_pages) + write_chunk;
@@ -154,7 +151,8 @@  static void iolimit_balance_dirty(struct iolimit *iolimit,
 
 	spin_lock_irqsave(&ub->ub_lock, flags);
 	/* precharge dirty pages */
-	throttle_charge(th, (long long)dirty << PAGE_SHIFT);
+	if (th->speed)
+		throttle_charge(th, (long long)dirty << PAGE_SHIFT);
 	spin_unlock_irqrestore(&ub->ub_lock, flags);
 }
 

Comments

Konstantin Khorenko June 28, 2019, 12:34 p.m.
On 06/28/2019 02:47 PM, Pavel Tikhomirov wrote:
> We had a crash on these stack:
>
> iolimit_balance_dirty
>   if (!th->speed) # <- th->speed is non-zero here
>     return;
>   ...
>   throttle_charge(th, (long long)dirty << PAGE_SHIFT);
>     ...
>     if (do_div(step, th->speed)) # <- th->speed is zero here, crash
>
> We don't change th->speed in between, throttle.speed is changed from:
>
> 1) iolimit_cgroup_write_u64 (cgroup beancounter.iolimit.speed)
> 2) and iolimit_ioctl->throttle_setup
>
> Both are under spin_lock_irq(&ub->ub_lock); so putting the check under
> the same lock should fix the race.
>
> https://jira.sw.ru/browse/PSBM-95815
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

Acked-by: Konstantin Khorenko <khorenko@virtuozzo.com>

> ---
>  kernel/ve/vziolimit.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/ve/vziolimit.c b/kernel/ve/vziolimit.c
> index d1cd687ab1f1..d6bbc8c1dd89 100644
> --- a/kernel/ve/vziolimit.c
> +++ b/kernel/ve/vziolimit.c
> @@ -138,9 +138,6 @@ static void iolimit_balance_dirty(struct iolimit *iolimit,
>  	struct throttle *th = &iolimit->throttle;
>  	unsigned long flags, dirty, state;
>
> -	if (!th->speed)
> -		return;
> -
>  	/* can be non-atomic on i386, but ok. this just hint. */
>  	state = th->state >> PAGE_SHIFT;
>  	dirty = ub_stat_get(ub, dirty_pages) + write_chunk;
> @@ -154,7 +151,8 @@ static void iolimit_balance_dirty(struct iolimit *iolimit,
>
>  	spin_lock_irqsave(&ub->ub_lock, flags);
>  	/* precharge dirty pages */
> -	throttle_charge(th, (long long)dirty << PAGE_SHIFT);
> +	if (th->speed)
> +		throttle_charge(th, (long long)dirty << PAGE_SHIFT);
>  	spin_unlock_irqrestore(&ub->ub_lock, flags);
>  }
>
>