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

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

[Jun'ichi Nomura]

I got system stall after the following warning with 3.6:

> 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
>  [<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 <j-nomura@ce.jp.nec.com>

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index f3b44a6..5015764 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -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;

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

[Vivek Goyal]

On Wed, Oct 10, 2012 at 02:11:03PM +0900, Jun'ichi Nomura wrote:
> I got system stall after the following warning with 3.6:
> 
> > 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
> >  [<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.

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.

> 
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index f3b44a6..5015764 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -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.

>  }
>  
>  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

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

[Jun'ichi Nomura]

Hi Vivek, thank you for comments.

On 10/11/12 00:59, Vivek Goyal wrote:
> 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.

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

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

> 
> 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.
> 
>>
>> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index f3b44a6..5015764 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -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.
       */

> 
>>  }
>>  
>>  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;
> 	}

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:

> 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
>  [<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.

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index f3b44a6..a31e678 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -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;

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

[Vivek Goyal]

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

[..]
> 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:
> 
> > 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
> >  [<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.
> 

Thanks. This patch looks good to me.

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index f3b44a6..a31e678 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -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;

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

[Tejun Heo]

Hello,

On Thu, Oct 11, 2012 at 10:31:46AM +0900, Jun'ichi Nomura wrote:
> >> -	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;
> > 	}

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

Thanks.

-- 
tejun

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

[Jun'ichi Nomura]

On 10/17/12 08:20, Tejun Heo wrote:
>>>> -	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;
>>> 	}
> 
> 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,

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

ent is &q->blkg_list again.

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

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

>                 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.

-- 
Jun'ichi Nomura, NEC Corporation

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

[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:

> 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
>  [<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 <j-nomura@ce.jp.nec.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
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(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index f3b44a6..54f35d1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -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)

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

[Vivek Goyal]

On Wed, Oct 17, 2012 at 09:02:22AM +0900, Jun'ichi Nomura wrote:
> On 10/17/12 08:20, Tejun Heo wrote:
> >>>> -	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;
> >>> 	}
> > 
> > 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,
> 
> >         /* walk to the next list_head, skip root blkcg */
> >         ent = ent->next;
> 
> ent is &q->blkg_list again.
> 
> >         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).
 
> 
> >                 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

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

[Jun'ichi Nomura]

On 10/17/12 22:47, Vivek Goyal wrote:
> On Wed, Oct 17, 2012 at 09:02:22AM +0900, Jun'ichi Nomura wrote:
>> On 10/17/12 08:20, Tejun Heo wrote:
>>>>>> -	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;
>>>>> 	}
>>>
>>> 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,
>>
>>>         /* walk to the next list_head, skip root blkcg */
>>>         ent = ent->next;
>>
>> ent is &q->blkg_list again.
>>
>>>         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.

>>>                 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.

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

-- 
Jun'ichi Nomura, NEC Corporation

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

[Vivek Goyal]

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

[..]
> >>>         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. :-)

> 
> >>>                 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.
> 
> 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

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

[Tejun Heo]

Hey, Vivek.

On Thu, Oct 18, 2012 at 09:31:49AM -0400, Vivek Goyal wrote:
> 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.

-- 
tejun

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

[Tejun Heo]

On Wed, Oct 17, 2012 at 05:45:36PM +0900, Jun'ichi Nomura wrote:
> 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:
...
> This patch clears q->root_blkg and q->root_rl.blkg when root blkg
> is destroyed.
> 
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>

Acked-by: Tejun Heo <tj@kernel.org>

Jens, this one needs Cc: stable.

Thanks.

-- 
tejun

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

[Vivek Goyal]

On Thu, Oct 18, 2012 at 02:20:53PM -0700, Tejun Heo wrote:
> Hey, Vivek.
> 
> On Thu, Oct 18, 2012 at 09:31:49AM -0400, Vivek Goyal wrote:
> > 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

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

[Jun'ichi Nomura]

On 10/19/12 23:53, Vivek Goyal wrote:
> On Thu, Oct 18, 2012 at 02:20:53PM -0700, Tejun Heo wrote:
>> Hey, Vivek.
>>
>> On Thu, Oct 18, 2012 at 09:31:49AM -0400, Vivek Goyal wrote:
>>> 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.

-- 
Jun'ichi Nomura, NEC Corporation

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

[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.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-cgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 54f35d1..a31e678 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -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;

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

[Vivek Goyal]

On Mon, Oct 22, 2012 at 10:15:37AM +0900, Jun'ichi Nomura wrote:
> __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 <vgoyal@redhat.com>

Vivek

> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
>  block/blk-cgroup.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 54f35d1..a31e678 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -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;

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

[Jens Axboe]

On 2012-10-22 03:15, Jun'ichi Nomura wrote:
> __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.

-- 
Jens Axboe

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

[Jens Axboe]

On 2012-10-17 10:45, Jun'ichi Nomura wrote:
> 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:
> 
>> 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
>>  [<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.

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

-- 
Jens Axboe