cgroup: Make the offline drain interruptible

Hello,

syzbot has been reporting "task hung in cgroup_subtree_control_write".

The underlying problem is an indefinite wait in a place where indefinite waits should not happen. A task past exit_signals() can block in its exit path for as long as, say, a FUSE daemon takes to answer a flush.

That wait is interruptible in name only. Once a thread group is exiting, prepare_signal() drops every signal sent to it, so nothing can wake the task. It still shows as TASK_INTERRUPTIBLE, and the hung task detector only looks at TASK_UNINTERRUPTIBLE, so the stuck task never appears in any report.

Since 1dffd95575eb ("cgroup: Defer kill_css_finish() in cgroup_apply_control_disable()"), a disabled css offlines only after every such task pinning it has exited, and the re-enable path waits for that offline in TASK_UNINTERRUPTIBLE. The writer inherits the unkillable wait, sits in D state, and is the only thing the hung task report shows.

This patchset makes the drain interruptible so that the writer can be killed. The wait in the exit path remains a separate problem.

Tao Yu posted a patch that rejects the re-enable with -EBUSY while csses are dying:

https://lore.kernel.org/all/[email protected]/

That fails re-enables that would have succeeded and keys off a counter that also covers removed cgroups and offlined-but-pinned csses, so this takes the interruptible wait instead.

0001-cgroup-Return-ENODEV-consistently-for-a-dead-cgroup.patch 0002-cgroup-Return-ERR_PTR-from-cgroup_kn_lock_live.patch 0003-cgroup-Make-the-offline-drain-interruptible.patch

0001 unifies the errno the interface file writers return for a removed cgroup. 0002 makes cgroup_kn_lock_live() return ERR_PTR so that it can report the interrupted drain. 0003 is the actual change.

This is against v7.3-rc1 (cee9395acd80) and also available in the following git branch:

https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git cgroup-drain-interruptible

diffstat follows.

 kernel/cgroup/cgroup-internal.h |  2 +-
 kernel/cgroup/cgroup-v1.c       | 16 ++++---
 kernel/cgroup/cgroup.c          | 98 ++++++++++++++++++++++++-----------------
 kernel/cgroup/debug.c           | 10 ++---
 4 files changed, 74 insertions(+), 52 deletions(-)

--
tejun

cgroup_kn_lock_live() returns NULL when the cgroup behind a kernfs node has already been removed. Most callers turn that into -ENODEV, but the cgroup.type, cgroup.max.descendants, cgroup.max.depth, cgroup.pressure, cgroup.freeze and cgroup.kill writers return -ENOENT. The divergence is accidental. Return -ENODEV from all of them, matching cgroup.procs and cgroup.subtree_control.

---
 kernel/cgroup/cgroup.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index c3a12fee7528..37bc8a67a40f 100644
@@ -3791,7 +3791,7 @@ static ssize_t cgroup_type_write(struct kernfs_open_file *of, char *buf,
 	/* drain dying csses before we re-apply (threaded) subtree control */
 	cgrp = cgroup_kn_lock_live(of->kn, true);
 	if (!cgrp)
-		return -ENOENT;
+		return -ENODEV;
 
 	/* threaded can only be enabled */
 	ret = cgroup_enable_threaded(cgrp);

@@ -3834,7 +3834,7 @@ static ssize_t cgroup_max_descendants_write(struct kernfs_open_file *of,
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
 	if (!cgrp)
-		return -ENOENT;
+		return -ENODEV;
 
 	WRITE_ONCE(cgrp->max_descendants, descendants);
 

@@ -3877,7 +3877,7 @@ static ssize_t cgroup_max_depth_write(struct kernfs_open_file *of,
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
 	if (!cgrp)
-		return -ENOENT;
+		return -ENODEV;
 
 	WRITE_ONCE(cgrp->max_depth, depth);
 

@@ -4193,7 +4193,7 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of,
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
 	if (!cgrp)
-		return -ENOENT;
+		return -ENODEV;
 
 	psi = cgroup_psi(cgrp);
 	if (psi->enabled != enable) {

@@ -4269,7 +4269,7 @@ static ssize_t cgroup_freeze_write(struct kernfs_open_file *of,
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
 	if (!cgrp)
-		return -ENOENT;
+		return -ENODEV;
 
 	cgroup_freeze(cgrp, freeze);
 

@@ -4331,7 +4331,7 @@ static ssize_t cgroup_kill_write(struct kernfs_open_file *of, char *buf,
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
 	if (!cgrp)
-		return -ENOENT;
+		return -ENODEV;
 
 	/*
 	 * Killing is a process directed operation, i.e. the whole thread-group

2.55.0

cgroup_kn_lock_live() returns NULL when the cgroup is dead and the callers translate that into an errno themselves. Return ERR_PTR(-ENODEV) instead and have the callers pass the errno through, so that failures other than a dead cgroup can be reported through the same return value. No functional change.

---
 kernel/cgroup/cgroup-v1.c |  8 +++----
 kernel/cgroup/cgroup.c    | 50 +++++++++++++++++++--------------------
 kernel/cgroup/debug.c     | 10 ++++----
 3 files changed, 34 insertions(+), 34 deletions(-)

index a4337c9b5287..7e008867f3ac 100644
@@ -506,8 +506,8 @@ static ssize_t __cgroup1_procs_write(struct kernfs_open_file *of,
 	enum cgroup_attach_lock_mode lock_mode;
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
-	if (!cgrp)
-		return -ENODEV;
+	if (IS_ERR(cgrp))
+		return PTR_ERR(cgrp);
 
 	task = cgroup_procs_write_start(buf, threadgroup, &lock_mode);
 	ret = PTR_ERR_OR_ZERO(task);

@@ -569,8 +569,8 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
 		return -EPERM;
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
-	if (!cgrp)
-		return -ENODEV;
+	if (IS_ERR(cgrp))
+		return PTR_ERR(cgrp);
 	spin_lock(&release_agent_path_lock);
 	strscpy(cgrp->root->release_agent_path, strstrip(buf),
 		sizeof(cgrp->root->release_agent_path));

index 883347b87842..96004cd65d09 100644
@@ -45,7 +45,7 @@ static int current_css_set_read(struct seq_file *seq, void *v)
 	struct cgroup_subsys_state *css;
 	int i, refcnt;
 
-	if (!cgroup_kn_lock_live(of->kn, false))
+	if (IS_ERR(cgroup_kn_lock_live(of->kn, false)))
 		return -ENODEV;
 
 	spin_lock_irq(&css_set_lock);

@@ -206,8 +206,8 @@ static int cgroup_subsys_states_read(struct seq_file *seq, void *v)
 	int i;
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
-	if (!cgrp)
-		return -ENODEV;
+	if (IS_ERR(cgrp))
+		return PTR_ERR(cgrp);
 
 	for_each_subsys(ss, i) {
 		css = rcu_dereference_check(cgrp->subsys[ss->id], true);

@@ -254,8 +254,8 @@ static int cgroup_masks_read(struct seq_file *seq, void *v)
 	struct cgroup *cgrp;
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
-	if (!cgrp)
-		return -ENODEV;
+	if (IS_ERR(cgrp))
+		return PTR_ERR(cgrp);
 
 	cgroup_masks_read_one(seq, "subtree_control", cgrp->subtree_control);
 	cgroup_masks_read_one(seq, "subtree_ss_mask", cgrp->subtree_ss_mask);

2.55.0

cgroup_subtree_control_write() and cgroup_type_write() drain dying csses from the subtree before re-applying control, so that a controller being re-enabled gets a fresh css instead of adopting one that is still dying. The drain waits for offline_css() in TASK_UNINTERRUPTIBLE.

Since 1dffd95575eb ("cgroup: Defer kill_css_finish() in cgroup_apply_control_disable()"), a disabled css offlines only after every task that still pins it has finished exiting. The root cause of the hang is an indefinite wait in a place where indefinite waits should not happen: a task past exit_signals() can block in its exit path for as long as, say, a FUSE daemon takes to answer a flush, and nothing can interrupt it because prepare_signal() drops every signal for an exiting group. The drain then escalated that into an uninterruptible wait for the writer, which sits in D state for as long as the exiting task does. That is the hang syzbot reports as "task hung in cgroup_subtree_control_write".

Make the drain interruptible. Nothing has been modified when it runs, so the write restarts or fails with EINTR without side effects. cgroup1 mount and remount drain the same way and become interruptible too. cgroup_destroy_root() drains from a workqueue where no signal is ever pending and keeps waiting as before.

This only stops the escalation. The write still waits for the exiting tasks, and the indefinite wait in the exit path remains a separate problem.

---
 kernel/cgroup/cgroup-internal.h |  2 +-
 kernel/cgroup/cgroup-v1.c       |  8 ++++--
 kernel/cgroup/cgroup.c          | 48 ++++++++++++++++++++++-----------
 3 files changed, 40 insertions(+), 18 deletions(-)

index 58797123b752..f2684f9f8e0a 100644
@@ -254,7 +254,7 @@ void cgroup_procs_write_finish(struct task_struct *task,
 			       enum cgroup_attach_lock_mode lock_mode)
 	__releases(&cgroup_threadgroup_rwsem);
 
-void cgroup_lock_and_drain_offline(struct cgroup *cgrp);
+int cgroup_lock_and_drain_offline(struct cgroup *cgrp);
 
 int cgroup_mkdir(struct kernfs_node *parent_kn, const char *name, umode_t mode);
 int cgroup_rmdir(struct kernfs_node *kn);

index 7e008867f3ac..167bf6555a49 100644
@@ -1097,7 +1097,9 @@ int cgroup1_reconfigure(struct fs_context *fc)
 	int ret = 0;
 	u32 added_mask, removed_mask;
 
-	cgroup_lock_and_drain_offline(&cgrp_dfl_root.cgrp);
+	ret = cgroup_lock_and_drain_offline(&cgrp_dfl_root.cgrp);
+	if (unlikely(ret))
+		return ret;
 
 	/* See what subsystems are wanted */
 	ret = check_cgroupfs_options(fc);

@@ -1262,7 +1264,9 @@ int cgroup1_get_tree(struct fs_context *fc)
 	if (!ns_capable(ctx->ns->user_ns, CAP_SYS_ADMIN))
 		return -EPERM;
 
-	cgroup_lock_and_drain_offline(&cgrp_dfl_root.cgrp);
+	ret = cgroup_lock_and_drain_offline(&cgrp_dfl_root.cgrp);
+	if (unlikely(ret))
+		return ret;
 
 	ret = cgroup1_root_to_use(fc);
 	if (!ret && !percpu_ref_tryget_live(&ctx->root->cgrp.self.refcnt))

2.55.0

Hello,

Applied 1-3 to cgroup/for-7.4.

Thanks.

tejun