Re: [PATCH cgroup/for-6.0-fixes] cgroup: Fix threadgroup_rwsem <-> cpus_read_lock() deadlock
From: Xuewen Yan
Date: Wed Aug 17 2022 - 02:56:02 EST
Hi Tejun
On Tue, Aug 16, 2022 at 7:27 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
>
> Bringing up a CPU may involve creating new tasks which requires read-locking
> threadgroup_rwsem, so threadgroup_rwsem nests inside cpus_read_lock().
Indeed, it is creating new kthreads. And not only creating new
kthread, but also destroying kthread. the backtrace is:
__switch_to
__schedule
schedule
percpu_rwsem_wait <<< wait for cgroup_threadgroup_rwsem
__percpu_down_read
exit_signals
do_exit
kthread
> 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 <tj@xxxxxxxxxx>
> ---
> 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
Is it better to change to creating new kthreads and destroying kthreads?
> + * 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);
In kernel5.15, I just set cgroup_attach_lock/unlock(true).
> 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 */
I backported your patch. to kernel5.4 and kernel5.15, and just setting
cgroup_attach_lock/unlock(true) when there are conflicts.
And the deadlock has not occured.
Reported-and-tested-by: Xuewen Yan <xuewen.yan@xxxxxxxxxx>
Thanks！