On Wed, Apr 05, 2023 at 10:15:32PM +0900, Tetsuo Handa wrote:
> syzbot is reporting circular locking dependency between cpu_hotplug_lock
> and freezer_mutex, for commit f5d39b020809 ("freezer,sched: Rewrite core
> freezer logic") replaced atomic_inc() in freezer_apply_state() with
> static_branch_inc() which holds cpu_hotplug_lock.
>
> cpu_hotplug_lock => cgroup_threadgroup_rwsem => freezer_mutex
>
> cgroup_file_write() {
> cgroup_procs_write() {
> __cgroup_procs_write() {
> cgroup_procs_write_start() {
> cgroup_attach_lock() {
> cpus_read_lock() {
> percpu_down_read(&cpu_hotplug_lock);
> }
> percpu_down_write(&cgroup_threadgroup_rwsem);
> }
> }
> cgroup_attach_task() {
> cgroup_migrate() {
> cgroup_migrate_execute() {
> freezer_attach() {
> mutex_lock(&freezer_mutex);
> (...snipped...)
> }
> }
> }
> }
> (...snipped...)
> }
> }
> }
>
> freezer_mutex => cpu_hotplug_lock
>
> cgroup_file_write() {
> freezer_write() {
> freezer_change_state() {
> mutex_lock(&freezer_mutex);
> freezer_apply_state() {
> static_branch_inc(&freezer_active) {
> static_key_slow_inc() {
> cpus_read_lock();
> static_key_slow_inc_cpuslocked();
> cpus_read_unlock();
> }
> }
> }
> mutex_unlock(&freezer_mutex);
> }
> }
> }
>
> Swap locking order by moving cpus_read_lock() in freezer_apply_state()
> to before mutex_lock(&freezer_mutex) in freezer_change_state().
>
> Reported-by: syzbot <syzbot+c39682e86c9d84152f93@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Link: https://syzkaller.appspot.com/bug?extid=c39682e86c9d84152f93
> Suggested-by: Hillf Danton <hdanton@xxxxxxxx>
> Fixes: f5d39b020809 ("freezer,sched: Rewrite core freezer logic")
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Thanks!
Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> kernel/cgroup/legacy_freezer.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup/legacy_freezer.c b/kernel/cgroup/legacy_freezer.c
> index 1b6b21851e9d..936473203a6b 100644
> --- a/kernel/cgroup/legacy_freezer.c
> +++ b/kernel/cgroup/legacy_freezer.c
> @@ -22,6 +22,7 @@
> #include <linux/freezer.h>
> #include <linux/seq_file.h>
> #include <linux/mutex.h>
> +#include <linux/cpu.h>
>
> /*
> * A cgroup is freezing if any FREEZING flags are set. FREEZING_SELF is
> @@ -350,7 +351,7 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze,
>
> if (freeze) {
> if (!(freezer->state & CGROUP_FREEZING))
> - static_branch_inc(&freezer_active);
> + static_branch_inc_cpuslocked(&freezer_active);
> freezer->state |= state;
> freeze_cgroup(freezer);
> } else {
> @@ -361,7 +362,7 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze,
> if (!(freezer->state & CGROUP_FREEZING)) {
> freezer->state &= ~CGROUP_FROZEN;
> if (was_freezing)
> - static_branch_dec(&freezer_active);
> + static_branch_dec_cpuslocked(&freezer_active);
> unfreeze_cgroup(freezer);
> }
> }
> @@ -379,6 +380,7 @@ static void freezer_change_state(struct freezer *freezer, bool freeze)
> {
> struct cgroup_subsys_state *pos;
>
> + cpus_read_lock();
> /*
> * Update all its descendants in pre-order traversal. Each
> * descendant will try to inherit its parent's FREEZING state as
> @@ -407,6 +409,7 @@ static void freezer_change_state(struct freezer *freezer, bool freeze)
> }
> rcu_read_unlock();
> mutex_unlock(&freezer_mutex);
> + cpus_read_unlock();
> }
>
> static ssize_t freezer_write(struct kernfs_open_file *of,
> --
> 2.34.1
>
