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.
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.342395] <TASK>
[ 34.344510] blkcg_unpin_online+0x38/0x60
[ 34.348523] cgwb_release_workfn+0x6a/0x200
[ 34.352708] process_one_work+0x1e5/0x3b0
[ 34.356742] ? rescuer_thread+0x390/0x390
[ 34.360758] worker_thread+0x50/0x3a0
[ 34.364425] ? rescuer_thread+0x390/0x390
[ 34.368447] kthread+0xd9/0x100
[ 34.371592] ? kthread_complete_and_exit+0x20/0x20
[ 34.376386] ret_from_fork+0x22/0x30
[ 34.379982] </TASK>
This confirms that a potential UAF situation can happen.
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 with no RCU read lock held.
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ý <[email protected]>
Reported-by: Yi Zhang <[email protected]>
Signed-off-by: Waiman Long <[email protected]>
---
block/blk-cgroup.c | 10 +++++++++-
mm/backing-dev.c | 8 ++++++--
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 57941d2a8ba3..904372bb96f1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
might_sleep();
- css_get(&blkcg->css);
+ /*
+ * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
+ * references gone and rcu_read_lock not held.
+ */
+ if (!css_tryget(&blkcg->css)) {
+ WARN_ON_ONCE(!rcu_read_lock_held());
+ return;
+ }
+
spin_lock_irq(&blkcg->lock);
while (!hlist_empty(&blkcg->blkg_list)) {
struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
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
On Tue, Nov 29, 2022 at 03:34:00PM -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.
>
> 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.342395] <TASK>
> [ 34.344510] blkcg_unpin_online+0x38/0x60
> [ 34.348523] cgwb_release_workfn+0x6a/0x200
> [ 34.352708] process_one_work+0x1e5/0x3b0
> [ 34.356742] ? rescuer_thread+0x390/0x390
> [ 34.360758] worker_thread+0x50/0x3a0
> [ 34.364425] ? rescuer_thread+0x390/0x390
> [ 34.368447] kthread+0xd9/0x100
> [ 34.371592] ? kthread_complete_and_exit+0x20/0x20
> [ 34.376386] ret_from_fork+0x22/0x30
> [ 34.379982] </TASK>
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
> This confirms that a potential UAF situation can happen.
>
> 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 with no RCU read lock held.
>
> 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.
--
With Best Regards,
Andy Shevchenko
On 11/30/22 8:16?AM, Michal Koutn? wrote:
> On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <[email protected]> wrote:
>> 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.
>
> Thanks for the test!
>
>> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>
>> might_sleep();
>>
>> - css_get(&blkcg->css);
>> + /*
>> + * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
>> + * references gone and rcu_read_lock not held.
>> + */
>> + if (!css_tryget(&blkcg->css)) {
>> + WARN_ON_ONCE(!rcu_read_lock_held());
>> + return;
>> + }
>
> As I followed the previous discussion, the principle is that obtaining a
> reference or being inside an RCU read section is sufficient.
>
> Consequently, I'd expect the two situations handled equally but here the
> no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)
>
> However, the might_sleep() in (non-sleepable) RCU reader section combo
> makes me wary anyway (not with the early return but tools would likely
> complain).
>
> All in all, can't the contract of blkcg_destroy_blkgs() declare that
> a caller must pass blkcg with a valid reference? (The body of
> blkcg_destroy_blkgs then wouldn't need to get neither put the inner
> reference).
Totally agree, the proposed patch feels more like a hacky workaround
rather than a true solution. Either the contract should be that it's
ALWAYS entered with RCU lock held and hence the tryget is fine, OR that
a reference always is held when entered.
I'm going to revert the offending patch for now, and then we can queue
up a proper patch when that exists.
--
Jens Axboe
On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <[email protected]> wrote:
> 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.
Thanks for the test!
> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>
> might_sleep();
>
> - css_get(&blkcg->css);
> + /*
> + * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
> + * references gone and rcu_read_lock not held.
> + */
> + if (!css_tryget(&blkcg->css)) {
> + WARN_ON_ONCE(!rcu_read_lock_held());
> + return;
> + }
As I followed the previous discussion, the principle is that obtaining a
reference or being inside an RCU read section is sufficient.
Consequently, I'd expect the two situations handled equally but here the
no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)
However, the might_sleep() in (non-sleepable) RCU reader section combo
makes me wary anyway (not with the early return but tools would likely
complain).
All in all, can't the contract of blkcg_destroy_blkgs() declare that
a caller must pass blkcg with a valid reference? (The body of
blkcg_destroy_blkgs then wouldn't need to get neither put the inner
reference).
HTH,
Michal
On 11/30/22 10:16, Michal Koutný wrote:
> On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <[email protected]> wrote:
>> 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.
> Thanks for the test!
>
>> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>
>> might_sleep();
>>
>> - css_get(&blkcg->css);
>> + /*
>> + * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
>> + * references gone and rcu_read_lock not held.
>> + */
>> + if (!css_tryget(&blkcg->css)) {
>> + WARN_ON_ONCE(!rcu_read_lock_held());
>> + return;
>> + }
> As I followed the previous discussion, the principle is that obtaining a
> reference or being inside an RCU read section is sufficient.
>
> Consequently, I'd expect the two situations handled equally but here the
> no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)
>
> However, the might_sleep() in (non-sleepable) RCU reader section combo
> makes me wary anyway (not with the early return but tools would likely
> complain).
>
> All in all, can't the contract of blkcg_destroy_blkgs() declare that
> a caller must pass blkcg with a valid reference? (The body of
> blkcg_destroy_blkgs then wouldn't need to get neither put the inner
> reference).
You are right. I should have pushed the might_sleep down(). Will post a
new version to fix that.
Thanks,
Longman