Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753763Ab2JJP7e (ORCPT ); Wed, 10 Oct 2012 11:59:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49092 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752059Ab2JJP7d (ORCPT ); Wed, 10 Oct 2012 11:59:33 -0400 Date: Wed, 10 Oct 2012 11:59:29 -0400 From: Vivek Goyal To: "Jun'ichi Nomura" Cc: Tejun Heo , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] Fix use-after-free of q->root_blkg and q->root_rl.blkg Message-ID: <20121010155929.GA18733@redhat.com> References: <50750367.2070508@ce.jp.nec.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50750367.2070508@ce.jp.nec.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5350 Lines: 128 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: > > [] warn_slowpath_common+0x85/0x9d > > [] warn_slowpath_null+0x1a/0x1c > > [] blk_put_rl+0x4d/0x95 > > [] __blk_put_request+0xc3/0xcb > > [] blk_finish_request+0x232/0x23f > > [] ? blk_end_bidi_request+0x34/0x5d > > [] blk_end_bidi_request+0x42/0x5d > > [] blk_end_request+0x10/0x12 > > [] scsi_io_completion+0x207/0x4d5 > > [] scsi_finish_command+0xfa/0x103 > > [] scsi_softirq_done+0xff/0x108 > > [] blk_done_softirq+0x8d/0xa1 > > [] ? generic_smp_call_function_single_interrupt+0x9f/0xd7 > > [] __do_softirq+0x102/0x213 > > [] ? lock_release_holdtime+0xb6/0xbb > > [] ? raise_softirq_irqoff+0x9/0x3d > > [] call_softirq+0x1c/0x30 > > [] do_softirq+0x4b/0xa3 > > [] irq_exit+0x53/0xd5 > > [] smp_call_function_single_interrupt+0x34/0x36 > > [] call_function_single_interrupt+0x6f/0x80 > > [] ? mwait_idle+0x94/0xcd > > [] ? mwait_idle+0x8b/0xcd > > [] cpu_idle+0xbb/0x114 > > [] rest_init+0xc1/0xc8 > > [] ? csum_partial_copy_generic+0x16c/0x16c > > [] start_kernel+0x3d4/0x3e1 > > [] ? kernel_init+0x1f7/0x1f7 > > [] x86_64_start_reservations+0xb8/0xbd > > [] 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 > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/