From patchwork Mon Mar 5 15:03:16 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joseph Qi X-Patchwork-Id: 10259211 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B9E8760134 for ; Mon, 5 Mar 2018 15:03:26 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A7D41289BC for ; Mon, 5 Mar 2018 15:03:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9C77728ABD; Mon, 5 Mar 2018 15:03:26 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C9D5428ABC for ; Mon, 5 Mar 2018 15:03:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751634AbeCEPDW (ORCPT ); Mon, 5 Mar 2018 10:03:22 -0500 Received: from out30-130.freemail.mail.aliyun.com ([115.124.30.130]:41128 "EHLO out30-130.freemail.mail.aliyun.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751294AbeCEPDW (ORCPT ); Mon, 5 Mar 2018 10:03:22 -0500 X-Alimail-AntiSpam: AC=PASS; BC=-1|-1; BR=01201311R181e4; CH=green; FP=0|-1|-1|-1|0|-1|-1|-1; HT=e01f04452; MF=joseph.qi@linux.alibaba.com; NM=1; PH=DS; RN=6; SR=0; TI=SMTPD_---0SyyDCH7_1520262196; Received: from JosephdeMacBook-Pro.local(mailfrom:joseph.qi@linux.alibaba.com fp:42.120.74.168) by smtp.aliyun-inc.com(127.0.0.1); Mon, 05 Mar 2018 23:03:16 +0800 From: Joseph Qi Subject: [PATCH v3] blk-throttle: fix race between blkcg_bio_issue_check() and cgroup_rmdir() To: Tejun Heo , Jens Axboe Cc: xuejiufei , Caspar Zhang , linux-block , cgroups@vger.kernel.org Message-ID: Date: Mon, 5 Mar 2018 23:03:16 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 Content-Language: en-US Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP We've triggered a WARNING in blk_throtl_bio() when throttling writeback io, which complains blkg->refcnt is already 0 when calling blkg_get(), and then kernel crashes with invalid page request. After investigating this issue, we've found it is caused by a race between blkcg_bio_issue_check() and cgroup_rmdir(), which is described below: writeback kworker cgroup_rmdir cgroup_destroy_locked kill_css css_killed_ref_fn css_killed_work_fn offline_css blkcg_css_offline blkcg_bio_issue_check rcu_read_lock blkg_lookup spin_trylock(q->queue_lock) blkg_destroy spin_unlock(q->queue_lock) blk_throtl_bio spin_lock_irq(q->queue_lock) ... spin_unlock_irq(q->queue_lock) rcu_read_unlock Since rcu can only prevent blkg from releasing when it is being used, the blkg->refcnt can be decreased to 0 during blkg_destroy() and schedule blkg release. Then trying to blkg_get() in blk_throtl_bio() will complains the WARNING. And then the corresponding blkg_put() will schedule blkg release again, which result in double free. This race is introduced by commit ae1188963611 ("blkcg: consolidate blkg creation in blkcg_bio_issue_check()"). Before this commit, it will lookup first and then try to lookup/create again with queue_lock. Since revive this logic is a bit drastic, so fix it by only offlining pd during blkcg_css_offline(), and move the rest destruction (especially blkg_put()) into blkcg_css_free(), which should be the right way as discussed. Fixes: ae1188963611 ("blkcg: consolidate blkg creation in blkcg_bio_issue_check()") Reported-by: Jiufei Xue Cc: stable@vger.kernel.org #4.3+ Signed-off-by: Joseph Qi --- block/blk-cgroup.c | 52 +++++++++++++++++++++++++++++++++++++--------- include/linux/blk-cgroup.h | 2 ++ 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index c2033a2..2e9f510 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -307,11 +307,27 @@ struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg, } } +static void blkg_pd_offline(struct blkcg_gq *blkg) +{ + int i; + + lockdep_assert_held(blkg->q->queue_lock); + lockdep_assert_held(&blkg->blkcg->lock); + + for (i = 0; i < BLKCG_MAX_POLS; i++) { + struct blkcg_policy *pol = blkcg_policy[i]; + + if (blkg->pd[i] && !blkg->pd_offline[i] && pol->pd_offline_fn) { + pol->pd_offline_fn(blkg->pd[i]); + blkg->pd_offline[i] = true; + } + } +} + static void blkg_destroy(struct blkcg_gq *blkg) { struct blkcg *blkcg = blkg->blkcg; struct blkcg_gq *parent = blkg->parent; - int i; lockdep_assert_held(blkg->q->queue_lock); lockdep_assert_held(&blkcg->lock); @@ -320,13 +336,6 @@ static void blkg_destroy(struct blkcg_gq *blkg) WARN_ON_ONCE(list_empty(&blkg->q_node)); WARN_ON_ONCE(hlist_unhashed(&blkg->blkcg_node)); - for (i = 0; i < BLKCG_MAX_POLS; i++) { - struct blkcg_policy *pol = blkcg_policy[i]; - - if (blkg->pd[i] && pol->pd_offline_fn) - pol->pd_offline_fn(blkg->pd[i]); - } - if (parent) { blkg_rwstat_add_aux(&parent->stat_bytes, &blkg->stat_bytes); blkg_rwstat_add_aux(&parent->stat_ios, &blkg->stat_ios); @@ -369,6 +378,7 @@ static void blkg_destroy_all(struct request_queue *q) struct blkcg *blkcg = blkg->blkcg; spin_lock(&blkcg->lock); + blkg_pd_offline(blkg); blkg_destroy(blkg); spin_unlock(&blkcg->lock); } @@ -1013,7 +1023,7 @@ static void blkcg_css_offline(struct cgroup_subsys_state *css) struct request_queue *q = blkg->q; if (spin_trylock(q->queue_lock)) { - blkg_destroy(blkg); + blkg_pd_offline(blkg); spin_unlock(q->queue_lock); } else { spin_unlock_irq(&blkcg->lock); @@ -1032,6 +1042,26 @@ static void blkcg_css_free(struct cgroup_subsys_state *css) struct blkcg *blkcg = css_to_blkcg(css); int i; + spin_lock_irq(&blkcg->lock); + + while (!hlist_empty(&blkcg->blkg_list)) { + struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first, + struct blkcg_gq, + blkcg_node); + struct request_queue *q = blkg->q; + + if (spin_trylock(q->queue_lock)) { + blkg_destroy(blkg); + spin_unlock(q->queue_lock); + } else { + spin_unlock_irq(&blkcg->lock); + cpu_relax(); + spin_lock_irq(&blkcg->lock); + } + } + + spin_unlock_irq(&blkcg->lock); + mutex_lock(&blkcg_pol_mutex); list_del(&blkcg->all_blkcgs_node); @@ -1371,8 +1401,10 @@ void blkcg_deactivate_policy(struct request_queue *q, spin_lock(&blkg->blkcg->lock); if (blkg->pd[pol->plid]) { - if (pol->pd_offline_fn) + if (!blkg->pd_offline[pol->plid] && pol->pd_offline_fn) { pol->pd_offline_fn(blkg->pd[pol->plid]); + blkg->pd_offline[pol->plid] = true; + } pol->pd_free_fn(blkg->pd[pol->plid]); blkg->pd[pol->plid] = NULL; } diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h index 69bea82..2bf8f47 100644 --- a/include/linux/blk-cgroup.h +++ b/include/linux/blk-cgroup.h @@ -133,6 +133,8 @@ struct blkcg_gq { struct blkg_rwstat stat_ios; struct blkg_policy_data *pd[BLKCG_MAX_POLS]; + /* is the corresponding policy data offline? */ + bool pd_offline[BLKCG_MAX_POLS]; struct rcu_head rcu_head; };