Fix use-after-free of q->root_blkg and q->root_rl.blkg

PATCH In review Jun'ichi Nomura 4 participants 18 messages Oct 10, 2012 – Oct 22, 2012

Subsystems (2)

Block layer cgroup blkio

lore

Share & tools

lore

Jun'ichi Nomura

author

2012-10-10 05:20 UTC #0 lore raw

PATCH 1/1 Fix use-after-free of q->root_blkg and q->root_rl.blkg

I got system stall after the following warning with 3.6:

Quoted 27 lines

WARNING: at /work/build/linux/block/blk-cgroup.h:250 blk_put_rl+0x4d/0x95()
Modules linked in: bridge stp llc sunrpc acpi_cpufreq freq_table mperf ipt_REJEC
T nf_conntrack_ipv4 nf_defrag_ipv4
Pid: 0, comm: swapper/0 Not tainted 3.6.0 #1
Call Trace:
 <IRQ>  [<ffffffff810453bd>] warn_slowpath_common+0x85/0x9d
 [<ffffffff810453ef>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff811d5f8d>] blk_put_rl+0x4d/0x95
 [<ffffffff811d614a>] __blk_put_request+0xc3/0xcb
 [<ffffffff811d71a3>] blk_finish_request+0x232/0x23f
 [<ffffffff811d76c3>] ? blk_end_bidi_request+0x34/0x5d
 [<ffffffff811d76d1>] blk_end_bidi_request+0x42/0x5d
 [<ffffffff811d7728>] blk_end_request+0x10/0x12
 [<ffffffff812cdf16>] scsi_io_completion+0x207/0x4d5
 [<ffffffff812c6fcf>] scsi_finish_command+0xfa/0x103
 [<ffffffff812ce2f8>] scsi_softirq_done+0xff/0x108
 [<ffffffff811dcea5>] blk_done_softirq+0x8d/0xa1
 [<ffffffff810915d5>] ? generic_smp_call_function_single_interrupt+0x9f/0xd7
 [<ffffffff8104cf5b>] __do_softirq+0x102/0x213
 [<ffffffff8108a5ec>] ? lock_release_holdtime+0xb6/0xbb
 [<ffffffff8104d2b4>] ? raise_softirq_irqoff+0x9/0x3d
 [<ffffffff81424dfc>] call_softirq+0x1c/0x30
 [<ffffffff81011beb>] do_softirq+0x4b/0xa3
 [<ffffffff8104cdb0>] irq_exit+0x53/0xd5
 [<ffffffff8102d865>] smp_call_function_single_interrupt+0x34/0x36
 [<ffffffff8142486f>] call_function_single_interrupt+0x6f/0x80
 <EOI>  [<ffffffff8101800b>] ? mwait_idle+0x94/0xcd

Quoted 8 lines

 [<ffffffff81018002>] ? mwait_idle+0x8b/0xcd
 [<ffffffff81017811>] cpu_idle+0xbb/0x114
 [<ffffffff81401fbd>] rest_init+0xc1/0xc8
 [<ffffffff81401efc>] ? csum_partial_copy_generic+0x16c/0x16c
 [<ffffffff81cdbd3d>] start_kernel+0x3d4/0x3e1
 [<ffffffff81cdb79e>] ? kernel_init+0x1f7/0x1f7
 [<ffffffff81cdb2dd>] x86_64_start_reservations+0xb8/0xbd
 [<ffffffff81cdb3e3>] x86_64_start_kernel+0x101/0x110

blk_put_rl() does this:

         if (rl->blkg && rl->blkg->blkcg != &blkcg_root)
                 blkg_put(rl->blkg);

but if rl is q->root_rl, rl->blkg might be a bogus pointer because blkcg_deactivate_policy() does not clear q->root_rl.blkg after blkg_destroy_all().

Attached patch works for me.

Signed-off-by Jun'ichi Nomura <[email protected]>

block/blk-cgroup.c +4 -1

index f3b44a6..5015764 100644
@@ -285,6 +285,9 @@ static void blkg_destroy_all(struct request_queue *q)
 		blkg_destroy(blkg);
 		spin_unlock(&blkcg->lock);
 	}
+
+	q->root_blkg = NULL;
+	q->root_rl.blkg = NULL;
 }
 
 static void blkg_rcu_free(struct rcu_head *rcu_head)

@@ -333,7 +336,7 @@ struct request_list *__blk_queue_next_rl(struct request_list *rl,
 
 	/* walk to the next list_head, skip root blkcg */
 	ent = ent->next;
-	if (ent == &q->root_blkg->q_node)
+	if (q->root_blkg && ent == &q->root_blkg->q_node)
 		ent = ent->next;
 	if (ent == &q->blkg_list)
 		return NULL;

Vivek Goyal

2012-10-10 15:59 UTC #1 lore raw

Quoted 38 lines · Jun'ichi Nomura

I got system stall after the following warning with 3.6:

Quoted 27 lines

WARNING: at /work/build/linux/block/blk-cgroup.h:250 blk_put_rl+0x4d/0x95()
Modules linked in: bridge stp llc sunrpc acpi_cpufreq freq_table mperf ipt_REJEC
T nf_conntrack_ipv4 nf_defrag_ipv4
Pid: 0, comm: swapper/0 Not tainted 3.6.0 #1
Call Trace:
 <IRQ>  [<ffffffff810453bd>] warn_slowpath_common+0x85/0x9d
 [<ffffffff810453ef>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff811d5f8d>] blk_put_rl+0x4d/0x95
 [<ffffffff811d614a>] __blk_put_request+0xc3/0xcb
 [<ffffffff811d71a3>] blk_finish_request+0x232/0x23f
 [<ffffffff811d76c3>] ? blk_end_bidi_request+0x34/0x5d
 [<ffffffff811d76d1>] blk_end_bidi_request+0x42/0x5d
 [<ffffffff811d7728>] blk_end_request+0x10/0x12
 [<ffffffff812cdf16>] scsi_io_completion+0x207/0x4d5
 [<ffffffff812c6fcf>] scsi_finish_command+0xfa/0x103
 [<ffffffff812ce2f8>] scsi_softirq_done+0xff/0x108
 [<ffffffff811dcea5>] blk_done_softirq+0x8d/0xa1
 [<ffffffff810915d5>] ? generic_smp_call_function_single_interrupt+0x9f/0xd7
 [<ffffffff8104cf5b>] __do_softirq+0x102/0x213
 [<ffffffff8108a5ec>] ? lock_release_holdtime+0xb6/0xbb
 [<ffffffff8104d2b4>] ? raise_softirq_irqoff+0x9/0x3d
 [<ffffffff81424dfc>] call_softirq+0x1c/0x30
 [<ffffffff81011beb>] do_softirq+0x4b/0xa3
 [<ffffffff8104cdb0>] irq_exit+0x53/0xd5
 [<ffffffff8102d865>] smp_call_function_single_interrupt+0x34/0x36
 [<ffffffff8142486f>] call_function_single_interrupt+0x6f/0x80
 <EOI>  [<ffffffff8101800b>] ? mwait_idle+0x94/0xcd

Quoted 8 lines

 [<ffffffff81018002>] ? mwait_idle+0x8b/0xcd
 [<ffffffff81017811>] cpu_idle+0xbb/0x114
 [<ffffffff81401fbd>] rest_init+0xc1/0xc8
 [<ffffffff81401efc>] ? csum_partial_copy_generic+0x16c/0x16c
 [<ffffffff81cdbd3d>] start_kernel+0x3d4/0x3e1
 [<ffffffff81cdb79e>] ? kernel_init+0x1f7/0x1f7
 [<ffffffff81cdb2dd>] x86_64_start_reservations+0xb8/0xbd
 [<ffffffff81cdb3e3>] x86_64_start_kernel+0x101/0x110

Quoted 8 lines

blk_put_rl() does this:
         if (rl->blkg && rl->blkg->blkcg != &blkcg_root)
                 blkg_put(rl->blkg);
but if rl is q->root_rl, rl->blkg might be a bogus pointer because blkcg_deactivate_policy() does not clear q->root_rl.blkg after blkg_destroy_all().
Attached patch works for me.

I think patch looks reasonable to me. Just that some more description would be nice. In fact, I will prefer some code comments too as I had to scratch my head for a while to figure out how did we reach here.

So looks like we deactivated cfq policy (most likely changed IO scheduler). That will destroy all the block groups (disconnect blkg from list and drop policy reference on group). If there are any pending IOs, then group will not be destroyed till IO is completed. (Because of cfqq reference on blkg and because of request list reference on blkg).

Now, all request list take a refenrece on associated blkg except q->root_rl. This means when last IO finished, it must have dropped the reference on cfqq which will drop reference on associated cfqg/blkg and immediately root blkg will be destroyed. And now we will call blk_put_rl() and that will try to access root_rl>blkg which has been just freed as last IO completed.

So problem here is that we don't take request list reference on root blkg and that creates all these corner cases.

So clearing q->root_blkg and q->root_rl.blkg during policy activation makes sense. That means that from queue and request list point of view root blkg is gone and you can't get to it. (It might still be around for some more time due to pending IOs though).

Some minor comments below.

Quoted 8 lines

Signed-off-by Jun'ichi Nomura <[email protected]>
block/blk-cgroup.c +0 -0
index f3b44a6..5015764 100644

block/blk-cgroup.c static void blkg_destroy_all(struct request_queue *q) · line 290

Quoted 6 lines

block/blk-cgroup.c +3 -0

@@ -285,6 +285,9 @@ static void blkg_destroy_all(struct request_queue *q)
 		blkg_destroy(blkg);
 		spin_unlock(&blkcg->lock);
 	}
+
+	q->root_blkg = NULL;
+	q->root_rl.blkg = NULL;

I think some of the above description about we not taking root_rl reference on root group can go here so that next time I don't have to scratch my head for a long time.

block/blk-cgroup.c struct request_list *__blk_queue_next_rl(struct request_list *rl, · line 339

Quoted 9 lines

block/blk-cgroup.c +1 -1

 }
 
 static void blkg_rcu_free(struct rcu_head *rcu_head)

@@ -333,7 +336,7 @@ struct request_list *__blk_queue_next_rl(struct request_list *rl,
 
 	/* walk to the next list_head, skip root blkcg */
 	ent = ent->next;
-	if (ent == &q->root_blkg->q_node)
+	if (q->root_blkg && ent == &q->root_blkg->q_node)

Can we fix it little differently. Little earlier in the code, we check for if q->blkg_list is empty, then all the groups are gone, and there are no more request lists hence and return NULL.

Current code:

        if (rl == &q->root_rl) {
                ent = &q->blkg_list;

Modified code:

        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
		/* There are no more block groups, hence no request lists */
		if (list_empty(ent))
			return NULL;
	}

Thanks
Vivek

Jun'ichi Nomura

author

2012-10-11 01:32 UTC #2 lore raw

Hi Vivek, thank you for comments.

Quoted 11 lines · Vivek Goyal

I think patch looks reasonable to me. Just that some more description would be nice. In fact, I will prefer some code comments too as I had to scratch my head for a while to figure out how did we reach here.
So looks like we deactivated cfq policy (most likely changed IO scheduler). That will destroy all the block groups (disconnect blkg from list and drop policy reference on group). If there are any pending IOs, then group will not be destroyed till IO is completed. (Because of cfqq reference on blkg and because of request list reference on blkg).

Quoted 6 lines

Now, all request list take a refenrece on associated blkg except q->root_rl. This means when last IO finished, it must have dropped the reference on cfqq which will drop reference on associated cfqg/blkg and immediately root blkg will be destroyed. And now we will call blk_put_rl() and that will try to access root_rl>blkg which has been just freed as last IO completed.

Yes, and for completion of any new IOs, blk_put_rl() is misled.

I'll try to extend the description according to your comments.

Quoted 14 lines

So problem here is that we don't take request list reference on root blkg and that creates all these corner cases.
So clearing q->root_blkg and q->root_rl.blkg during policy activation makes sense. That means that from queue and request list point of view root blkg is gone and you can't get to it. (It might still be around for some more time due to pending IOs though).
Some minor comments below.
Quoted 3 lines
Signed-off-by Jun'ichi Nomura <[email protected]>

Quoted 15 lines

block/blk-cgroup.c static void blkg_destroy_all(struct request_queue *q) · line 290
Quoted 11 lines
block/blk-cgroup.c +3 -0
index f3b44a6..5015764 100644
@@ -285,6 +285,9 @@ static void blkg_destroy_all(struct request_queue *q)
 		blkg_destroy(blkg);
 		spin_unlock(&blkcg->lock);
 	}
+
+	q->root_blkg = NULL;
+	q->root_rl.blkg = NULL;
I think some of the above description about we not taking root_rl reference on root group can go here so that next time I don't have to scratch my head for a long time.

I put the following comment:
/*
* root blkg is destroyed. Just clear the pointer since
* root_rl does not take reference on root blkg. */

Quoted 19 lines

block/blk-cgroup.c struct request_list *__blk_queue_next_rl(struct request_list *rl, · line 339
Quoted 10 lines
block/blk-cgroup.c +1 -1
 }
 
 static void blkg_rcu_free(struct rcu_head *rcu_head)
@@ -333,7 +336,7 @@ struct request_list *__blk_queue_next_rl(struct request_list *rl,
 
 	/* walk to the next list_head, skip root blkcg */
 	ent = ent->next;
-	if (ent == &q->root_blkg->q_node)
+	if (q->root_blkg && ent == &q->root_blkg->q_node)
Can we fix it little differently. Little earlier in the code, we check for if q->blkg_list is empty, then all the groups are gone, and there are no more request lists hence and return NULL.
Current code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;

Quoted 7 lines

Modified code:

        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
		/* There are no more block groups, hence no request lists */
		if (list_empty(ent))
			return NULL;
	}

OK. I changed that.

Below is the updated version of the patch.

======================================================================
blk_put_rl() does not call blkg_put() for q->root_rl because we don't take request list reference on q->root_blkg. However, if root_blkg is once attached then detached (freed), blk_put_rl() is confused by the bogus pointer in q->root_blkg.

For example, with !CONFIG_BLK_DEV_THROTTLING && CONFIG_CFQ_GROUP_IOSCHED, switching IO scheduler from cfq to deadline will cause system stall after the following warning with 3.6:

Quoted 26 lines

WARNING: at /work/build/linux/block/blk-cgroup.h:250 blk_put_rl+0x4d/0x95()
Modules linked in: bridge stp llc sunrpc acpi_cpufreq freq_table mperf ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4
Pid: 0, comm: swapper/0 Not tainted 3.6.0 #1
Call Trace:
 <IRQ>  [<ffffffff810453bd>] warn_slowpath_common+0x85/0x9d
 [<ffffffff810453ef>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff811d5f8d>] blk_put_rl+0x4d/0x95
 [<ffffffff811d614a>] __blk_put_request+0xc3/0xcb
 [<ffffffff811d71a3>] blk_finish_request+0x232/0x23f
 [<ffffffff811d76c3>] ? blk_end_bidi_request+0x34/0x5d
 [<ffffffff811d76d1>] blk_end_bidi_request+0x42/0x5d
 [<ffffffff811d7728>] blk_end_request+0x10/0x12
 [<ffffffff812cdf16>] scsi_io_completion+0x207/0x4d5
 [<ffffffff812c6fcf>] scsi_finish_command+0xfa/0x103
 [<ffffffff812ce2f8>] scsi_softirq_done+0xff/0x108
 [<ffffffff811dcea5>] blk_done_softirq+0x8d/0xa1
 [<ffffffff810915d5>] ? generic_smp_call_function_single_interrupt+0x9f/0xd7
 [<ffffffff8104cf5b>] __do_softirq+0x102/0x213
 [<ffffffff8108a5ec>] ? lock_release_holdtime+0xb6/0xbb
 [<ffffffff8104d2b4>] ? raise_softirq_irqoff+0x9/0x3d
 [<ffffffff81424dfc>] call_softirq+0x1c/0x30
 [<ffffffff81011beb>] do_softirq+0x4b/0xa3
 [<ffffffff8104cdb0>] irq_exit+0x53/0xd5
 [<ffffffff8102d865>] smp_call_function_single_interrupt+0x34/0x36
 [<ffffffff8142486f>] call_function_single_interrupt+0x6f/0x80
 <EOI>  [<ffffffff8101800b>] ? mwait_idle+0x94/0xcd

Quoted 8 lines

 [<ffffffff81018002>] ? mwait_idle+0x8b/0xcd
 [<ffffffff81017811>] cpu_idle+0xbb/0x114
 [<ffffffff81401fbd>] rest_init+0xc1/0xc8
 [<ffffffff81401efc>] ? csum_partial_copy_generic+0x16c/0x16c
 [<ffffffff81cdbd3d>] start_kernel+0x3d4/0x3e1
 [<ffffffff81cdb79e>] ? kernel_init+0x1f7/0x1f7
 [<ffffffff81cdb2dd>] x86_64_start_reservations+0xb8/0xbd
 [<ffffffff81cdb3e3>] x86_64_start_kernel+0x101/0x110

This patch clears q->root_blkg and q->root_rl.blkg when root blkg is destroyed.
__blk_queue_next_rl(), which uses q->root_blkg without check, is changed to exit early when all blkg's are destroyed.

block/blk-cgroup.c +10 -0

index f3b44a6..a31e678 100644
@@ -285,6 +285,13 @@ static void blkg_destroy_all(struct request_queue *q)
 		blkg_destroy(blkg);
 		spin_unlock(&blkcg->lock);
 	}
+
+	/*
+	 * root blkg is destroyed.  Just clear the pointer since
+	 * root_rl does not take reference on root blkg.
+	 */
+	q->root_blkg = NULL;
+	q->root_rl.blkg = NULL;
 }
 
 static void blkg_rcu_free(struct rcu_head *rcu_head)

@@ -326,6 +333,9 @@ struct request_list *__blk_queue_next_rl(struct request_list *rl,
 	 */
 	if (rl == &q->root_rl) {
 		ent = &q->blkg_list;
+		/* There are no more block groups, hence no request lists */
+		if (list_empty(ent))
+			return NULL;
 	} else {
 		blkg = container_of(rl, struct blkcg_gq, rl);
 		ent = &blkg->q_node;

Vivek Goyal

Acked-by

2012-10-11 18:55 UTC #3 lore raw

On Thu, Oct 11, 2012 at 10:31:46AM +0900, Jun'ichi Nomura wrote:

[..]

Quoted 47 lines

Below is the updated version of the patch.

For example, with !CONFIG_BLK_DEV_THROTTLING && CONFIG_CFQ_GROUP_IOSCHED, switching IO scheduler from cfq to deadline will cause system stall after the following warning with 3.6:

Quoted 26 lines

WARNING: at /work/build/linux/block/blk-cgroup.h:250 blk_put_rl+0x4d/0x95()
Modules linked in: bridge stp llc sunrpc acpi_cpufreq freq_table mperf ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4
Pid: 0, comm: swapper/0 Not tainted 3.6.0 #1
Call Trace:
 <IRQ>  [<ffffffff810453bd>] warn_slowpath_common+0x85/0x9d
 [<ffffffff810453ef>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff811d5f8d>] blk_put_rl+0x4d/0x95
 [<ffffffff811d614a>] __blk_put_request+0xc3/0xcb
 [<ffffffff811d71a3>] blk_finish_request+0x232/0x23f
 [<ffffffff811d76c3>] ? blk_end_bidi_request+0x34/0x5d
 [<ffffffff811d76d1>] blk_end_bidi_request+0x42/0x5d
 [<ffffffff811d7728>] blk_end_request+0x10/0x12
 [<ffffffff812cdf16>] scsi_io_completion+0x207/0x4d5
 [<ffffffff812c6fcf>] scsi_finish_command+0xfa/0x103
 [<ffffffff812ce2f8>] scsi_softirq_done+0xff/0x108
 [<ffffffff811dcea5>] blk_done_softirq+0x8d/0xa1
 [<ffffffff810915d5>] ? generic_smp_call_function_single_interrupt+0x9f/0xd7
 [<ffffffff8104cf5b>] __do_softirq+0x102/0x213
 [<ffffffff8108a5ec>] ? lock_release_holdtime+0xb6/0xbb
 [<ffffffff8104d2b4>] ? raise_softirq_irqoff+0x9/0x3d
 [<ffffffff81424dfc>] call_softirq+0x1c/0x30
 [<ffffffff81011beb>] do_softirq+0x4b/0xa3
 [<ffffffff8104cdb0>] irq_exit+0x53/0xd5
 [<ffffffff8102d865>] smp_call_function_single_interrupt+0x34/0x36
 [<ffffffff8142486f>] call_function_single_interrupt+0x6f/0x80
 <EOI>  [<ffffffff8101800b>] ? mwait_idle+0x94/0xcd

Quoted 8 lines

 [<ffffffff81018002>] ? mwait_idle+0x8b/0xcd
 [<ffffffff81017811>] cpu_idle+0xbb/0x114
 [<ffffffff81401fbd>] rest_init+0xc1/0xc8
 [<ffffffff81401efc>] ? csum_partial_copy_generic+0x16c/0x16c
 [<ffffffff81cdbd3d>] start_kernel+0x3d4/0x3e1
 [<ffffffff81cdb79e>] ? kernel_init+0x1f7/0x1f7
 [<ffffffff81cdb2dd>] x86_64_start_reservations+0xb8/0xbd
 [<ffffffff81cdb3e3>] x86_64_start_kernel+0x101/0x110

Quoted 5 lines

This patch clears q->root_blkg and q->root_rl.blkg when root blkg is destroyed.
__blk_queue_next_rl(), which uses q->root_blkg without check, is changed to exit early when all blkg's are destroyed.

Thanks. This patch looks good to me.

Acked-by Vivek Goyal <[email protected]>

Vivek

Quoted 28 lines

block/blk-cgroup.c +10 -0

index f3b44a6..a31e678 100644
@@ -285,6 +285,13 @@ static void blkg_destroy_all(struct request_queue *q)
 		blkg_destroy(blkg);
 		spin_unlock(&blkcg->lock);
 	}
+
+	/*
+	 * root blkg is destroyed.  Just clear the pointer since
+	 * root_rl does not take reference on root blkg.
+	 */
+	q->root_blkg = NULL;
+	q->root_rl.blkg = NULL;
 }
 
 static void blkg_rcu_free(struct rcu_head *rcu_head)

@@ -326,6 +333,9 @@ struct request_list *__blk_queue_next_rl(struct request_list *rl,
 	 */
 	if (rl == &q->root_rl) {
 		ent = &q->blkg_list;
+		/* There are no more block groups, hence no request lists */
+		if (list_empty(ent))
+			return NULL;
 	} else {
 		blkg = container_of(rl, struct blkcg_gq, rl);
 		ent = &blkg->q_node;

Tejun Heo

maintainer

2012-10-16 23:20 UTC #4 lore raw

In reply to Jun'ichi Nomura

Hello,

Quoted 11 lines · Jun'ichi Nomura

Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-	if (ent == &q->root_blkg->q_node)
+	if (q->root_blkg && ent == &q->root_blkg->q_node)
Can we fix it little differently. Little earlier in the code, we check for if q->blkg_list is empty, then all the groups are gone, and there are no more request lists hence and return NULL.
Current code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;

Quoted 7 lines

Modified code:

        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
		/* There are no more block groups, hence no request lists */
		if (list_empty(ent))
			return NULL;
	}

Do we need this at all? q->root_blkg being NULL is completely fine there and the comparison would work as expected, no?

Thanks.

Signature · 1 line

tejun

Jun'ichi Nomura

author

2012-10-17 00:02 UTC #5 lore raw

Quoted 19 lines · Tejun Heo

Quoted 11 lines
Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-	if (ent == &q->root_blkg->q_node)
+	if (q->root_blkg && ent == &q->root_blkg->q_node)
Can we fix it little differently. Little earlier in the code, we check for if q->blkg_list is empty, then all the groups are gone, and there are no more request lists hence and return NULL.
Current code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
Quoted 7 lines
Quoted 7 lines
Modified code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
		/* There are no more block groups, hence no request lists */
		if (list_empty(ent))
			return NULL;
	}

Quoted 2 lines

Do we need this at all? q->root_blkg being NULL is completely fine there and the comparison would work as expected, no?

Hmm?

If list_empty(ent) and q->root_blkg == NULL,

Quoted 2 lines

        /* walk to the next list_head, skip root blkcg */
        ent = ent->next;

ent is &q->blkg_list again.

Quoted 1 line

        if (ent == &q->root_blkg->q_node)

So ent is not &q->root_blkg->q_node.

Quoted 3 lines

                ent = ent->next;
        if (ent == &q->blkg_list)
                return NULL;

And we return NULL here.

Ah, yes. You are correct.
We can do without the above hunk.

Signature · 1 line

Jun'ichi Nomura, NEC Corporation

Jun'ichi Nomura

author

2012-10-17 08:47 UTC #6 lore raw

PATCH 1/1 blkcg: Fix use-after-free of q->root_blkg and q->root_rl.blkg

blk_put_rl() does not call blkg_put() for q->root_rl because we don't take request list reference on q->root_blkg. However, if root_blkg is once attached then detached (freed), blk_put_rl() is confused by the bogus pointer in q->root_blkg.

For example, with !CONFIG_BLK_DEV_THROTTLING && CONFIG_CFQ_GROUP_IOSCHED,
switching IO scheduler from cfq to deadline will cause system stall after the following warning with 3.6:

Quoted 29 lines

WARNING: at /work/build/linux/block/blk-cgroup.h:250
blk_put_rl+0x4d/0x95()
Modules linked in: bridge stp llc sunrpc acpi_cpufreq freq_table mperf
ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4
Pid: 0, comm: swapper/0 Not tainted 3.6.0 #1
Call Trace:
 <IRQ>  [<ffffffff810453bd>] warn_slowpath_common+0x85/0x9d
 [<ffffffff810453ef>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff811d5f8d>] blk_put_rl+0x4d/0x95
 [<ffffffff811d614a>] __blk_put_request+0xc3/0xcb
 [<ffffffff811d71a3>] blk_finish_request+0x232/0x23f
 [<ffffffff811d76c3>] ? blk_end_bidi_request+0x34/0x5d
 [<ffffffff811d76d1>] blk_end_bidi_request+0x42/0x5d
 [<ffffffff811d7728>] blk_end_request+0x10/0x12
 [<ffffffff812cdf16>] scsi_io_completion+0x207/0x4d5
 [<ffffffff812c6fcf>] scsi_finish_command+0xfa/0x103
 [<ffffffff812ce2f8>] scsi_softirq_done+0xff/0x108
 [<ffffffff811dcea5>] blk_done_softirq+0x8d/0xa1
 [<ffffffff810915d5>] ?
 generic_smp_call_function_single_interrupt+0x9f/0xd7
 [<ffffffff8104cf5b>] __do_softirq+0x102/0x213
 [<ffffffff8108a5ec>] ? lock_release_holdtime+0xb6/0xbb
 [<ffffffff8104d2b4>] ? raise_softirq_irqoff+0x9/0x3d
 [<ffffffff81424dfc>] call_softirq+0x1c/0x30
 [<ffffffff81011beb>] do_softirq+0x4b/0xa3
 [<ffffffff8104cdb0>] irq_exit+0x53/0xd5
 [<ffffffff8102d865>] smp_call_function_single_interrupt+0x34/0x36
 [<ffffffff8142486f>] call_function_single_interrupt+0x6f/0x80
 <EOI>  [<ffffffff8101800b>] ? mwait_idle+0x94/0xcd

Quoted 8 lines

 [<ffffffff81018002>] ? mwait_idle+0x8b/0xcd
 [<ffffffff81017811>] cpu_idle+0xbb/0x114
 [<ffffffff81401fbd>] rest_init+0xc1/0xc8
 [<ffffffff81401efc>] ? csum_partial_copy_generic+0x16c/0x16c
 [<ffffffff81cdbd3d>] start_kernel+0x3d4/0x3e1
 [<ffffffff81cdb79e>] ? kernel_init+0x1f7/0x1f7
 [<ffffffff81cdb2dd>] x86_64_start_reservations+0xb8/0xbd
 [<ffffffff81cdb3e3>] x86_64_start_kernel+0x101/0x110

This patch clears q->root_blkg and q->root_rl.blkg when root blkg is destroyed.

Signed-off-by Jun'ichi Nomura <[email protected]>

Acked-by Vivek Goyal <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

---
v3:
Removed a hunk for NULL-check q->root_blkg in __blk_queue_next_rl(). Current code can handle the case without the change.

v2:
Added comments in code based on Vivek's suggestion.

 block/blk-cgroup.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

block/blk-cgroup.c +7 -0

index f3b44a6..54f35d1 100644
@@ -285,6 +285,13 @@ static void blkg_destroy_all(struct request_queue *q)
 		blkg_destroy(blkg);
 		spin_unlock(&blkcg->lock);
 	}
+
+	/*
+	 * root blkg is destroyed.  Just clear the pointer since
+	 * root_rl does not take reference on root blkg.
+	 */
+	q->root_blkg = NULL;
+	q->root_rl.blkg = NULL;
 }
 
 static void blkg_rcu_free(struct rcu_head *rcu_head)

Tejun Heo

maintainer Acked-by on v1 1/1

2012-10-18 21:21 UTC #7 lore raw

Re: [PATCH] blkcg: Fix use-after-free of q->root_blkg and q->root_rl.blkg

Quoted 9 lines · Jun'ichi Nomura

blk_put_rl() does not call blkg_put() for q->root_rl because we don't take request list reference on q->root_blkg. However, if root_blkg is once attached then detached (freed), blk_put_rl() is confused by the bogus pointer in q->root_blkg.
For example, with !CONFIG_BLK_DEV_THROTTLING && CONFIG_CFQ_GROUP_IOSCHED,
switching IO scheduler from cfq to deadline will cause system stall after the following warning with 3.6:

...

Quoted 7 lines

This patch clears q->root_blkg and q->root_rl.blkg when root blkg is destroyed.
Signed-off-by Jun'ichi Nomura <[email protected]>
Acked-by Vivek Goyal <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>

Acked-by Tejun Heo <[email protected]>

Jens, this one needs Cc: stable.

Thanks.

Signature · 1 line

tejun

Jens Axboe

maintainer Applied

2012-10-22 18:43 UTC #8 lore raw

Re: [PATCH] blkcg: Fix use-after-free of q->root_blkg and q->root_rl.blkg

In reply to Jun'ichi Nomura

Quoted 48 lines · Jun'ichi Nomura

For example, with !CONFIG_BLK_DEV_THROTTLING && CONFIG_CFQ_GROUP_IOSCHED,
switching IO scheduler from cfq to deadline will cause system stall after the following warning with 3.6:

Quoted 29 lines

WARNING: at /work/build/linux/block/blk-cgroup.h:250
blk_put_rl+0x4d/0x95()
Modules linked in: bridge stp llc sunrpc acpi_cpufreq freq_table mperf
ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4
Pid: 0, comm: swapper/0 Not tainted 3.6.0 #1
Call Trace:
 <IRQ>  [<ffffffff810453bd>] warn_slowpath_common+0x85/0x9d
 [<ffffffff810453ef>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff811d5f8d>] blk_put_rl+0x4d/0x95
 [<ffffffff811d614a>] __blk_put_request+0xc3/0xcb
 [<ffffffff811d71a3>] blk_finish_request+0x232/0x23f
 [<ffffffff811d76c3>] ? blk_end_bidi_request+0x34/0x5d
 [<ffffffff811d76d1>] blk_end_bidi_request+0x42/0x5d
 [<ffffffff811d7728>] blk_end_request+0x10/0x12
 [<ffffffff812cdf16>] scsi_io_completion+0x207/0x4d5
 [<ffffffff812c6fcf>] scsi_finish_command+0xfa/0x103
 [<ffffffff812ce2f8>] scsi_softirq_done+0xff/0x108
 [<ffffffff811dcea5>] blk_done_softirq+0x8d/0xa1
 [<ffffffff810915d5>] ?
 generic_smp_call_function_single_interrupt+0x9f/0xd7
 [<ffffffff8104cf5b>] __do_softirq+0x102/0x213
 [<ffffffff8108a5ec>] ? lock_release_holdtime+0xb6/0xbb
 [<ffffffff8104d2b4>] ? raise_softirq_irqoff+0x9/0x3d
 [<ffffffff81424dfc>] call_softirq+0x1c/0x30
 [<ffffffff81011beb>] do_softirq+0x4b/0xa3
 [<ffffffff8104cdb0>] irq_exit+0x53/0xd5
 [<ffffffff8102d865>] smp_call_function_single_interrupt+0x34/0x36
 [<ffffffff8142486f>] call_function_single_interrupt+0x6f/0x80
 <EOI>  [<ffffffff8101800b>] ? mwait_idle+0x94/0xcd

Quoted 8 lines

 [<ffffffff81018002>] ? mwait_idle+0x8b/0xcd
 [<ffffffff81017811>] cpu_idle+0xbb/0x114
 [<ffffffff81401fbd>] rest_init+0xc1/0xc8
 [<ffffffff81401efc>] ? csum_partial_copy_generic+0x16c/0x16c
 [<ffffffff81cdbd3d>] start_kernel+0x3d4/0x3e1
 [<ffffffff81cdb79e>] ? kernel_init+0x1f7/0x1f7
 [<ffffffff81cdb2dd>] x86_64_start_reservations+0xb8/0xbd
 [<ffffffff81cdb3e3>] x86_64_start_kernel+0x101/0x110

Quoted 2 lines

This patch clears q->root_blkg and q->root_rl.blkg when root blkg is destroyed.

Thanks, applied to for-linus (and marked stable).

Signature · 1 line

Jens Axboe

Vivek Goyal

2012-10-17 13:47 UTC #9 lore raw

In reply to Jun'ichi Nomura

Quoted 27 lines · Jun'ichi Nomura

Quoted 19 lines · Tejun Heo
Quoted 11 lines
Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-	if (ent == &q->root_blkg->q_node)
+	if (q->root_blkg && ent == &q->root_blkg->q_node)
Can we fix it little differently. Little earlier in the code, we check for if q->blkg_list is empty, then all the groups are gone, and there are no more request lists hence and return NULL.
Current code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
Quoted 7 lines
Quoted 7 lines
Modified code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
		/* There are no more block groups, hence no request lists */
		if (list_empty(ent))
			return NULL;
	}
Quoted 2 lines
Do we need this at all? q->root_blkg being NULL is completely fine there and the comparison would work as expected, no?
Hmm?
If list_empty(ent) and q->root_blkg == NULL,

Quoted 8 lines

Quoted 2 lines
        /* walk to the next list_head, skip root blkcg */
        ent = ent->next;
ent is &q->blkg_list again.
Quoted 1 line
        if (ent == &q->root_blkg->q_node)
So ent is not &q->root_blkg->q_node.

If q->root_blkg is NULL, will it not lead to NULL pointer dereference. (q->root_blkg->q_node).

Quoted 9 lines

Quoted 4 lines
                ent = ent->next;
        if (ent == &q->blkg_list)
                return NULL;
And we return NULL here.
Ah, yes. You are correct.
We can do without the above hunk.

I would rather prefer to check for this boundary condition early and return instead of letting it fall through all these conditions and then figure out yes we have no next rl. IMO, code becomes easier to understand if nothing else. Otherwise one needs a step by step explanation as above to show that case of q->root_blkg is covered.

Thanks
Vivek

Jun'ichi Nomura

author

2012-10-18 02:57 UTC #10 lore raw

Quoted 31 lines · Vivek Goyal

Quoted 30 lines · Jun'ichi Nomura
Quoted 19 lines · Tejun Heo
Quoted 11 lines
Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-	if (ent == &q->root_blkg->q_node)
+	if (q->root_blkg && ent == &q->root_blkg->q_node)
Can we fix it little differently. Little earlier in the code, we check for if q->blkg_list is empty, then all the groups are gone, and there are no more request lists hence and return NULL.
Current code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
Quoted 7 lines
Quoted 7 lines
Modified code:
        if (rl == &q->root_rl) {
                ent = &q->blkg_list;
		/* There are no more block groups, hence no request lists */
		if (list_empty(ent))
			return NULL;
	}
Quoted 2 lines
Do we need this at all? q->root_blkg being NULL is completely fine there and the comparison would work as expected, no?
Hmm?
If list_empty(ent) and q->root_blkg == NULL,
Quoted 2 lines
        /* walk to the next list_head, skip root blkcg */
        ent = ent->next;

Quoted 8 lines

Quoted 5 lines
ent is &q->blkg_list again.
Quoted 1 line
        if (ent == &q->root_blkg->q_node)
So ent is not &q->root_blkg->q_node.
If q->root_blkg is NULL, will it not lead to NULL pointer dereference. (q->root_blkg->q_node).

It's not dereferenced.

Quoted 6 lines

Quoted 3 lines

                ent = ent->next;
        if (ent == &q->blkg_list)
                return NULL;

And we return NULL here.

Quoted 8 lines

Quoted 2 lines
Ah, yes. You are correct.
We can do without the above hunk.
I would rather prefer to check for this boundary condition early and return instead of letting it fall through all these conditions and then figure out yes we have no next rl. IMO, code becomes easier to understand if nothing else. Otherwise one needs a step by step explanation as above to show that case of q->root_blkg is covered.

I have same opinion as yours that it's good for readability.

Signature · 1 line

Jun'ichi Nomura, NEC Corporation

Vivek Goyal

2012-10-18 13:31 UTC #11 lore raw

On Thu, Oct 18, 2012 at 11:56:34AM +0900, Jun'ichi Nomura wrote:

[..]

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
        if (ent == &q->root_blkg->q_node)
So ent is not &q->root_blkg->q_node.
If q->root_blkg is NULL, will it not lead to NULL pointer dereference. (q->root_blkg->q_node).
It's not dereferenced.

Ok. We are taking address of root_blkg->q_node so even if root_blkg=NULL, address is just offset from null. Little subtle for me. :-)

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
                ent = ent->next;
        if (ent == &q->blkg_list)
                return NULL;
And we return NULL here.
Ah, yes. You are correct.
We can do without the above hunk.

Quoted 7 lines

Quoted 5 lines
I would rather prefer to check for this boundary condition early and return instead of letting it fall through all these conditions and then figure out yes we have no next rl. IMO, code becomes easier to understand if nothing else. Otherwise one needs a step by step explanation as above to show that case of q->root_blkg is covered.
I have same opinion as yours that it's good for readability.

Tejun, for the sake of readability, are you fine with keeping the original check and original patch which I had acked.

Thanks
Vivek

Tejun Heo

maintainer

2012-10-18 21:20 UTC #12 lore raw

Hey, Vivek.

Quoted 2 lines · Vivek Goyal

Tejun, for the sake of readability, are you fine with keeping the original check and original patch which I had acked.

Can you please send another patch to change that? It really isn't a related change and I don't wanna mix the two.

Thanks.

Signature · 1 line

tejun

Vivek Goyal

2012-10-19 14:54 UTC #13 lore raw

Quoted 8 lines · Tejun Heo

Hey, Vivek.
Quoted 2 lines · Vivek Goyal
Tejun, for the sake of readability, are you fine with keeping the original check and original patch which I had acked.
Can you please send another patch to change that? It really isn't a related change and I don't wanna mix the two.

Sure. Jun'ichi, would you like to send that cleanup line in a separate patch?

Thanks
Vivek

Jun'ichi Nomura

author

2012-10-22 00:58 UTC #14 lore raw

Quoted 11 lines · Vivek Goyal

Quoted 8 lines · Tejun Heo
Hey, Vivek.
Quoted 2 lines · Vivek Goyal
Tejun, for the sake of readability, are you fine with keeping the original check and original patch which I had acked.
Can you please send another patch to change that? It really isn't a related change and I don't wanna mix the two.
Sure. Jun'ichi, would you like to send that cleanup line in a separate patch?

OK. I will send that patch.

Signature · 1 line

Jun'ichi Nomura, NEC Corporation

Jun'ichi Nomura

author

2012-10-22 01:16 UTC #15 lore raw

PATCH 1/1 blkcg: stop iteration early if root_rl is the only request list

In reply to Tejun Heo

__blk_queue_next_rl() finds next request list based on blkg_list while skipping root_blkg in the list.
OTOH, root_rl is special as it may exist even without root_blkg.

Though the later part of the function handles such a case correctly, exiting early is good for readability of the code.

Signed-off-by Jun'ichi Nomura <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-cgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

block/blk-cgroup.c +3 -0

index 54f35d1..a31e678 100644
@@ -333,6 +333,9 @@ struct request_list *__blk_queue_next_rl(struct request_list *rl,
 	 */
 	if (rl == &q->root_rl) {
 		ent = &q->blkg_list;
+		/* There are no more block groups, hence no request lists */
+		if (list_empty(ent))
+			return NULL;
 	} else {
 		blkg = container_of(rl, struct blkcg_gq, rl);
 		ent = &blkg->q_node;

Vivek Goyal

Acked-by on v1 1/1

2012-10-22 15:31 UTC #16 lore raw

Re: [PATCH] blkcg: stop iteration early if root_rl is the only request list

Quoted 7 lines · Jun'ichi Nomura

__blk_queue_next_rl() finds next request list based on blkg_list while skipping root_blkg in the list.
OTOH, root_rl is special as it may exist even without root_blkg.
Though the later part of the function handles such a case correctly, exiting early is good for readability of the code.

Thanks for the patch Junichi. This one looks good to me.

Acked-by Vivek Goyal <[email protected]>

Vivek

Quoted 22 lines

Signed-off-by Jun'ichi Nomura <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-cgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

block/blk-cgroup.c +3 -0

index 54f35d1..a31e678 100644
@@ -333,6 +333,9 @@ struct request_list *__blk_queue_next_rl(struct request_list *rl,
 	 */
 	if (rl == &q->root_rl) {
 		ent = &q->blkg_list;
+		/* There are no more block groups, hence no request lists */
+		if (list_empty(ent))
+			return NULL;
 	} else {
 		blkg = container_of(rl, struct blkcg_gq, rl);
 		ent = &blkg->q_node;

Jens Axboe

maintainer Applied

2012-10-22 18:43 UTC #17 lore raw

Re: [PATCH] blkcg: stop iteration early if root_rl is the only request list

In reply to Jun'ichi Nomura

Quoted 6 lines · Jun'ichi Nomura

__blk_queue_next_rl() finds next request list based on blkg_list while skipping root_blkg in the list.
OTOH, root_rl is special as it may exist even without root_blkg.
Though the later part of the function handles such a case correctly, exiting early is good for readability of the code.

Applied to for-linus, thanks.

Signature · 1 line

Jens Axboe