Re: [PATCH] cgroup: Stop task iteration when rebinding subsystem

[Michal Koutný]

Hello Jianfeng.

On Fri, May 26, 2023 at 07:41:39PM +0800, Xiu Jianfeng <xiujianfeng@xxxxxxxxxxxxxxx> wrote:
> The race that cause this bug can be shown as below:
> 
> (hotplug cpu)                | (umount cpuset)
> mutex_lock(&cpuset_mutex)    | mutex_lock(&cgroup_mutex)
> cpuset_hotplug_workfn        |
>  rebuild_root_domains        |  rebind_subsystems
>   update_tasks_root_domain   |   spin_lock_irq(&css_set_lock)
>    css_task_iter_start       |    list_move_tail(&cset->e_cset_node[ss->id]
>    while(css_task_iter_next) |                  &dcgrp->e_csets[ss->id]);
>    css_task_iter_end         |   spin_unlock_irq(&css_set_lock)
> mutex_unlock(&cpuset_mutex)  | mutex_unlock(&cgroup_mutex)

Good catch!

> 
> Inside css_task_iter_start/next/end, css_set_lock is hold and then
> released, so when iterating task(left side), the css_set may be moved to
> another list(right side), then it->cset_head points to the old list head
> and it->cset_pos->next points to the head node of new list, which can't
> be used as struct css_set.

I find your analysis sane -- the stale it->cset_head is problematic.

> To fix this issue, introduce CSS_TASK_ITER_STOPPED flag for css_task_iter.
> when moving css_set to dcgrp->e_csets[ss->id] in rebind_subsystems(), stop
> the task iteration.

Does it mean that iteration would not yield all tasks that are
associated with give cs->css? That sounds like broken correctness of the
iterator.

I may suggest a slightly different approach that should not affect
running iterators.
- I had to switch from all css_sets to only scgrp's css_sets since
  css_set_table order of css_sets may be different than scgrp->e_csets
- Not sure how portable is using array element as a `member` argument of
  offsetof (in expansion of list_for_each_entry_safe).

This is only to illustrate the idea, i.e. merely compile tested.

WDYT?

Regards,
Michal

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 625d7483951c..e67d2a0776c1 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1798,7 +1798,7 @@ int rebind_subsystems(struct cgroup_root *dst_root, u16 ss_mask)
 {
 	struct cgroup *dcgrp = &dst_root->cgrp;
 	struct cgroup_subsys *ss;
-	int ssid, i, ret;
+	int ssid, ret;
 	u16 dfl_disable_ss_mask = 0;
 
 	lockdep_assert_held(&cgroup_mutex);
@@ -1842,7 +1842,8 @@ int rebind_subsystems(struct cgroup_root *dst_root, u16 ss_mask)
 		struct cgroup_root *src_root = ss->root;
 		struct cgroup *scgrp = &src_root->cgrp;
 		struct cgroup_subsys_state *css = cgroup_css(scgrp, ss);
-		struct css_set *cset;
+		struct css_set *cset, *cset_pos;
+		struct css_task_iter *it;
 
 		WARN_ON(!css || cgroup_css(dcgrp, ss));
 
@@ -1860,9 +1861,18 @@ int rebind_subsystems(struct cgroup_root *dst_root, u16 ss_mask)
 		css->cgroup = dcgrp;
 
 		spin_lock_irq(&css_set_lock);
-		hash_for_each(css_set_table, i, cset, hlist)
+		WARN_ON(!list_empty(&dcgrp->e_csets[ss->id]));
+		list_for_each_entry_safe(cset, cset_pos, &scgrp->e_csets[ss->id], e_cset_node[ss->id]) {
 			list_move_tail(&cset->e_cset_node[ss->id],
 				       &dcgrp->e_csets[ss->id]);
+			/* all css_sets of scgrp together in same order to dcgrp,
+			 * patch in-flight iterators to preserve correct iteration,
+			 * cset_head is under css_set_lock */
+			list_for_each_entry(it, &cset->task_iters, iters_node) {
+				if (it->cset_head == &scgrp->e_csets[ss->id])
+					it->cset_head = &dcgrp->e_csets[ss->id];
+			}
+		}
 		spin_unlock_irq(&css_set_lock);
 
 		if (ss->css_rstat_flush) {

Attachment: signature.asc
Description: PGP signature