[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20221208220141.2625775-2-longman@redhat.com>
Date: Thu, 8 Dec 2022 17:01:39 -0500
From: Waiman Long <longman@...hat.com>
To: Jens Axboe <axboe@...nel.dk>, Tejun Heo <tj@...nel.org>,
Josef Bacik <josef@...icpanda.com>,
Zefan Li <lizefan.x@...edance.com>,
Johannes Weiner <hannes@...xchg.org>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: cgroups@...r.kernel.org, linux-block@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Michal Koutný <mkoutny@...e.com>,
"Dennis Zhou (Facebook)" <dennisszhou@...il.com>,
Waiman Long <longman@...hat.com>,
Yi Zhang <yi.zhang@...hat.com>
Subject: [PATCH-block 1/3] bdi, blk-cgroup: Fix potential UAF of blkcg
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 css_tryget() in blkcg_destroy_blkgs() and issue a warning
if css_tryget() fails.
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@...e.com>
Reported-by: Yi Zhang <yi.zhang@...hat.com>
Signed-off-by: Waiman Long <longman@...hat.com>
---
block/blk-cgroup.c | 8 ++++++++
mm/backing-dev.c | 8 ++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 1bb939d3b793..21cc88349f21 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(!css_tryget(&blkcg->css)))
+ return;
+
might_sleep();
spin_lock_irq(&blkcg->lock);
@@ -1110,6 +1117,7 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
}
spin_unlock_irq(&blkcg->lock);
+ css_put(&blkcg->css);
}
/**
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
Powered by blists - more mailing lists