[RFC PATCH] sched/fair: Filter by css_is_dying() in the last tg_unthrottle_up()

[Michal Koutný]

The commit b027789e5e50 ("sched/fair: Prevent dead task groups from
regaining cfs_rq's") tackled the issue of use-after-free of cfs_rqs
surviving on the leaf list after cgroup removal. Two key changes of the
fix are:
- move destroy_cfs_bandwidth() before the list_del_leaf_cfs_rq() so that
  tg_unthrottle_up(cfs_rq) will not re-add the cfs_rq into the list,
- insert RCU GP between task_group tree unlinking and leaf list removal
  (ancestors walk the tree and could call tg_unthrottle_up() (re-add)
  too).

This is correct but it makes the task_group removal path in cpu
controller unnecessarily complex. This patch simplifies the code by
removing the GP but ensuring that cfs_rq won't be re-added when it is
going away. The css_is_dying() check and list_add_leaf_cfs_rq() cannot
race against sched_released_group() because they are both in one RCU
read section and cpu_cgroup_css_released() is called after an RCU GP
(more precisely the sequence is: rmdir, css_is_dying()=1, GP,
.css_offline(), .css_released()).

Autogroups are not cgroups so they are not affected by bandwidth timer
(GP before free is kept).

Cc: Tejun Heo <tj@xxxxxxxxxx>
Signed-off-by: Michal Koutný <mkoutny@xxxxxxxx>
---
 kernel/sched/autogroup.c | 10 ++++++--
 kernel/sched/core.c      | 55 ++++++++--------------------------------
 kernel/sched/fair.c      |  6 +++++
 kernel/sched/sched.h     |  4 +--
 4 files changed, 27 insertions(+), 48 deletions(-)

As explained in the message, this relies on the RCU GP between css_is_dying()
returning false and the potential .css_offline() call. 
This is stated in the css_is_dying() documentation:

> the actual offline operations are RCU delayed and this test returns %true
> also when @css is scheduled to be offlined.

On the other hand the documentation of the underlying
percpu_ref_kill_and_confirm() says (to discourage relying on GP):

> There are no implied RCU grace periods between kill and release.

This seems to discord with each other at first thought. (That's why I marked
this RFC.)

However, if one takes into account that percpu_refs as used by css are never
switched to atomic besides the actual killing (and they start in per-cpu mode),
the GP (inserted in __percpu_ref_switch_to_atomic()) is warranted.


diff --git a/kernel/sched/autogroup.c b/kernel/sched/autogroup.c
index 8629b37d118e..e53d1ea9afc0 100644
--- a/kernel/sched/autogroup.c
+++ b/kernel/sched/autogroup.c
@@ -22,6 +22,11 @@ void autogroup_free(struct task_group *tg)
 	kfree(tg->autogroup);
 }
 
+static void sched_free_group_rcu(struct rcu_head *rcu)
+{
+	sched_free_group(container_of(rcu, struct task_group, rcu));
+}
+
 static inline void autogroup_destroy(struct kref *kref)
 {
 	struct autogroup *ag = container_of(kref, struct autogroup, kref);
@@ -31,8 +36,9 @@ static inline void autogroup_destroy(struct kref *kref)
 	ag->tg->rt_se = NULL;
 	ag->tg->rt_rq = NULL;
 #endif
-	sched_release_group(ag->tg);
-	sched_destroy_group(ag->tg);
+	sched_released_group(ag->tg);
+	/* Wait for possible concurrent references to cfs_rqs complete: */
+	call_rcu(&ag->tg->rcu, sched_free_group_rcu);
 }
 
 static inline void autogroup_kref_put(struct autogroup *ag)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3c9b0fda64ac..2c5b22f54ab8 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -9711,7 +9711,7 @@ static inline void alloc_uclamp_sched_group(struct task_group *tg,
 #endif
 }
 
-static void sched_free_group(struct task_group *tg)
+void sched_free_group(struct task_group *tg)
 {
 	free_fair_sched_group(tg);
 	free_rt_sched_group(tg);
@@ -9719,22 +9719,6 @@ static void sched_free_group(struct task_group *tg)
 	kmem_cache_free(task_group_cache, tg);
 }
 
-static void sched_free_group_rcu(struct rcu_head *rcu)
-{
-	sched_free_group(container_of(rcu, struct task_group, rcu));
-}
-
-static void sched_unregister_group(struct task_group *tg)
-{
-	unregister_fair_sched_group(tg);
-	unregister_rt_sched_group(tg);
-	/*
-	 * We have to wait for yet another RCU grace period to expire, as
-	 * print_cfs_stats() might run concurrently.
-	 */
-	call_rcu(&tg->rcu, sched_free_group_rcu);
-}
-
 /* allocate runqueue etc for a new task group */
 struct task_group *sched_create_group(struct task_group *parent)
 {
@@ -9777,40 +9761,23 @@ void sched_online_group(struct task_group *tg, struct task_group *parent)
 	online_fair_sched_group(tg);
 }
 
-/* rcu callback to free various structures associated with a task group */
-static void sched_unregister_group_rcu(struct rcu_head *rhp)
-{
-	/* Now it should be safe to free those cfs_rqs: */
-	sched_unregister_group(container_of(rhp, struct task_group, rcu));
-}
-
-void sched_destroy_group(struct task_group *tg)
-{
-	/* Wait for possible concurrent references to cfs_rqs complete: */
-	call_rcu(&tg->rcu, sched_unregister_group_rcu);
-}
-
-void sched_release_group(struct task_group *tg)
+void sched_released_group(struct task_group *tg)
 {
 	unsigned long flags;
 
 	/*
-	 * Unlink first, to avoid walk_tg_tree_from() from finding us (via
-	 * sched_cfs_period_timer()).
-	 *
-	 * For this to be effective, we have to wait for all pending users of
-	 * this task group to leave their RCU critical section to ensure no new
-	 * user will see our dying task group any more. Specifically ensure
-	 * that tg_unthrottle_up() won't add decayed cfs_rq's to it.
-	 *
-	 * We therefore defer calling unregister_fair_sched_group() to
-	 * sched_unregister_group() which is guarantied to get called only after the
-	 * current RCU grace period has expired.
+	 * We get here only after all CPUs see tg as dying and an RCU grace
+	 * period. Despite that there may still be concurrent RCU readers
+	 * (walk_tg_tree_from() or task_group list) in their RCU sections.
+	 * Their references to tg stay valid only inside the RCU read section.
 	 */
 	spin_lock_irqsave(&task_group_lock, flags);
 	list_del_rcu(&tg->list);
 	list_del_rcu(&tg->siblings);
 	spin_unlock_irqrestore(&task_group_lock, flags);
+
+	unregister_fair_sched_group(tg);
+	unregister_rt_sched_group(tg);
 }
 
 static void sched_change_group(struct task_struct *tsk, int type)
@@ -9925,7 +9892,7 @@ static void cpu_cgroup_css_released(struct cgroup_subsys_state *css)
 {
 	struct task_group *tg = css_tg(css);
 
-	sched_release_group(tg);
+	sched_released_group(tg);
 }
 
 static void cpu_cgroup_css_free(struct cgroup_subsys_state *css)
@@ -9935,7 +9902,7 @@ static void cpu_cgroup_css_free(struct cgroup_subsys_state *css)
 	/*
 	 * Relies on the RCU grace period between css_released() and this.
 	 */
-	sched_unregister_group(tg);
+	sched_free_group(tg);
 }
 
 /*
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 6e476f6d9435..b3081ece2e16 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4762,6 +4762,12 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
 		cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
 					     cfs_rq->throttled_clock_task;
 
+		/*
+		 * Last tg_unthrottle_up() may happen in a task_group being removed,
+		 * it is only RCU protected so don't store it into leaf list.
+		 */
+		if (css_is_dying(&tg->css))
+			return 0;
 		/* Add cfs_rq with load or one or more already running entities to the list */
 		if (!cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running)
 			list_add_leaf_cfs_rq(cfs_rq);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 0e66749486e7..560151258d92 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -503,8 +503,8 @@ extern int sched_rt_can_attach(struct task_group *tg, struct task_struct *tsk);
 extern struct task_group *sched_create_group(struct task_group *parent);
 extern void sched_online_group(struct task_group *tg,
 			       struct task_group *parent);
-extern void sched_destroy_group(struct task_group *tg);
-extern void sched_release_group(struct task_group *tg);
+extern void sched_released_group(struct task_group *tg);
+extern void sched_free_group(struct task_group *tg);
 
 extern void sched_move_task(struct task_struct *tsk);
 
-- 
2.33.1