However, cpuset's ->attach(), which may be called with thredagroup_rwsem
write-locked, also wants to disable CPU hotplug and acquires
cpus_read_lock(), leading to a deadlock.

Fix it by guaranteeing that ->attach() is always called with CPU hotplug
disabled and removing cpus_read_lock() call from cpuset_attach().

Signed-off-by: Tejun Heo <redacted>
---
Hello, sorry about the delay.

So, the previous patch + the revert isn't quite correct because we sometimes
elide both cpus_read_lock() and threadgroup_rwsem together and
cpuset_attach() woudl end up running without CPU hotplug enabled. Can you
please test whether this patch fixes the problem?

Thanks.

 kernel/cgroup/cgroup.c |   77 ++++++++++++++++++++++++++++++++++---------------
 kernel/cgroup/cpuset.c |    3 -
 2 files changed, 55 insertions(+), 25 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index ffaccd6373f1e..52502f34fae8c 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c

@@ -2369,6 +2369,47 @@ int task_cgroup_path(struct task_struct *task, char *buf, size_t buflen)
 }
 EXPORT_SYMBOL_GPL(task_cgroup_path);

+/**
+ * cgroup_attach_lock - Lock for ->attach()
+ * @lock_threadgroup: whether to down_write cgroup_threadgroup_rwsem
+ *
+ * cgroup migration sometimes needs to stabilize threadgroups against forks and
+ * exits by write-locking cgroup_threadgroup_rwsem. However, some ->attach()
+ * implementations (e.g. cpuset), also need to disable CPU hotplug.
+ * Unfortunately, letting ->attach() operations acquire cpus_read_lock() can
+ * lead to deadlocks.
+ *
+ * Bringing up a CPU may involve creating new tasks which requires read-locking

+ * threadgroup_rwsem, so threadgroup_rwsem nests inside cpus_read_lock(). If we
+ * call an ->attach() which acquires the cpus lock while write-locking
+ * threadgroup_rwsem, the locking order is reversed and we end up waiting for an
+ * on-going CPU hotplug operation which in turn is waiting for the
+ * threadgroup_rwsem to be released to create new tasks. For more details:
+ *
+ *   http://lkml.kernel.org/r/20220711174629.uehfmqegcwn2lqzu@wubuntu
+ *
+ * Resolve the situation by always acquiring cpus_read_lock() before optionally
+ * write-locking cgroup_threadgroup_rwsem. This allows ->attach() to assume that
+ * CPU hotplug is disabled on entry.
+ */
+static void cgroup_attach_lock(bool lock_threadgroup)
+{
+       cpus_read_lock();
+       if (lock_threadgroup)
+               percpu_down_write(&cgroup_threadgroup_rwsem);
+}
+
+/**
+ * cgroup_attach_unlock - Undo cgroup_attach_lock()
+ * @lock_threadgroup: whether to up_write cgroup_threadgroup_rwsem
+ */
+static void cgroup_attach_unlock(bool lock_threadgroup)
+{
+       if (lock_threadgroup)
+               percpu_up_write(&cgroup_threadgroup_rwsem);
+       cpus_read_unlock();
+}
+
 /**
  * cgroup_migrate_add_task - add a migration target task to a migration context
  * @task: target task

@@ -2841,8 +2882,7 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader,
 }

 struct task_struct *cgroup_procs_write_start(char *buf, bool threadgroup,
-                                            bool *locked)
-       __acquires(&cgroup_threadgroup_rwsem)
+                                            bool *threadgroup_locked)
 {
        struct task_struct *tsk;
        pid_t pid;

@@ -2859,12 +2899,8 @@ struct task_struct *cgroup_procs_write_start(char *buf, bool threadgroup,
         * Therefore, we can skip the global lock.
         */
        lockdep_assert_held(&cgroup_mutex);
-       if (pid || threadgroup) {
-               percpu_down_write(&cgroup_threadgroup_rwsem);
-               *locked = true;
-       } else {
-               *locked = false;
-       }
+       *threadgroup_locked = pid || threadgroup;
+       cgroup_attach_lock(*threadgroup_locked);

        rcu_read_lock();
        if (pid) {

@@ -2895,17 +2931,14 @@ struct task_struct *cgroup_procs_write_start(char *buf, bool threadgroup,
        goto out_unlock_rcu;

 out_unlock_threadgroup:
-       if (*locked) {
-               percpu_up_write(&cgroup_threadgroup_rwsem);
-               *locked = false;
-       }
+       cgroup_attach_unlock(*threadgroup_locked);
+       *threadgroup_locked = false;
 out_unlock_rcu:
        rcu_read_unlock();
        return tsk;
 }

-void cgroup_procs_write_finish(struct task_struct *task, bool locked)
-       __releases(&cgroup_threadgroup_rwsem)
+void cgroup_procs_write_finish(struct task_struct *task, bool threadgroup_locked)
 {
        struct cgroup_subsys *ss;
        int ssid;

@@ -2913,8 +2946,8 @@ void cgroup_procs_write_finish(struct task_struct *task, bool locked)
        /* release reference from cgroup_procs_write_start() */
        put_task_struct(task);

-       if (locked)
-               percpu_up_write(&cgroup_threadgroup_rwsem);
+       cgroup_attach_unlock(threadgroup_locked);
+
        for_each_subsys(ss, ssid)
                if (ss->post_attach)
                        ss->post_attach();

@@ -3000,8 +3033,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
         * write-locking can be skipped safely.
         */
        has_tasks = !list_empty(&mgctx.preloaded_src_csets);
-       if (has_tasks)
-               percpu_down_write(&cgroup_threadgroup_rwsem);
+       cgroup_attach_lock(has_tasks);

        /* NULL dst indicates self on default hierarchy */
        ret = cgroup_migrate_prepare_dst(&mgctx);

@@ -3022,8 +3054,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
        ret = cgroup_migrate_execute(&mgctx);
 out_finish:
        cgroup_migrate_finish(&mgctx);
-       if (has_tasks)
-               percpu_up_write(&cgroup_threadgroup_rwsem);
+       cgroup_attach_unlock(has_tasks);

        return ret;
 }

@@ -4971,13 +5002,13 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
        struct task_struct *task;
        const struct cred *saved_cred;
        ssize_t ret;
-       bool locked;
+       bool threadgroup_locked;

        dst_cgrp = cgroup_kn_lock_live(of->kn, false);
        if (!dst_cgrp)
                return -ENODEV;

-       task = cgroup_procs_write_start(buf, threadgroup, &locked);
+       task = cgroup_procs_write_start(buf, threadgroup, &threadgroup_locked);
        ret = PTR_ERR_OR_ZERO(task);
        if (ret)
                goto out_unlock;

@@ -5003,7 +5034,7 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
        ret = cgroup_attach_task(dst_cgrp, task, threadgroup);

 out_finish:
-       cgroup_procs_write_finish(task, locked);
+       cgroup_procs_write_finish(task, threadgroup_locked);
 out_unlock:
        cgroup_kn_unlock(of->kn);

diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 58aadfda9b8b3..1f3a55297f39d 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c

@@ -2289,7 +2289,7 @@ static void cpuset_attach(struct cgroup_taskset *tset)
        cgroup_taskset_first(tset, &css);
        cs = css_cs(css);

-       cpus_read_lock();
+       lockdep_assert_cpus_held();     /* see cgroup_attach_lock() */
        percpu_down_write(&cpuset_rwsem);

        guarantee_online_mems(cs, &cpuset_attach_nodemask_to);

@@ -2343,7 +2343,6 @@ static void cpuset_attach(struct cgroup_taskset *tset)
                wake_up(&cpuset_attach_wq);

        percpu_up_write(&cpuset_rwsem);
-       cpus_read_unlock();
 }

 /* The various types of files and directories in a cpuset file system */