[PATCH v2 0/2] cpufreq: Fix a circular lock dependency problem

[PATCH v2 0/2] cpufreq: Fix a circular lock dependency problem

[Waiman Long]

 v2:
  - Remove the retry loop in the store method.

This patchset works around a circular lock dependency issue in the
cpufreq driver reported by lockdep. The two locks involved are the
cpu_hotplup_lock and the reference count of a sysfs file.

The cpufreq_register_driver() function uses the lock sequence:

  cpus_read_lock --> kn->count

Whereas the cpufreq sysfs store method uses the sequence:

  kn->count --> cpus_read_lock

This is not really an issue as a shared lock is used on the
cpu_hotplup_lock. However, the lockdep code isn't able to handle
shared locking. So one way to work around this is to define a
cpus_read_trylock() function and uses it in the store method instead.

Waiman Long (2):
  cpu/hotplug: Add a cpus_read_trylock() function
  cpufreq: Fix a circular lock dependency problem

 drivers/cpufreq/cpufreq.c | 7 ++++++-
 include/linux/cpu.h       | 2 ++
 kernel/cpu.c              | 6 ++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH v2 1/2] cpu/hotplug: Add a cpus_read_trylock() function

[Waiman Long]

There are use cases where it can be useful to have a cpus_read_trylock()
function to work around circular lock dependency problem involving
the cpu_hotplug_lock.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/cpu.h | 2 ++
 kernel/cpu.c        | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index a97a63e..e850bfe 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -103,6 +103,7 @@ static inline void cpu_maps_update_done(void)
 extern void cpus_write_unlock(void);
 extern void cpus_read_lock(void);
 extern void cpus_read_unlock(void);
+extern int  cpus_read_trylock(void);
 extern void lockdep_assert_cpus_held(void);
 extern void cpu_hotplug_disable(void);
 extern void cpu_hotplug_enable(void);
@@ -115,6 +116,7 @@ static inline void cpus_write_lock(void) { }
 static inline void cpus_write_unlock(void) { }
 static inline void cpus_read_lock(void) { }
 static inline void cpus_read_unlock(void) { }
+static inline int  cpus_read_trylock(void) { return true; }
 static inline void lockdep_assert_cpus_held(void) { }
 static inline void cpu_hotplug_disable(void) { }
 static inline void cpu_hotplug_enable(void) { }
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 0db8938..307486b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -290,6 +290,12 @@ void cpus_read_lock(void)
 }
 EXPORT_SYMBOL_GPL(cpus_read_lock);
 
+int cpus_read_trylock(void)
+{
+	return percpu_down_read_trylock(&cpu_hotplug_lock);
+}
+EXPORT_SYMBOL_GPL(cpus_read_trylock);
+
 void cpus_read_unlock(void)
 {
 	percpu_up_read(&cpu_hotplug_lock);
-- 
1.8.3.1

[PATCH v2 2/2] cpufreq: Fix a circular lock dependency problem

[Waiman Long]

With lockdep turned on, the following circular lock dependency problem
was reported:

[   57.470040] ======================================================
[   57.502900] WARNING: possible circular locking dependency detected
[   57.535208] 4.18.0-0.rc3.1.el8+7.x86_64+debug #1 Tainted: G
[   57.577761] ------------------------------------------------------
[   57.609714] tuned/1505 is trying to acquire lock:
[   57.633808] 00000000559deec5 (cpu_hotplug_lock.rw_sem){++++}, at: store+0x27/0x120
[   57.672880]
[   57.672880] but task is already holding lock:
[   57.702184] 000000002136ca64 (kn->count#118){++++}, at: kernfs_fop_write+0x1d0/0x410
[   57.742176]
[   57.742176] which lock already depends on the new lock.
[   57.742176]
[   57.785220]
[   57.785220] the existing dependency chain (in reverse order) is:
    :
[   58.932512] other info that might help us debug this:
[   58.932512]
[   58.973344] Chain exists of:
[   58.973344]   cpu_hotplug_lock.rw_sem --> subsys mutex#5 --> kn->count#118
[   58.973344]
[   59.030795]  Possible unsafe locking scenario:
[   59.030795]
[   59.061248]        CPU0                    CPU1
[   59.085377]        ----                    ----
[   59.108160]   lock(kn->count#118);
[   59.124935]                                lock(subsys mutex#5);
[   59.156330]                                lock(kn->count#118);
[   59.186088]   lock(cpu_hotplug_lock.rw_sem);
[   59.208541]
[   59.208541]  *** DEADLOCK ***

In the cpufreq_register_driver() function, the lock sequence is:

  cpus_read_lock --> kn->count

For the cpufreq sysfs store method, the lock sequence is:

  kn->count --> cpus_read_lock

These sequences are actually safe as they are taking a share lock on
cpu_hotplug_lock. However, the current lockdep code doesn't check for
share locking when detecting circular lock dependency.  Fixing that
could be a substantial effort.

Instead, we can work around this problem by using cpus_read_trylock()
in the store method which is much simpler. The chance of not getting
the read lock is very small. If that happens, the userspace application
that writes the sysfs file will get an error.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 drivers/cpufreq/cpufreq.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b0dfd32..1ddb5ce 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -923,7 +923,12 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	struct freq_attr *fattr = to_attr(attr);
 	ssize_t ret = -EINVAL;
 
-	cpus_read_lock();
+	/*
+	 * cpus_read_trylock() is used here to work around a circular lock
+	 * dependency problem with respect to the cpufreq_register_driver().
+	 */
+	if (!cpus_read_trylock())
+		return -EBUSY;
 
 	if (cpu_online(policy->cpu)) {
 		down_write(&policy->rwsem);
-- 
1.8.3.1

Re: [PATCH v2 0/2] cpufreq: Fix a circular lock dependency problem

[Rafael J. Wysocki]

On Tuesday, July 24, 2018 8:26:03 PM CEST Waiman Long wrote:
>  v2:
>   - Remove the retry loop in the store method.
> 
> This patchset works around a circular lock dependency issue in the
> cpufreq driver reported by lockdep. The two locks involved are the
> cpu_hotplup_lock and the reference count of a sysfs file.
> 
> The cpufreq_register_driver() function uses the lock sequence:
> 
>   cpus_read_lock --> kn->count
> 
> Whereas the cpufreq sysfs store method uses the sequence:
> 
>   kn->count --> cpus_read_lock
> 
> This is not really an issue as a shared lock is used on the
> cpu_hotplup_lock. However, the lockdep code isn't able to handle
> shared locking. So one way to work around this is to define a
> cpus_read_trylock() function and uses it in the store method instead.
> 
> Waiman Long (2):
>   cpu/hotplug: Add a cpus_read_trylock() function
>   cpufreq: Fix a circular lock dependency problem
> 
>  drivers/cpufreq/cpufreq.c | 7 ++++++-
>  include/linux/cpu.h       | 2 ++
>  kernel/cpu.c              | 6 ++++++
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> 

I'm queuing up this series for 4.19.

If anyone has any concerns regarding it, please let me know.

Thanks,
Rafael