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@xxxxxxxx>
Reported-by: Yi Zhang <yi.zhang@xxxxxxxxxx>
Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
---
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
