From: Waiman Long <longman@redhat.com>
To: Jens Axboe <axboe@kernel.dk>, Tejun Heo <tj@kernel.org>,
Josef Bacik <josef@toxicpanda.com>,
Zefan Li <lizefan.x@bytedance.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
"Michal Koutný" <mkoutny@suse.com>,
"Dennis Zhou (Facebook)" <dennisszhou@gmail.com>,
"Waiman Long" <longman@redhat.com>,
"Yi Zhang" <yi.zhang@redhat.com>
Subject: [PATCH v4 1/2] bdi, blk-cgroup: Fix potential UAF of blkcg
Date: Wed, 14 Dec 2022 22:31:31 -0500 [thread overview]
Message-ID: <20221215033132.230023-2-longman@redhat.com> (raw)
In-Reply-To: <20221215033132.230023-1-longman@redhat.com>
Commit 59b57717fff8 ("blkcg: delay blkg destruction until after
writeback has finished") delayed call to blkcg_destroy_blkgs() to
cgwb_release_workfn(). However, it is done after a css_put() of blkcg
which may be the final put that causes the blkcg to be freed as RCU
read lock isn't held.
Another place where blkcg_destroy_blkgs() can be called indirectly via
blkcg_unpin_online() is from the offline_css() function called from
css_killed_work_fn(). Over there, the potentially final css_put() call
is issued after offline_css().
By adding a css_tryget() into blkcg_destroy_blkgs() and warning its
failure, the following stack trace was produced in a test system on
bootup.
[ 34.254240] RIP: 0010:blkcg_destroy_blkgs+0x16a/0x1a0
:
[ 34.339943] Call Trace:
[ 34.344510] blkcg_unpin_online+0x38/0x60
[ 34.348523] cgwb_release_workfn+0x6a/0x200
[ 34.352708] process_one_work+0x1e5/0x3b0
[ 34.360758] worker_thread+0x50/0x3a0
[ 34.368447] kthread+0xd9/0x100
[ 34.376386] ret_from_fork+0x22/0x30
This confirms that a potential UAF situation can really happen in
cgwb_release_workfn().
Fix that by delaying the css_put() until after the blkcg_unpin_online()
call. Also use percpu_ref_is_zero() in blkcg_destroy_blkgs() and issue
a warning if reference count is zero.
The reproducing system can no longer produce a warning with this patch.
All the runnable block/0* tests including block/027 were run successfully
without failure.
Fixes: 59b57717fff8 ("blkcg: delay blkg destruction until after writeback has finished")
Suggested-by: Michal Koutný <mkoutny@suse.com>
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
block/blk-cgroup.c | 7 +++++++
mm/backing-dev.c | 8 ++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 1bb939d3b793..ca28306aa1b1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1084,6 +1084,13 @@ struct list_head *blkcg_get_cgwb_list(struct cgroup_subsys_state *css)
*/
static void blkcg_destroy_blkgs(struct blkcg *blkcg)
{
+ /*
+ * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
+ * references gone.
+ */
+ if (WARN_ON_ONCE(percpu_ref_is_zero(&blkcg->css.refcnt)))
+ return;
+
might_sleep();
spin_lock_irq(&blkcg->lock);
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c30419a5e119..36f75b072325 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -390,11 +390,15 @@ static void cgwb_release_workfn(struct work_struct *work)
wb_shutdown(wb);
css_put(wb->memcg_css);
- css_put(wb->blkcg_css);
mutex_unlock(&wb->bdi->cgwb_release_mutex);
- /* triggers blkg destruction if no online users left */
+ /*
+ * Triggers blkg destruction if no online users left
+ * The final blkcg css_put() has to be done after blkcg_unpin_online()
+ * to avoid use-after-free.
+ */
blkcg_unpin_online(wb->blkcg_css);
+ css_put(wb->blkcg_css);
fprop_local_destroy_percpu(&wb->memcg_completions);
--
2.31.1
next prev parent reply other threads:[~2022-12-15 3:32 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-15 3:31 [PATCH v4 0/2] blk-cgroup: Fix potential UAF & flush rstat at blkgs destruction path Waiman Long
2022-12-15 3:31 ` Waiman Long [this message]
2022-12-15 3:31 ` [PATCH v4 2/2] blk-cgroup: Flush stats " Waiman Long
2023-02-02 4:15 ` Ming Lei
2023-02-02 22:35 ` Tejun Heo
2023-02-02 3:26 ` [PATCH v4 0/2] blk-cgroup: Fix potential UAF & flush rstat " Waiman Long
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221215033132.230023-2-longman@redhat.com \
--to=longman@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=axboe@kernel.dk \
--cc=cgroups@vger.kernel.org \
--cc=dennisszhou@gmail.com \
--cc=hannes@cmpxchg.org \
--cc=josef@toxicpanda.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lizefan.x@bytedance.com \
--cc=mkoutny@suse.com \
--cc=tj@kernel.org \
--cc=yi.zhang@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).