Commit 57dcd64c authored by Tetsuo Handa's avatar Tetsuo Handa Committed by Tejun Heo

cgroup,freezer: hold cpu_hotplug_lock before freezer_mutex

syzbot is reporting circular locking dependency between cpu_hotplug_lock
and freezer_mutex, for commit f5d39b02 ("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: default avatarsyzbot <syzbot+c39682e86c9d84152f93@syzkaller.appspotmail.com>
Link: https://syzkaller.appspot.com/bug?extid=c39682e86c9d84152f93Suggested-by: default avatarHillf Danton <hdanton@sina.com>
Fixes: f5d39b02 ("freezer,sched: Rewrite core freezer logic")
Signed-off-by: default avatarTetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: default avatarPeter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: default avatarMukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: default avatarTejun Heo <tj@kernel.org>
parent 292fd843
...@@ -22,6 +22,7 @@ ...@@ -22,6 +22,7 @@
#include <linux/freezer.h> #include <linux/freezer.h>
#include <linux/seq_file.h> #include <linux/seq_file.h>
#include <linux/mutex.h> #include <linux/mutex.h>
#include <linux/cpu.h>
/* /*
* A cgroup is freezing if any FREEZING flags are set. FREEZING_SELF is * 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, ...@@ -350,7 +351,7 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze,
if (freeze) { if (freeze) {
if (!(freezer->state & CGROUP_FREEZING)) if (!(freezer->state & CGROUP_FREEZING))
static_branch_inc(&freezer_active); static_branch_inc_cpuslocked(&freezer_active);
freezer->state |= state; freezer->state |= state;
freeze_cgroup(freezer); freeze_cgroup(freezer);
} else { } else {
...@@ -361,7 +362,7 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze, ...@@ -361,7 +362,7 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze,
if (!(freezer->state & CGROUP_FREEZING)) { if (!(freezer->state & CGROUP_FREEZING)) {
freezer->state &= ~CGROUP_FROZEN; freezer->state &= ~CGROUP_FROZEN;
if (was_freezing) if (was_freezing)
static_branch_dec(&freezer_active); static_branch_dec_cpuslocked(&freezer_active);
unfreeze_cgroup(freezer); unfreeze_cgroup(freezer);
} }
} }
...@@ -379,6 +380,7 @@ static void freezer_change_state(struct freezer *freezer, bool freeze) ...@@ -379,6 +380,7 @@ static void freezer_change_state(struct freezer *freezer, bool freeze)
{ {
struct cgroup_subsys_state *pos; struct cgroup_subsys_state *pos;
cpus_read_lock();
/* /*
* Update all its descendants in pre-order traversal. Each * Update all its descendants in pre-order traversal. Each
* descendant will try to inherit its parent's FREEZING state as * 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) ...@@ -407,6 +409,7 @@ static void freezer_change_state(struct freezer *freezer, bool freeze)
} }
rcu_read_unlock(); rcu_read_unlock();
mutex_unlock(&freezer_mutex); mutex_unlock(&freezer_mutex);
cpus_read_unlock();
} }
static ssize_t freezer_write(struct kernfs_open_file *of, static ssize_t freezer_write(struct kernfs_open_file *of,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment