* [bug report] PM / reboot: Eliminate race between reboot and suspend
@ 2020-11-18 18:59 ` Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-11-18 18:59 UTC (permalink / raw)
To: kernelfans; +Cc: linux-pm, kexec
Hello Pingfan Liu,
The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
and suspend" from Jul 31, 2018, leads to the following static checker
warning:
kernel/power/main.c:27 lock_system_sleep()
warn: called with lock held. '&system_transition_mutex'
kernel/reboot.c
345
346 mutex_lock(&system_transition_mutex);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The patch changed the code to take this lock.
347 switch (cmd) {
348 case LINUX_REBOOT_CMD_RESTART:
349 kernel_restart(NULL);
350 break;
351
352 case LINUX_REBOOT_CMD_CAD_ON:
353 C_A_D = 1;
354 break;
355
356 case LINUX_REBOOT_CMD_CAD_OFF:
357 C_A_D = 0;
358 break;
359
360 case LINUX_REBOOT_CMD_HALT:
361 kernel_halt();
362 do_exit(0);
363 panic("cannot halt");
364
365 case LINUX_REBOOT_CMD_POWER_OFF:
366 kernel_power_off();
367 do_exit(0);
368 break;
369
370 case LINUX_REBOOT_CMD_RESTART2:
371 ret = strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1);
372 if (ret < 0) {
373 ret = -EFAULT;
374 break;
375 }
376 buffer[sizeof(buffer) - 1] = '\0';
377
378 kernel_restart(buffer);
379 break;
380
381 #ifdef CONFIG_KEXEC_CORE
382 case LINUX_REBOOT_CMD_KEXEC:
383 ret = kernel_kexec();
^^^^^^^^^^^^^^^^^^^^
Called with lock held.
384 break;
385 #endif
But kernel_kexec() also tries to take the &system_transition_mutex so
it will dead lock.
kernel/kexec_core.c
1125 int kernel_kexec(void)
1126 {
1127 int error = 0;
1128
1129 if (!mutex_trylock(&kexec_mutex))
1130 return -EBUSY;
1131 if (!kexec_image) {
1132 error = -EINVAL;
1133 goto Unlock;
1134 }
1135
1136 #ifdef CONFIG_KEXEC_JUMP
1137 if (kexec_image->preserve_context) {
1138 lock_system_sleep();
^^^^^^^^^^^^^^^^^^^
Here.
1139 pm_prepare_console();
1140 error = freeze_processes();
1141 if (error) {
1142 error = -EBUSY;
1143 goto Restore_console;
1144 }
1145 suspend_console();
1146 error = dpm_suspend_start(PMSG_FREEZE);
1147 if (error)
1148 goto Resume_console;
1149 /* At this point, dpm_suspend_start() has been called,
1150 * but *not* dpm_suspend_end(). We *must* call
1151 * dpm_suspend_end() now. Otherwise, drivers for
1152 * some devices (e.g. interrupt controllers) become
1153 * desynchronized with the actual state of the
1154 * hardware at resume time, and evil weirdness ensues.
1155 */
1156 error = dpm_suspend_end(PMSG_FREEZE);
1157 if (error)
1158 goto Resume_devices;
1159 error = suspend_disable_secondary_cpus();
1160 if (error)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread* [bug report] PM / reboot: Eliminate race between reboot and suspend
@ 2020-11-18 18:59 ` Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-11-18 18:59 UTC (permalink / raw)
To: kernelfans; +Cc: kexec, linux-pm
Hello Pingfan Liu,
The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
and suspend" from Jul 31, 2018, leads to the following static checker
warning:
kernel/power/main.c:27 lock_system_sleep()
warn: called with lock held. '&system_transition_mutex'
kernel/reboot.c
345
346 mutex_lock(&system_transition_mutex);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The patch changed the code to take this lock.
347 switch (cmd) {
348 case LINUX_REBOOT_CMD_RESTART:
349 kernel_restart(NULL);
350 break;
351
352 case LINUX_REBOOT_CMD_CAD_ON:
353 C_A_D = 1;
354 break;
355
356 case LINUX_REBOOT_CMD_CAD_OFF:
357 C_A_D = 0;
358 break;
359
360 case LINUX_REBOOT_CMD_HALT:
361 kernel_halt();
362 do_exit(0);
363 panic("cannot halt");
364
365 case LINUX_REBOOT_CMD_POWER_OFF:
366 kernel_power_off();
367 do_exit(0);
368 break;
369
370 case LINUX_REBOOT_CMD_RESTART2:
371 ret = strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1);
372 if (ret < 0) {
373 ret = -EFAULT;
374 break;
375 }
376 buffer[sizeof(buffer) - 1] = '\0';
377
378 kernel_restart(buffer);
379 break;
380
381 #ifdef CONFIG_KEXEC_CORE
382 case LINUX_REBOOT_CMD_KEXEC:
383 ret = kernel_kexec();
^^^^^^^^^^^^^^^^^^^^
Called with lock held.
384 break;
385 #endif
But kernel_kexec() also tries to take the &system_transition_mutex so
it will dead lock.
kernel/kexec_core.c
1125 int kernel_kexec(void)
1126 {
1127 int error = 0;
1128
1129 if (!mutex_trylock(&kexec_mutex))
1130 return -EBUSY;
1131 if (!kexec_image) {
1132 error = -EINVAL;
1133 goto Unlock;
1134 }
1135
1136 #ifdef CONFIG_KEXEC_JUMP
1137 if (kexec_image->preserve_context) {
1138 lock_system_sleep();
^^^^^^^^^^^^^^^^^^^
Here.
1139 pm_prepare_console();
1140 error = freeze_processes();
1141 if (error) {
1142 error = -EBUSY;
1143 goto Restore_console;
1144 }
1145 suspend_console();
1146 error = dpm_suspend_start(PMSG_FREEZE);
1147 if (error)
1148 goto Resume_console;
1149 /* At this point, dpm_suspend_start() has been called,
1150 * but *not* dpm_suspend_end(). We *must* call
1151 * dpm_suspend_end() now. Otherwise, drivers for
1152 * some devices (e.g. interrupt controllers) become
1153 * desynchronized with the actual state of the
1154 * hardware at resume time, and evil weirdness ensues.
1155 */
1156 error = dpm_suspend_end(PMSG_FREEZE);
1157 if (error)
1158 goto Resume_devices;
1159 error = suspend_disable_secondary_cpus();
1160 if (error)
regards,
dan carpenter
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
2020-11-18 18:59 ` Dan Carpenter
@ 2021-01-20 9:30 ` Baoquan He
-1 siblings, 0 replies; 11+ messages in thread
From: Baoquan He @ 2021-01-20 9:30 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernelfans, kexec, linux-pm, dyoung
Hi,
On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> Hello Pingfan Liu,
>
> The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> and suspend" from Jul 31, 2018, leads to the following static checker
> warning:
>
> kernel/power/main.c:27 lock_system_sleep()
> warn: called with lock held. '&system_transition_mutex'
This is a good finding. I think we can simply remove the lock/unlock
pair of system_transition_mutex in kernel_kexec() function. The dead
lock should be easily triggered, but it hasn't caused any failure report
because the feature 'kexec jump' is almost not used by anyone as far as
I know. We may need to find out who is using it and where it's used
through an inquiry. Before that, we can just remove the lock operation
inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 80905e5aa8ae..a0b6780740c8 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -1134,7 +1134,6 @@ int kernel_kexec(void)
#ifdef CONFIG_KEXEC_JUMP
if (kexec_image->preserve_context) {
- lock_system_sleep();
pm_prepare_console();
error = freeze_processes();
if (error) {
@@ -1197,7 +1196,6 @@ int kernel_kexec(void)
thaw_processes();
Restore_console:
pm_restore_console();
- unlock_system_sleep();
}
#endif
>
> kernel/reboot.c
> 345
> 346 mutex_lock(&system_transition_mutex);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The patch changed the code to take this lock.
>
> 347 switch (cmd) {
> 348 case LINUX_REBOOT_CMD_RESTART:
> 349 kernel_restart(NULL);
> 350 break;
> 351
> 352 case LINUX_REBOOT_CMD_CAD_ON:
> 353 C_A_D = 1;
> 354 break;
> 355
> 356 case LINUX_REBOOT_CMD_CAD_OFF:
> 357 C_A_D = 0;
> 358 break;
> 359
> 360 case LINUX_REBOOT_CMD_HALT:
> 361 kernel_halt();
> 362 do_exit(0);
> 363 panic("cannot halt");
> 364
> 365 case LINUX_REBOOT_CMD_POWER_OFF:
> 366 kernel_power_off();
> 367 do_exit(0);
> 368 break;
> 369
> 370 case LINUX_REBOOT_CMD_RESTART2:
> 371 ret = strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1);
> 372 if (ret < 0) {
> 373 ret = -EFAULT;
> 374 break;
> 375 }
> 376 buffer[sizeof(buffer) - 1] = '\0';
> 377
> 378 kernel_restart(buffer);
> 379 break;
> 380
> 381 #ifdef CONFIG_KEXEC_CORE
> 382 case LINUX_REBOOT_CMD_KEXEC:
> 383 ret = kernel_kexec();
> ^^^^^^^^^^^^^^^^^^^^
> Called with lock held.
>
> 384 break;
> 385 #endif
>
> But kernel_kexec() also tries to take the &system_transition_mutex so
> it will dead lock.
>
> kernel/kexec_core.c
> 1125 int kernel_kexec(void)
> 1126 {
> 1127 int error = 0;
> 1128
> 1129 if (!mutex_trylock(&kexec_mutex))
> 1130 return -EBUSY;
> 1131 if (!kexec_image) {
> 1132 error = -EINVAL;
> 1133 goto Unlock;
> 1134 }
> 1135
> 1136 #ifdef CONFIG_KEXEC_JUMP
> 1137 if (kexec_image->preserve_context) {
> 1138 lock_system_sleep();
> ^^^^^^^^^^^^^^^^^^^
> Here.
>
> 1139 pm_prepare_console();
> 1140 error = freeze_processes();
> 1141 if (error) {
> 1142 error = -EBUSY;
> 1143 goto Restore_console;
> 1144 }
> 1145 suspend_console();
> 1146 error = dpm_suspend_start(PMSG_FREEZE);
> 1147 if (error)
> 1148 goto Resume_console;
> 1149 /* At this point, dpm_suspend_start() has been called,
> 1150 * but *not* dpm_suspend_end(). We *must* call
> 1151 * dpm_suspend_end() now. Otherwise, drivers for
> 1152 * some devices (e.g. interrupt controllers) become
> 1153 * desynchronized with the actual state of the
> 1154 * hardware at resume time, and evil weirdness ensues.
> 1155 */
> 1156 error = dpm_suspend_end(PMSG_FREEZE);
> 1157 if (error)
> 1158 goto Resume_devices;
> 1159 error = suspend_disable_secondary_cpus();
> 1160 if (error)
>
> regards,
> dan carpenter
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
@ 2021-01-20 9:30 ` Baoquan He
0 siblings, 0 replies; 11+ messages in thread
From: Baoquan He @ 2021-01-20 9:30 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dyoung, kexec, kernelfans, linux-pm
Hi,
On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> Hello Pingfan Liu,
>
> The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> and suspend" from Jul 31, 2018, leads to the following static checker
> warning:
>
> kernel/power/main.c:27 lock_system_sleep()
> warn: called with lock held. '&system_transition_mutex'
This is a good finding. I think we can simply remove the lock/unlock
pair of system_transition_mutex in kernel_kexec() function. The dead
lock should be easily triggered, but it hasn't caused any failure report
because the feature 'kexec jump' is almost not used by anyone as far as
I know. We may need to find out who is using it and where it's used
through an inquiry. Before that, we can just remove the lock operation
inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 80905e5aa8ae..a0b6780740c8 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -1134,7 +1134,6 @@ int kernel_kexec(void)
#ifdef CONFIG_KEXEC_JUMP
if (kexec_image->preserve_context) {
- lock_system_sleep();
pm_prepare_console();
error = freeze_processes();
if (error) {
@@ -1197,7 +1196,6 @@ int kernel_kexec(void)
thaw_processes();
Restore_console:
pm_restore_console();
- unlock_system_sleep();
}
#endif
>
> kernel/reboot.c
> 345
> 346 mutex_lock(&system_transition_mutex);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The patch changed the code to take this lock.
>
> 347 switch (cmd) {
> 348 case LINUX_REBOOT_CMD_RESTART:
> 349 kernel_restart(NULL);
> 350 break;
> 351
> 352 case LINUX_REBOOT_CMD_CAD_ON:
> 353 C_A_D = 1;
> 354 break;
> 355
> 356 case LINUX_REBOOT_CMD_CAD_OFF:
> 357 C_A_D = 0;
> 358 break;
> 359
> 360 case LINUX_REBOOT_CMD_HALT:
> 361 kernel_halt();
> 362 do_exit(0);
> 363 panic("cannot halt");
> 364
> 365 case LINUX_REBOOT_CMD_POWER_OFF:
> 366 kernel_power_off();
> 367 do_exit(0);
> 368 break;
> 369
> 370 case LINUX_REBOOT_CMD_RESTART2:
> 371 ret = strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1);
> 372 if (ret < 0) {
> 373 ret = -EFAULT;
> 374 break;
> 375 }
> 376 buffer[sizeof(buffer) - 1] = '\0';
> 377
> 378 kernel_restart(buffer);
> 379 break;
> 380
> 381 #ifdef CONFIG_KEXEC_CORE
> 382 case LINUX_REBOOT_CMD_KEXEC:
> 383 ret = kernel_kexec();
> ^^^^^^^^^^^^^^^^^^^^
> Called with lock held.
>
> 384 break;
> 385 #endif
>
> But kernel_kexec() also tries to take the &system_transition_mutex so
> it will dead lock.
>
> kernel/kexec_core.c
> 1125 int kernel_kexec(void)
> 1126 {
> 1127 int error = 0;
> 1128
> 1129 if (!mutex_trylock(&kexec_mutex))
> 1130 return -EBUSY;
> 1131 if (!kexec_image) {
> 1132 error = -EINVAL;
> 1133 goto Unlock;
> 1134 }
> 1135
> 1136 #ifdef CONFIG_KEXEC_JUMP
> 1137 if (kexec_image->preserve_context) {
> 1138 lock_system_sleep();
> ^^^^^^^^^^^^^^^^^^^
> Here.
>
> 1139 pm_prepare_console();
> 1140 error = freeze_processes();
> 1141 if (error) {
> 1142 error = -EBUSY;
> 1143 goto Restore_console;
> 1144 }
> 1145 suspend_console();
> 1146 error = dpm_suspend_start(PMSG_FREEZE);
> 1147 if (error)
> 1148 goto Resume_console;
> 1149 /* At this point, dpm_suspend_start() has been called,
> 1150 * but *not* dpm_suspend_end(). We *must* call
> 1151 * dpm_suspend_end() now. Otherwise, drivers for
> 1152 * some devices (e.g. interrupt controllers) become
> 1153 * desynchronized with the actual state of the
> 1154 * hardware at resume time, and evil weirdness ensues.
> 1155 */
> 1156 error = dpm_suspend_end(PMSG_FREEZE);
> 1157 if (error)
> 1158 goto Resume_devices;
> 1159 error = suspend_disable_secondary_cpus();
> 1160 if (error)
>
> regards,
> dan carpenter
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
2021-01-20 9:30 ` Baoquan He
@ 2021-01-21 9:10 ` Pingfan Liu
-1 siblings, 0 replies; 11+ messages in thread
From: Pingfan Liu @ 2021-01-21 9:10 UTC (permalink / raw)
To: Baoquan He; +Cc: Dan Carpenter, Kexec Mailing List, linux-pm, Dave Young
On Wed, Jan 20, 2021 at 5:30 PM Baoquan He <bhe@redhat.com> wrote:
>
> Hi,
>
> On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> > Hello Pingfan Liu,
> >
> > The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> > and suspend" from Jul 31, 2018, leads to the following static checker
> > warning:
> >
> > kernel/power/main.c:27 lock_system_sleep()
> > warn: called with lock held. '&system_transition_mutex'
>
> This is a good finding. I think we can simply remove the lock/unlock
> pair of system_transition_mutex in kernel_kexec() function. The dead
> lock should be easily triggered, but it hasn't caused any failure report
> because the feature 'kexec jump' is almost not used by anyone as far as
> I know. We may need to find out who is using it and where it's used
> through an inquiry. Before that, we can just remove the lock operation
> inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
>
>
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 80905e5aa8ae..a0b6780740c8 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1134,7 +1134,6 @@ int kernel_kexec(void)
>
> #ifdef CONFIG_KEXEC_JUMP
> if (kexec_image->preserve_context) {
> - lock_system_sleep();
> pm_prepare_console();
> error = freeze_processes();
> if (error) {
> @@ -1197,7 +1196,6 @@ int kernel_kexec(void)
> thaw_processes();
> Restore_console:
> pm_restore_console();
> - unlock_system_sleep();
This should work since the only caller syscall_reboot has already
placed kernel_kexec() under the protection of system_transition_mutex.
Thanks for the fix.
Reviewed-by: Pingfan Liu <kernelfans@gmail.com>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
@ 2021-01-21 9:10 ` Pingfan Liu
0 siblings, 0 replies; 11+ messages in thread
From: Pingfan Liu @ 2021-01-21 9:10 UTC (permalink / raw)
To: Baoquan He; +Cc: Dave Young, Kexec Mailing List, Dan Carpenter, linux-pm
On Wed, Jan 20, 2021 at 5:30 PM Baoquan He <bhe@redhat.com> wrote:
>
> Hi,
>
> On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> > Hello Pingfan Liu,
> >
> > The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> > and suspend" from Jul 31, 2018, leads to the following static checker
> > warning:
> >
> > kernel/power/main.c:27 lock_system_sleep()
> > warn: called with lock held. '&system_transition_mutex'
>
> This is a good finding. I think we can simply remove the lock/unlock
> pair of system_transition_mutex in kernel_kexec() function. The dead
> lock should be easily triggered, but it hasn't caused any failure report
> because the feature 'kexec jump' is almost not used by anyone as far as
> I know. We may need to find out who is using it and where it's used
> through an inquiry. Before that, we can just remove the lock operation
> inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
>
>
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 80905e5aa8ae..a0b6780740c8 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1134,7 +1134,6 @@ int kernel_kexec(void)
>
> #ifdef CONFIG_KEXEC_JUMP
> if (kexec_image->preserve_context) {
> - lock_system_sleep();
> pm_prepare_console();
> error = freeze_processes();
> if (error) {
> @@ -1197,7 +1196,6 @@ int kernel_kexec(void)
> thaw_processes();
> Restore_console:
> pm_restore_console();
> - unlock_system_sleep();
This should work since the only caller syscall_reboot has already
placed kernel_kexec() under the protection of system_transition_mutex.
Thanks for the fix.
Reviewed-by: Pingfan Liu <kernelfans@gmail.com>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
2021-01-21 9:10 ` Pingfan Liu
@ 2021-01-21 14:42 ` Rafael J. Wysocki
-1 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2021-01-21 14:42 UTC (permalink / raw)
To: Pingfan Liu, Baoquan He
Cc: Dan Carpenter, Kexec Mailing List, Linux PM, Dave Young
On Thu, Jan 21, 2021 at 10:14 AM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 5:30 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > Hi,
> >
> > On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> > > Hello Pingfan Liu,
> > >
> > > The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> > > and suspend" from Jul 31, 2018, leads to the following static checker
> > > warning:
> > >
> > > kernel/power/main.c:27 lock_system_sleep()
> > > warn: called with lock held. '&system_transition_mutex'
> >
> > This is a good finding. I think we can simply remove the lock/unlock
> > pair of system_transition_mutex in kernel_kexec() function. The dead
> > lock should be easily triggered, but it hasn't caused any failure report
> > because the feature 'kexec jump' is almost not used by anyone as far as
> > I know. We may need to find out who is using it and where it's used
> > through an inquiry. Before that, we can just remove the lock operation
> > inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
> >
> >
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index 80905e5aa8ae..a0b6780740c8 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -1134,7 +1134,6 @@ int kernel_kexec(void)
> >
> > #ifdef CONFIG_KEXEC_JUMP
> > if (kexec_image->preserve_context) {
> > - lock_system_sleep();
> > pm_prepare_console();
> > error = freeze_processes();
> > if (error) {
> > @@ -1197,7 +1196,6 @@ int kernel_kexec(void)
> > thaw_processes();
> > Restore_console:
> > pm_restore_console();
> > - unlock_system_sleep();
>
> This should work since the only caller syscall_reboot has already
> placed kernel_kexec() under the protection of system_transition_mutex.
>
> Thanks for the fix.
>
> Reviewed-by: Pingfan Liu <kernelfans@gmail.com>
OK, so can anyone please submit that patch formally (Cc linux-pm, please)?
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
@ 2021-01-21 14:42 ` Rafael J. Wysocki
0 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2021-01-21 14:42 UTC (permalink / raw)
To: Pingfan Liu, Baoquan He
Cc: Dave Young, Kexec Mailing List, Dan Carpenter, Linux PM
On Thu, Jan 21, 2021 at 10:14 AM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 5:30 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > Hi,
> >
> > On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> > > Hello Pingfan Liu,
> > >
> > > The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> > > and suspend" from Jul 31, 2018, leads to the following static checker
> > > warning:
> > >
> > > kernel/power/main.c:27 lock_system_sleep()
> > > warn: called with lock held. '&system_transition_mutex'
> >
> > This is a good finding. I think we can simply remove the lock/unlock
> > pair of system_transition_mutex in kernel_kexec() function. The dead
> > lock should be easily triggered, but it hasn't caused any failure report
> > because the feature 'kexec jump' is almost not used by anyone as far as
> > I know. We may need to find out who is using it and where it's used
> > through an inquiry. Before that, we can just remove the lock operation
> > inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
> >
> >
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index 80905e5aa8ae..a0b6780740c8 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -1134,7 +1134,6 @@ int kernel_kexec(void)
> >
> > #ifdef CONFIG_KEXEC_JUMP
> > if (kexec_image->preserve_context) {
> > - lock_system_sleep();
> > pm_prepare_console();
> > error = freeze_processes();
> > if (error) {
> > @@ -1197,7 +1196,6 @@ int kernel_kexec(void)
> > thaw_processes();
> > Restore_console:
> > pm_restore_console();
> > - unlock_system_sleep();
>
> This should work since the only caller syscall_reboot has already
> placed kernel_kexec() under the protection of system_transition_mutex.
>
> Thanks for the fix.
>
> Reviewed-by: Pingfan Liu <kernelfans@gmail.com>
OK, so can anyone please submit that patch formally (Cc linux-pm, please)?
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
2021-01-21 14:42 ` Rafael J. Wysocki
@ 2021-01-22 7:38 ` Baoquan He
-1 siblings, 0 replies; 11+ messages in thread
From: Baoquan He @ 2021-01-22 7:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Pingfan Liu, Dave Young, Kexec Mailing List, Dan Carpenter, Linux PM
On 01/21/21 at 03:42pm, Rafael J. Wysocki wrote:
> On Thu, Jan 21, 2021 at 10:14 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 5:30 PM Baoquan He <bhe@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> > > > Hello Pingfan Liu,
> > > >
> > > > The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> > > > and suspend" from Jul 31, 2018, leads to the following static checker
> > > > warning:
> > > >
> > > > kernel/power/main.c:27 lock_system_sleep()
> > > > warn: called with lock held. '&system_transition_mutex'
> > >
> > > This is a good finding. I think we can simply remove the lock/unlock
> > > pair of system_transition_mutex in kernel_kexec() function. The dead
> > > lock should be easily triggered, but it hasn't caused any failure report
> > > because the feature 'kexec jump' is almost not used by anyone as far as
> > > I know. We may need to find out who is using it and where it's used
> > > through an inquiry. Before that, we can just remove the lock operation
> > > inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
> > >
> > >
> > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > index 80905e5aa8ae..a0b6780740c8 100644
> > > --- a/kernel/kexec_core.c
> > > +++ b/kernel/kexec_core.c
> > > @@ -1134,7 +1134,6 @@ int kernel_kexec(void)
> > >
> > > #ifdef CONFIG_KEXEC_JUMP
> > > if (kexec_image->preserve_context) {
> > > - lock_system_sleep();
> > > pm_prepare_console();
> > > error = freeze_processes();
> > > if (error) {
> > > @@ -1197,7 +1196,6 @@ int kernel_kexec(void)
> > > thaw_processes();
> > > Restore_console:
> > > pm_restore_console();
> > > - unlock_system_sleep();
> >
> > This should work since the only caller syscall_reboot has already
> > placed kernel_kexec() under the protection of system_transition_mutex.
> >
> > Thanks for the fix.
> >
> > Reviewed-by: Pingfan Liu <kernelfans@gmail.com>
>
> OK, so can anyone please submit that patch formally (Cc linux-pm, please)?
I will submit a patch with Pingfan's ack, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bug report] PM / reboot: Eliminate race between reboot and suspend
@ 2021-01-22 7:38 ` Baoquan He
0 siblings, 0 replies; 11+ messages in thread
From: Baoquan He @ 2021-01-22 7:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Kexec Mailing List, Dave Young, Dan Carpenter, Pingfan Liu, Linux PM
On 01/21/21 at 03:42pm, Rafael J. Wysocki wrote:
> On Thu, Jan 21, 2021 at 10:14 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 5:30 PM Baoquan He <bhe@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 11/18/20 at 09:59pm, Dan Carpenter wrote:
> > > > Hello Pingfan Liu,
> > > >
> > > > The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
> > > > and suspend" from Jul 31, 2018, leads to the following static checker
> > > > warning:
> > > >
> > > > kernel/power/main.c:27 lock_system_sleep()
> > > > warn: called with lock held. '&system_transition_mutex'
> > >
> > > This is a good finding. I think we can simply remove the lock/unlock
> > > pair of system_transition_mutex in kernel_kexec() function. The dead
> > > lock should be easily triggered, but it hasn't caused any failure report
> > > because the feature 'kexec jump' is almost not used by anyone as far as
> > > I know. We may need to find out who is using it and where it's used
> > > through an inquiry. Before that, we can just remove the lock operation
> > > inside CONFIG_KEXEC_JUMP ifdeffery scope. Thanks.
> > >
> > >
> > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > index 80905e5aa8ae..a0b6780740c8 100644
> > > --- a/kernel/kexec_core.c
> > > +++ b/kernel/kexec_core.c
> > > @@ -1134,7 +1134,6 @@ int kernel_kexec(void)
> > >
> > > #ifdef CONFIG_KEXEC_JUMP
> > > if (kexec_image->preserve_context) {
> > > - lock_system_sleep();
> > > pm_prepare_console();
> > > error = freeze_processes();
> > > if (error) {
> > > @@ -1197,7 +1196,6 @@ int kernel_kexec(void)
> > > thaw_processes();
> > > Restore_console:
> > > pm_restore_console();
> > > - unlock_system_sleep();
> >
> > This should work since the only caller syscall_reboot has already
> > placed kernel_kexec() under the protection of system_transition_mutex.
> >
> > Thanks for the fix.
> >
> > Reviewed-by: Pingfan Liu <kernelfans@gmail.com>
>
> OK, so can anyone please submit that patch formally (Cc linux-pm, please)?
I will submit a patch with Pingfan's ack, thanks.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 11+ messages in thread
* [bug report] PM / reboot: Eliminate race between reboot and suspend
@ 2019-11-19 6:18 Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2019-11-19 6:18 UTC (permalink / raw)
To: kernelfans; +Cc: linux-pm
Hello Pingfan Liu,
The patch 55f2503c3b69: "PM / reboot: Eliminate race between reboot
and suspend" from Jul 31, 2018, leads to the following static checker
warning:
kernel/reboot.c:395 __do_sys_reboot()
error: double unlocked 'system_transition_mutex' (orig line 387)
kernel/reboot.c
310 SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
311 void __user *, arg)
312 {
313 struct pid_namespace *pid_ns = task_active_pid_ns(current);
314 char buffer[256];
315 int ret = 0;
316
317 /* We only trust the superuser with rebooting the system. */
318 if (!ns_capable(pid_ns->user_ns, CAP_SYS_BOOT))
319 return -EPERM;
320
321 /* For safety, we require "magic" arguments. */
322 if (magic1 != LINUX_REBOOT_MAGIC1 ||
323 (magic2 != LINUX_REBOOT_MAGIC2 &&
324 magic2 != LINUX_REBOOT_MAGIC2A &&
325 magic2 != LINUX_REBOOT_MAGIC2B &&
326 magic2 != LINUX_REBOOT_MAGIC2C))
327 return -EINVAL;
328
329 /*
330 * If pid namespaces are enabled and the current task is in a child
331 * pid_namespace, the command is handled by reboot_pid_ns() which will
332 * call do_exit().
333 */
334 ret = reboot_pid_ns(pid_ns, cmd);
335 if (ret)
336 return ret;
337
338 /* Instead of trying to make the power_off code look like
339 * halt when pm_power_off is not set do it the easy way.
340 */
341 if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
342 cmd = LINUX_REBOOT_CMD_HALT;
343
344 mutex_lock(&system_transition_mutex);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This used to be reboot_mutex.
345 switch (cmd) {
346 case LINUX_REBOOT_CMD_RESTART:
347 kernel_restart(NULL);
348 break;
349
350 case LINUX_REBOOT_CMD_CAD_ON:
351 C_A_D = 1;
352 break;
353
354 case LINUX_REBOOT_CMD_CAD_OFF:
355 C_A_D = 0;
356 break;
357
358 case LINUX_REBOOT_CMD_HALT:
359 kernel_halt();
360 do_exit(0);
361 panic("cannot halt");
362
363 case LINUX_REBOOT_CMD_POWER_OFF:
364 kernel_power_off();
365 do_exit(0);
366 break;
367
368 case LINUX_REBOOT_CMD_RESTART2:
369 ret = strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1);
370 if (ret < 0) {
371 ret = -EFAULT;
372 break;
373 }
374 buffer[sizeof(buffer) - 1] = '\0';
375
376 kernel_restart(buffer);
377 break;
378
379 #ifdef CONFIG_KEXEC_CORE
380 case LINUX_REBOOT_CMD_KEXEC:
381 ret = kernel_kexec();
382 break;
383 #endif
384
385 #ifdef CONFIG_HIBERNATION
386 case LINUX_REBOOT_CMD_SW_SUSPEND:
387 ret = hibernate();
^^^^^^^^^^^^^^^^^
The first thing that hibernate() does is call lock_system_sleep() which
use to take the pm_mutex, but now it takes system_transition_mutex so
it will just hang forever.
388 break;
389 #endif
390
391 default:
392 ret = -EINVAL;
393 break;
394 }
395 mutex_unlock(&system_transition_mutex);
396 return ret;
397 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-01-22 7:40 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 18:59 [bug report] PM / reboot: Eliminate race between reboot and suspend Dan Carpenter
2020-11-18 18:59 ` Dan Carpenter
2021-01-20 9:30 ` Baoquan He
2021-01-20 9:30 ` Baoquan He
2021-01-21 9:10 ` Pingfan Liu
2021-01-21 9:10 ` Pingfan Liu
2021-01-21 14:42 ` Rafael J. Wysocki
2021-01-21 14:42 ` Rafael J. Wysocki
2021-01-22 7:38 ` Baoquan He
2021-01-22 7:38 ` Baoquan He
-- strict thread matches above, loose matches on Subject: below --
2019-11-19 6:18 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.