| Message ID | 20221213184446.50181-2-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-cgroup: Fix potential UAF & flush rstat at blkgs destruction path | expand |
On Tue, Dec 13, 2022 at 01:44:45PM -0500, Waiman Long wrote: > 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. This doesn't agree with the code anymore. Otherwise Acked-by: Tejun Heo <tj@kernel.org> Thanks.
On 12/13/22 14:29, Tejun Heo wrote: > On Tue, Dec 13, 2022 at 01:44:45PM -0500, Waiman Long wrote: >> 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. > This doesn't agree with the code anymore. Otherwise > > Acked-by: Tejun Heo <tj@kernel.org> Sorry, I overlooked the commit log in my update. I will update it if I need another version, or Jens can make the following edit: css_tryget() -> percpu_ref_is_zero(). Thanks, Longman
On 12/13/22 12:53 PM, Waiman Long wrote: > > On 12/13/22 14:29, Tejun Heo wrote: >> On Tue, Dec 13, 2022 at 01:44:45PM -0500, Waiman Long wrote: >>> 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. >> This doesn't agree with the code anymore. Otherwise >> >> Acked-by: Tejun Heo <tj@kernel.org> > > Sorry, I overlooked the commit log in my update. I will update it if I need another version, or Jens can make the following edit: > > css_tryget() -> percpu_ref_is_zero(). Since the other one also needs an edit, would be great if you could just send out a v4.
On 12/14/22 11:54, Jens Axboe wrote: > On 12/13/22 12:53 PM, Waiman Long wrote: >> On 12/13/22 14:29, Tejun Heo wrote: >>> On Tue, Dec 13, 2022 at 01:44:45PM -0500, Waiman Long wrote: >>>> 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. >>> This doesn't agree with the code anymore. Otherwise >>> >>> Acked-by: Tejun Heo <tj@kernel.org> >> Sorry, I overlooked the commit log in my update. I will update it if I need another version, or Jens can make the following edit: >> >> css_tryget() -> percpu_ref_is_zero(). > Since the other one also needs an edit, would be great if you could > just send out a v4. > Sure, will do that. Cheers, Longman
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);
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@suse.com> Reported-by: Yi Zhang <yi.zhang@redhat.com> Signed-off-by: Waiman Long <longman@redhat.com> --- block/blk-cgroup.c | 7 +++++++ mm/backing-dev.c | 8 ++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)