[PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[Toshiaki Makita]

cfq_group_service_tree_add() is applying new_weight at the beginning of
the function via cfq_update_group_weight().
This actually allows weight to change between adding it to and subtracting
it from children_weight, and triggers WARN_ON_ONCE() in
cfq_group_service_tree_del(), or even causes oops by divide error during
vfr calculation in cfq_group_service_tree_add().

The detailed scenario is as follows:
1. Create blkio cgroups P and P's child C.
   Set P's weight to 500 and perform some I/O to apply new_weight.
   This P's I/O completes before starting C's I/O.
2. C starts I/O and cfq_group_service_tree_add() is called with C.
3. cfq_group_service_tree_add() walks up the tree during children_weight
   calculation and adds parent P's weight (500) to children_weight of root.
   children_weight becomes 500.
4. Set P's weight to 1000.
5. P starts I/O and cfq_group_service_tree_add() is called with P.
6. cfq_group_service_tree_add() applies its new_weight (1000).
7. I/O of C completes and cfq_group_service_tree_del() is called with C.
8. I/O of P completes and cfq_group_service_tree_del() is called with P.
9. cfq_group_service_tree_del() subtracts P's weight (1000) from
   children_weight of root. children_weight becomes -500.
   This triggers WARN_ON_ONCE().
10. Set P's weight to 500.
11. P starts I/O and cfq_group_service_tree_add() is called with P.
12. cfq_group_service_tree_add() applies its new_weight (500) and adds it
    to children_weight of root. children_weight becomes 0. Calculation of
    vfr triggers oops by divide error.

weight should be updated right before adding it to children_weight.

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Reported-by: Ruki Sekiya <sekiya.ruki-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
Signed-off-by: Toshiaki Makita <makita.toshiaki-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
v2:
- Add comments in the code.
- Reword cgroup names in changelog.

 block/cfq-iosched.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index cadc378..faf175e 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1272,15 +1272,22 @@ __cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
 	rb_insert_color(&cfqg->rb_node, &st->rb);
 }
 
+/*
+ * This has to be called only on activation of cfqg
+ */
 static void
 cfq_update_group_weight(struct cfq_group *cfqg)
 {
-	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
-
 	if (cfqg->new_weight) {
 		cfqg->weight = cfqg->new_weight;
 		cfqg->new_weight = 0;
 	}
+}
+
+static void
+cfq_update_group_leaf_weight(struct cfq_group *cfqg)
+{
+	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
 
 	if (cfqg->new_leaf_weight) {
 		cfqg->leaf_weight = cfqg->new_leaf_weight;
@@ -1299,7 +1306,11 @@ cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
 	/* add to the service tree */
 	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
 
-	cfq_update_group_weight(cfqg);
+	/*
+	 * Update leaf_weight.  We cannot update weight at this point
+	 * because cfqg might already have been activated by its child.
+	 */
+	cfq_update_group_leaf_weight(cfqg);
 	__cfq_group_service_tree_add(st, cfqg);
 
 	/*
@@ -1323,6 +1334,7 @@ cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
 	 */
 	while ((parent = cfqg_parent(pos))) {
 		if (propagate) {
+			cfq_update_group_weight(pos);
 			propagate = !parent->nr_active++;
 			parent->children_weight += pos->weight;
 		}
-- 
1.8.1.2

Re: [PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[Vivek Goyal]

On Wed, Aug 27, 2014 at 05:32:32PM +0900, Toshiaki Makita wrote:
> cfq_group_service_tree_add() is applying new_weight at the beginning of
> the function via cfq_update_group_weight().
> This actually allows weight to change between adding it to and subtracting
> it from children_weight, and triggers WARN_ON_ONCE() in
> cfq_group_service_tree_del(), or even causes oops by divide error during
> vfr calculation in cfq_group_service_tree_add().
> 
> The detailed scenario is as follows:
> 1. Create blkio cgroups P and P's child C.
>    Set P's weight to 500 and perform some I/O to apply new_weight.
>    This P's I/O completes before starting C's I/O.
> 2. C starts I/O and cfq_group_service_tree_add() is called with C.
> 3. cfq_group_service_tree_add() walks up the tree during children_weight
>    calculation and adds parent P's weight (500) to children_weight of root.
>    children_weight becomes 500.
> 4. Set P's weight to 1000.
> 5. P starts I/O and cfq_group_service_tree_add() is called with P.
> 6. cfq_group_service_tree_add() applies its new_weight (1000).
> 7. I/O of C completes and cfq_group_service_tree_del() is called with C.
> 8. I/O of P completes and cfq_group_service_tree_del() is called with P.
> 9. cfq_group_service_tree_del() subtracts P's weight (1000) from
>    children_weight of root. children_weight becomes -500.
>    This triggers WARN_ON_ONCE().
> 10. Set P's weight to 500.
> 11. P starts I/O and cfq_group_service_tree_add() is called with P.
> 12. cfq_group_service_tree_add() applies its new_weight (500) and adds it
>     to children_weight of root. children_weight becomes 0. Calculation of
>     vfr triggers oops by divide error.
> 
> weight should be updated right before adding it to children_weight.
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Reported-by: Ruki Sekiya <sekiya.ruki-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> Signed-off-by: Toshiaki Makita <makita.toshiaki-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Hi Toshiaki,

Thanks for the fix. Somebody opened a bug for this and attached similar
patch. I was waiting for customer feedback before posting the patch. Good
that you beat me to it.

Acked-by: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks
Vivek

> ---
> v2:
> - Add comments in the code.
> - Reword cgroup names in changelog.
> 
>  block/cfq-iosched.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index cadc378..faf175e 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1272,15 +1272,22 @@ __cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
>  	rb_insert_color(&cfqg->rb_node, &st->rb);
>  }
>  
> +/*
> + * This has to be called only on activation of cfqg
> + */
>  static void
>  cfq_update_group_weight(struct cfq_group *cfqg)
>  {
> -	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
> -
>  	if (cfqg->new_weight) {
>  		cfqg->weight = cfqg->new_weight;
>  		cfqg->new_weight = 0;
>  	}
> +}
> +
> +static void
> +cfq_update_group_leaf_weight(struct cfq_group *cfqg)
> +{
> +	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
>  
>  	if (cfqg->new_leaf_weight) {
>  		cfqg->leaf_weight = cfqg->new_leaf_weight;
> @@ -1299,7 +1306,11 @@ cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
>  	/* add to the service tree */
>  	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
>  
> -	cfq_update_group_weight(cfqg);
> +	/*
> +	 * Update leaf_weight.  We cannot update weight at this point
> +	 * because cfqg might already have been activated by its child.
> +	 */
> +	cfq_update_group_leaf_weight(cfqg);
>  	__cfq_group_service_tree_add(st, cfqg);
>  
>  	/*
> @@ -1323,6 +1334,7 @@ cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
>  	 */
>  	while ((parent = cfqg_parent(pos))) {
>  		if (propagate) {
> +			cfq_update_group_weight(pos);
>  			propagate = !parent->nr_active++;
>  			parent->children_weight += pos->weight;
>  		}
> -- 
> 1.8.1.2
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[Toshiaki Makita]

(2014/08/27 21:07), Vivek Goyal wrote:
> On Wed, Aug 27, 2014 at 05:32:32PM +0900, Toshiaki Makita wrote:
...
> Hi Toshiaki,
> 
> Thanks for the fix. Somebody opened a bug for this and attached similar
> patch. I was waiting for customer feedback before posting the patch. Good
> that you beat me to it.
> 
> Acked-by: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Thanks
> Vivek

I had googled it before posting the patch, but couldn't find any bug
report including patches.
Thanks for letting me know.

Toshiaki Makita

Re: [PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[Jens Axboe]

On 08/27/2014 02:32 AM, Toshiaki Makita wrote:
> cfq_group_service_tree_add() is applying new_weight at the beginning of
> the function via cfq_update_group_weight().
> This actually allows weight to change between adding it to and subtracting
> it from children_weight, and triggers WARN_ON_ONCE() in
> cfq_group_service_tree_del(), or even causes oops by divide error during
> vfr calculation in cfq_group_service_tree_add().
> 
> The detailed scenario is as follows:
> 1. Create blkio cgroups P and P's child C.
>    Set P's weight to 500 and perform some I/O to apply new_weight.
>    This P's I/O completes before starting C's I/O.
> 2. C starts I/O and cfq_group_service_tree_add() is called with C.
> 3. cfq_group_service_tree_add() walks up the tree during children_weight
>    calculation and adds parent P's weight (500) to children_weight of root.
>    children_weight becomes 500.
> 4. Set P's weight to 1000.
> 5. P starts I/O and cfq_group_service_tree_add() is called with P.
> 6. cfq_group_service_tree_add() applies its new_weight (1000).
> 7. I/O of C completes and cfq_group_service_tree_del() is called with C.
> 8. I/O of P completes and cfq_group_service_tree_del() is called with P.
> 9. cfq_group_service_tree_del() subtracts P's weight (1000) from
>    children_weight of root. children_weight becomes -500.
>    This triggers WARN_ON_ONCE().
> 10. Set P's weight to 500.
> 11. P starts I/O and cfq_group_service_tree_add() is called with P.
> 12. cfq_group_service_tree_add() applies its new_weight (500) and adds it
>     to children_weight of root. children_weight becomes 0. Calculation of
>     vfr triggers oops by divide error.
> 
> weight should be updated right before adding it to children_weight.

I already queued up the previous one yesterday. I'd welcome the
comments, but could you send them against the previous one?

-- 
Jens Axboe

Re: [PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[Toshiaki Makita]

(2014/08/27 23:10), Jens Axboe wrote:
...
> I already queued up the previous one yesterday. I'd welcome the
> comments, but could you send them against the previous one?

I was not aware of that...
Sure, will respin.

Thanks,
Toshiaki Makita

Re: [PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[Tejun Heo]

As it looks like the patch needs to be respun anyway.

On Wed, Aug 27, 2014 at 05:32:32PM +0900, Toshiaki Makita wrote:
> +	/*
> +	 * Update leaf_weight.  We cannot update weight at this point
> +	 * because cfqg might already have been activated by its child.

Maybe something like the following is better?

 because cfqg might already have been activated and is contributing
 its current weight to the parent's child_weight.

Thanks.

-- 
tejun

Re: [PATCH v2 RESEND] cfq-iosched: Fix wrong children_weight calculation

[Toshiaki Makita]

(2014/08/27 23:15), Tejun Heo wrote:
> As it looks like the patch needs to be respun anyway.
> 
> On Wed, Aug 27, 2014 at 05:32:32PM +0900, Toshiaki Makita wrote:
>> +	/*
>> +	 * Update leaf_weight.  We cannot update weight at this point
>> +	 * because cfqg might already have been activated by its child.
> 
> Maybe something like the following is better?
> 
>  because cfqg might already have been activated and is contributing
>  its current weight to the parent's child_weight.
> 

Thank you for your comment.
It looks better.
I'll reword it.

Thanks,
Toshiaki Makita