| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090524112943.041935da@linux-lm>
Date: Sun, 24 May 2009 11:29:43 +0800
From: Ming Lei <tom.leiming@...il.com>
To: "Rafael J. Wysocki" <rjw@...k.pl>
Cc: Johannes Berg <johannes@...solutions.net>,
Alan Stern <stern@...land.harvard.edu>,
Oleg Nesterov <oleg@...hat.com>, Ingo Molnar <mingo@...e.hu>,
Zdenek Kabelac <zdenek.kabelac@...il.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
pm list <linux-pm@...ts.linux-foundation.org>
Subject: Re: INFO: possible circular locking dependency at
cleanup_workqueue_thread
δΊ Sun, 24 May 2009 01:20:29 +0200
"Rafael J. Wysocki" <rjw@...k.pl> ει:
> On Saturday 23 May 2009, Johannes Berg wrote:
> > On Sat, 2009-05-23 at 00:23 +0200, Rafael J. Wysocki wrote:
> >
> > > > I just arrived at the same conclusion, heh. I can't say I
> > > > understand these changes though, the part about calling the
> > > > platform differently may make sense, but calling why disable
> > > > non-boot CPUs at a different place?
> > >
> > > Because the ordering of platform callbacks and cpu[_up()|_down()]
> > > is also important, at least on resume.
> > >
> > > In principle we can call device_pm_unlock() right before calling
> > > disable_nonboot_cpus() and take the lock again right after calling
> > > enable_nonboot_cpus(), if that helps.
> >
> > Probably, unless the cpu_add_remove_lock wasn't a red herring after
> > all. I'd test, but I don't have much time today, will be travelling
> > tomorrow and be at UDS all week next week so I don't know when I'll
> > get to it -- could you provide a patch and also attach it to
> > http://bugzilla.kernel.org/show_bug.cgi?id=13245 please? Miles (the
> > reporter of that bug) has been very helpful in testing before.
>
> OK
>
> The patch is appended for reference (Alan, please have a look; I
> can't recall why exactly we have called device_pm_lock() from the
> core suspend/hibernation code instead of acquiring the lock locally
> in drivers/base/power/main.c) and I'll attach it to the bug entry too.
>
> Thanks,
> Rafael
>
> ---
> From: Rafael J. Wysocki <rjw@...k.pl>
> Subject: PM: Do not hold dpm_list_mtx while disabling/enabling
> nonboot CPUs
>
> We shouldn't hold dpm_list_mtx while executing
> [disable|enable]_nonboot_cpus(), because theoretically this may lead
> to a deadlock as shown by the following example (provided by Johannes
> Berg):
>
> CPU 3 CPU 2 CPU 1
> suspend/hibernate
> something:
> rtnl_lock() device_pm_lock()
> -> mutex_lock(&dpm_list_mtx)
>
> mutex_lock(&dpm_list_mtx)
>
> linkwatch_work
> -> rtnl_lock()
> disable_nonboot_cpus()
> -> flush CPU 3 workqueue
>
> Fortunately, device drivers are supposed to stop any activities that
> might lead to the registration of new device objects and/or to the
> removal of the existing ones way before disable_nonboot_cpus() is
> called, so it shouldn't be necessary to hold dpm_list_mtx over the
> entire late part of device suspend and early part of device resume.
>
> Thus, during the late suspend and the early resume of devices acquire
> dpm_list_mtx only when dpm_list is going to be traversed and release
> it right after that.
>
> Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
> ---
> drivers/base/power/main.c | 4 ++++
> kernel/kexec.c | 2 --
> kernel/power/disk.c | 21 +++------------------
> kernel/power/main.c | 7 +------
> 4 files changed, 8 insertions(+), 26 deletions(-)
>
I try to apply the patch against lastest next tree(2009-05-22), but
"patch -p1" is failured:
[lm@...ux-lm linux-2.6]$ patch -p1 < ../patch_rx/INFO_possible_circular_locking_dependency_at_cleanup_workqueue_thread.patch
patching file kernel/power/disk.c
Hunk #1 succeeded at 215 with fuzz 2.
Hunk #3 succeeded at 278 with fuzz 1.
Hunk #4 FAILED at 343.
Hunk #5 succeeded at 396 with fuzz 2 (offset -4 lines).
Hunk #6 FAILED at 454.
Hunk #7 succeeded at 485 with fuzz 2.
2 out of 7 hunks FAILED -- saving rejects to file kernel/power/disk.c.rej
patching file kernel/power/main.c
Hunk #1 succeeded at 289 with fuzz 1 (offset 18 lines).
patching file drivers/base/power/main.c
Hunk #3 succeeded at 616 with fuzz 2.
Hunk #4 succeeded at 625 with fuzz 2.
patching file kernel/kexec.c
Hunk #1 succeeded at 1451 with fuzz 2.
Hunk #2 succeeded at 1488 with fuzz 2.
> Index: linux-2.6/kernel/power/disk.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/disk.c
> +++ linux-2.6/kernel/power/disk.c
> @@ -215,8 +215,6 @@ static int create_image(int platform_mod
> if (error)
> return error;
>
> - device_pm_lock();
> -
> /* At this point, device_suspend() has been called, but *not*
> * device_power_down(). We *must* call device_power_down()
> now.
> * Otherwise, drivers for some devices (e.g. interrupt
> controllers) @@ -227,7 +225,7 @@ static int create_image(int
> platform_mod if (error) {
> printk(KERN_ERR "PM: Some devices failed to power
> down, " "aborting hibernation\n");
> - goto Unlock;
> + return error;
> }
>
> error = platform_pre_snapshot(platform_mode);
> @@ -280,9 +278,6 @@ static int create_image(int platform_mod
> device_power_up(in_suspend ?
> (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
>
> - Unlock:
> - device_pm_unlock();
> -
> return error;
> }
>
> @@ -348,13 +343,11 @@ static int resume_target_kernel(bool pla
> {
> int error;
>
> - device_pm_lock();
> -
> error = device_power_down(PMSG_QUIESCE);
> if (error) {
> printk(KERN_ERR "PM: Some devices failed to power
> down, " "aborting resume\n");
> - goto Unlock;
> + return error;
> }
>
> error = platform_pre_restore(platform_mode);
> @@ -407,9 +400,6 @@ static int resume_target_kernel(bool pla
>
> device_power_up(PMSG_RECOVER);
>
> - Unlock:
> - device_pm_unlock();
> -
> return error;
> }
>
> @@ -468,11 +458,9 @@ int hibernation_platform_enter(void)
> goto Resume_devices;
> }
>
> - device_pm_lock();
> -
> error = device_power_down(PMSG_HIBERNATE);
> if (error)
> - goto Unlock;
> + goto Resume_devices;
>
> error = hibernation_ops->prepare();
> if (error)
> @@ -497,9 +485,6 @@ int hibernation_platform_enter(void)
>
> device_power_up(PMSG_RESTORE);
>
> - Unlock:
> - device_pm_unlock();
> -
> Resume_devices:
> entering_platform_hibernation = false;
> device_resume(PMSG_RESTORE);
> Index: linux-2.6/kernel/power/main.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/main.c
> +++ linux-2.6/kernel/power/main.c
> @@ -271,12 +271,10 @@ static int suspend_enter(suspend_state_t
> {
> int error;
>
> - device_pm_lock();
> -
> if (suspend_ops->prepare) {
> error = suspend_ops->prepare();
> if (error)
> - goto Done;
> + return error;
> }
>
> error = device_power_down(PMSG_SUSPEND);
> @@ -325,9 +323,6 @@ static int suspend_enter(suspend_state_t
> if (suspend_ops->finish)
> suspend_ops->finish();
>
> - Done:
> - device_pm_unlock();
> -
> return error;
> }
>
> Index: linux-2.6/drivers/base/power/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/main.c
> +++ linux-2.6/drivers/base/power/main.c
> @@ -357,6 +357,7 @@ static void dpm_power_up(pm_message_t st
> {
> struct device *dev;
>
> + mutex_lock(&dpm_list_mtx);
> list_for_each_entry(dev, &dpm_list, power.entry)
> if (dev->power.status > DPM_OFF) {
> int error;
> @@ -366,6 +367,7 @@ static void dpm_power_up(pm_message_t st
> if (error)
> pm_dev_err(dev, state, " early",
> error); }
> + mutex_unlock(&dpm_list_mtx);
> }
>
> /**
> @@ -614,6 +616,7 @@ int device_power_down(pm_message_t state
> int error = 0;
>
> suspend_device_irqs();
> + mutex_lock(&dpm_list_mtx);
> list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
> error = suspend_device_noirq(dev, state);
> if (error) {
> @@ -622,6 +625,7 @@ int device_power_down(pm_message_t state
> }
> dev->power.status = DPM_OFF_IRQ;
> }
> + mutex_unlock(&dpm_list_mtx);
> if (error)
> device_power_up(resume_event(state));
> return error;
> Index: linux-2.6/kernel/kexec.c
> ===================================================================
> --- linux-2.6.orig/kernel/kexec.c
> +++ linux-2.6/kernel/kexec.c
> @@ -1451,7 +1451,6 @@ int kernel_kexec(void)
> error = device_suspend(PMSG_FREEZE);
> if (error)
> goto Resume_console;
> - device_pm_lock();
> /* At this point, device_suspend() has been called,
> * but *not* device_power_down(). We *must*
> * device_power_down() now. Otherwise, drivers for
> @@ -1489,7 +1488,6 @@ int kernel_kexec(void)
> enable_nonboot_cpus();
> device_power_up(PMSG_RESTORE);
> Resume_devices:
> - device_pm_unlock();
> device_resume(PMSG_RESTORE);
> Resume_console:
> resume_console();
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Lei Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists