* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-09-30 8:50 [PATCH] coredump: fix unfreezable coredumping task Andrey Ryabinin
@ 2016-09-30 9:01 ` Andrey Ryabinin
2016-09-30 12:47 ` Oleg Nesterov
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Andrey Ryabinin @ 2016-09-30 9:01 UTC (permalink / raw)
To: Alexander Viro, Tejun Heo, Rafael J. Wysocki, Pavel Machek,
Oleg Nesterov
Cc: linux-pm, linux-fsdevel, linux-kernel, stable
On 09/30/2016 11:50 AM, Andrey Ryabinin wrote:
> It could be not possible to freeze coredumping task when it waits
> for 'core_state->startup' completion, because threads are frozen
> in get_signal() before they got a chance to complete 'core_state->startup'.
>
> Use freezer_do_not_count() to tell freezer to ignore coredumping
> task while it waits for core_state->startup completion.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: stable@vger.kernel.org
> ---
> fs/coredump.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 281b768..eb9c92c 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -1,6 +1,7 @@
> #include <linux/slab.h>
> #include <linux/file.h>
> #include <linux/fdtable.h>
> +#include <linux/freezer.h>
> #include <linux/mm.h>
> #include <linux/stat.h>
> #include <linux/fcntl.h>
> @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
> if (core_waiters > 0) {
> struct core_thread *ptr;
>
> + freezer_do_not_count();
> wait_for_completion(&core_state->startup);
> + freezer_count();
As alternative to this we could introduce and use wait_for_completion_freezable():
---
fs/coredump.c | 2 +-
include/linux/completion.h | 1 +
kernel/sched/completion.c | 7 +++++++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index 281b768..c145233 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -423,7 +423,7 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
if (core_waiters > 0) {
struct core_thread *ptr;
- wait_for_completion(&core_state->startup);
+ wait_for_completion_freezable(&core_state->startup);
/*
* Wait for all the threads to become inactive, so that
* all the thread context (extended register state, like
diff --git a/include/linux/completion.h b/include/linux/completion.h
index 5d5aaae..8c2626f 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -89,6 +89,7 @@ static inline void reinit_completion(struct completion *x)
}
extern void wait_for_completion(struct completion *);
+extern void wait_for_completion_freezable(struct completion *);
extern void wait_for_completion_io(struct completion *);
extern int wait_for_completion_interruptible(struct completion *x);
extern int wait_for_completion_killable(struct completion *x);
diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
index 8d0f35d..0b9d1e3 100644
--- a/kernel/sched/completion.c
+++ b/kernel/sched/completion.c
@@ -11,6 +11,7 @@
* Waiting for completion is a typically sync point, but not an exclusion point.
*/
+#include <linux/freezer.h>
#include <linux/sched.h>
#include <linux/completion.h>
@@ -123,6 +124,12 @@ void __sched wait_for_completion(struct completion *x)
}
EXPORT_SYMBOL(wait_for_completion);
+void __sched wait_for_completion_freezable(struct completion *x)
+{
+ __wait_for_common(x, freezable_schedule_timeout,
+ MAX_SCHEDULE_TIMEOUT, TASK_UNINTERRUPTIBLE);
+}
+
/**
* wait_for_completion_timeout: - waits for completion of a task (w/timeout)
* @x: holds the state of this particular completion
--
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-09-30 8:50 [PATCH] coredump: fix unfreezable coredumping task Andrey Ryabinin
2016-09-30 9:01 ` Andrey Ryabinin
@ 2016-09-30 12:47 ` Oleg Nesterov
2016-10-04 7:18 ` Michal Hocko
2016-10-03 9:41 ` Pavel Machek
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Oleg Nesterov @ 2016-09-30 12:47 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Alexander Viro, Tejun Heo, Rafael J. Wysocki, Pavel Machek,
linux-pm, linux-fsdevel, linux-kernel, stable
On 09/30, Andrey Ryabinin wrote:
>
> @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
> if (core_waiters > 0) {
> struct core_thread *ptr;
>
> + freezer_do_not_count();
> wait_for_completion(&core_state->startup);
> + freezer_count();
Agreed... we could probably even do
--- x/fs/coredump.c
+++ x/fs/coredump.c
@@ -423,7 +423,13 @@ static int coredump_wait(int exit_code,
if (core_waiters > 0) {
struct core_thread *ptr;
- wait_for_completion(&core_state->startup);
+ if (wait_for_completion_interruptible(&core_state->startup)) {
+ /* see the comment in dump_interrupted() */
+ down_write(&mm->mmap_sem);
+ coredump_finish(mm, false);
+ up_write(&mm->mmap_sem);
+ return -EINTR;
+ }
/*
* Wait for all the threads to become inactive, so that
* all the thread context (extended register state, like
but this change looks fine to me too.
Acked-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-09-30 12:47 ` Oleg Nesterov
@ 2016-10-04 7:18 ` Michal Hocko
2016-10-04 16:13 ` Oleg Nesterov
0 siblings, 1 reply; 10+ messages in thread
From: Michal Hocko @ 2016-10-04 7:18 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrey Ryabinin, Alexander Viro, Tejun Heo, Rafael J. Wysocki,
Pavel Machek, linux-pm, linux-fsdevel, linux-kernel, stable
On Fri 30-09-16 14:47:41, Oleg Nesterov wrote:
> On 09/30, Andrey Ryabinin wrote:
> >
> > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
> > if (core_waiters > 0) {
> > struct core_thread *ptr;
> >
> > + freezer_do_not_count();
> > wait_for_completion(&core_state->startup);
> > + freezer_count();
>
> Agreed... we could probably even do
>
> --- x/fs/coredump.c
> +++ x/fs/coredump.c
> @@ -423,7 +423,13 @@ static int coredump_wait(int exit_code,
> if (core_waiters > 0) {
> struct core_thread *ptr;
>
> - wait_for_completion(&core_state->startup);
> + if (wait_for_completion_interruptible(&core_state->startup)) {
> + /* see the comment in dump_interrupted() */
> + down_write(&mm->mmap_sem);
> + coredump_finish(mm, false);
> + up_write(&mm->mmap_sem);
> + return -EINTR;
> + }
> /*
> * Wait for all the threads to become inactive, so that
> * all the thread context (extended register state, like
This looks like a very good idea to me. We really want to make the whole
coredump_wait killable. I guess this should help us to remove the
hackish sig->flags & SIGNAL_GROUP_COREDUMP check from
__task_will_free_mem. Or are there any other problems that would make
oom victims in the middle of coredump problematic?
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-10-04 7:18 ` Michal Hocko
@ 2016-10-04 16:13 ` Oleg Nesterov
2016-10-05 9:17 ` Michal Hocko
0 siblings, 1 reply; 10+ messages in thread
From: Oleg Nesterov @ 2016-10-04 16:13 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrey Ryabinin, Alexander Viro, Tejun Heo, Rafael J. Wysocki,
Pavel Machek, linux-pm, linux-fsdevel, linux-kernel, stable
On 10/04, Michal Hocko wrote:
>
> On Fri 30-09-16 14:47:41, Oleg Nesterov wrote:
> > On 09/30, Andrey Ryabinin wrote:
> > >
> > > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
> > > if (core_waiters > 0) {
> > > struct core_thread *ptr;
> > >
> > > + freezer_do_not_count();
> > > wait_for_completion(&core_state->startup);
> > > + freezer_count();
> >
> > Agreed... we could probably even do
> >
> > --- x/fs/coredump.c
> > +++ x/fs/coredump.c
> > @@ -423,7 +423,13 @@ static int coredump_wait(int exit_code,
> > if (core_waiters > 0) {
> > struct core_thread *ptr;
> >
> > - wait_for_completion(&core_state->startup);
> > + if (wait_for_completion_interruptible(&core_state->startup)) {
> > + /* see the comment in dump_interrupted() */
> > + down_write(&mm->mmap_sem);
> > + coredump_finish(mm, false);
> > + up_write(&mm->mmap_sem);
> > + return -EINTR;
> > + }
> > /*
> > * Wait for all the threads to become inactive, so that
> > * all the thread context (extended register state, like
>
> This looks like a very good idea to me. We really want to make the whole
> coredump_wait killable.
Well, it is already killable. And with the change above it can sleep
in down_write(mmap_sem) and we really need this lock to abort, so it
won't necessarily react to SIGKILL faster.
> I guess this should help us to remove the
> hackish sig->flags & SIGNAL_GROUP_COREDUMP check from
> __task_will_free_mem.
Why? This doesn't depend on "killable". __task_will_free_mem() checks
this flag to detect the CLONE_VM processes which won't exit soon because
they participate in the coredumping.
Oleg.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-10-04 16:13 ` Oleg Nesterov
@ 2016-10-05 9:17 ` Michal Hocko
0 siblings, 0 replies; 10+ messages in thread
From: Michal Hocko @ 2016-10-05 9:17 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrey Ryabinin, Alexander Viro, Tejun Heo, Rafael J. Wysocki,
Pavel Machek, linux-pm, linux-fsdevel, linux-kernel, stable
On Tue 04-10-16 18:13:05, Oleg Nesterov wrote:
> On 10/04, Michal Hocko wrote:
> >
> > On Fri 30-09-16 14:47:41, Oleg Nesterov wrote:
> > > On 09/30, Andrey Ryabinin wrote:
> > > >
> > > > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
> > > > if (core_waiters > 0) {
> > > > struct core_thread *ptr;
> > > >
> > > > + freezer_do_not_count();
> > > > wait_for_completion(&core_state->startup);
> > > > + freezer_count();
> > >
> > > Agreed... we could probably even do
> > >
> > > --- x/fs/coredump.c
> > > +++ x/fs/coredump.c
> > > @@ -423,7 +423,13 @@ static int coredump_wait(int exit_code,
> > > if (core_waiters > 0) {
> > > struct core_thread *ptr;
> > >
> > > - wait_for_completion(&core_state->startup);
> > > + if (wait_for_completion_interruptible(&core_state->startup)) {
> > > + /* see the comment in dump_interrupted() */
> > > + down_write(&mm->mmap_sem);
> > > + coredump_finish(mm, false);
> > > + up_write(&mm->mmap_sem);
> > > + return -EINTR;
> > > + }
> > > /*
> > > * Wait for all the threads to become inactive, so that
> > > * all the thread context (extended register state, like
> >
> > This looks like a very good idea to me. We really want to make the whole
> > coredump_wait killable.
>
> Well, it is already killable.
Except wait_for_completion is not killable and the exiting tasks might
be blocked in a !killable state blocking this one to continue. But...
> And with the change above it can sleep
> in down_write(mmap_sem) and we really need this lock to abort, so it
> won't necessarily react to SIGKILL faster.
you are right that somebody might be holding mmap_sem and we cannot get
rid of it here.
> > I guess this should help us to remove the
> > hackish sig->flags & SIGNAL_GROUP_COREDUMP check from
> > __task_will_free_mem.
>
> Why? This doesn't depend on "killable". __task_will_free_mem() checks
> this flag to detect the CLONE_VM processes which won't exit soon because
> they participate in the coredumping.
I just (wrongly) assumed that if we make this path killable completely
we can guarantee a forward progress and get rid of SIGNAL_GROUP_COREDUMP
check completely. But you are right this won't be sufficient.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-09-30 8:50 [PATCH] coredump: fix unfreezable coredumping task Andrey Ryabinin
2016-09-30 9:01 ` Andrey Ryabinin
2016-09-30 12:47 ` Oleg Nesterov
@ 2016-10-03 9:41 ` Pavel Machek
2016-11-07 16:27 ` Andrey Ryabinin
2016-11-07 22:26 ` Andrew Morton
4 siblings, 0 replies; 10+ messages in thread
From: Pavel Machek @ 2016-10-03 9:41 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Alexander Viro, Tejun Heo, Rafael J. Wysocki, Oleg Nesterov,
linux-pm, linux-fsdevel, linux-kernel, stable
[-- Attachment #1: Type: text/plain, Size: 677 bytes --]
On Fri 2016-09-30 11:50:34, Andrey Ryabinin wrote:
> It could be not possible to freeze coredumping task when it waits
> for 'core_state->startup' completion, because threads are frozen
> in get_signal() before they got a chance to complete 'core_state->startup'.
>
> Use freezer_do_not_count() to tell freezer to ignore coredumping
> task while it waits for core_state->startup completion.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: stable@vger.kernel.org
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-09-30 8:50 [PATCH] coredump: fix unfreezable coredumping task Andrey Ryabinin
` (2 preceding siblings ...)
2016-10-03 9:41 ` Pavel Machek
@ 2016-11-07 16:27 ` Andrey Ryabinin
2016-11-07 22:26 ` Andrew Morton
4 siblings, 0 replies; 10+ messages in thread
From: Andrey Ryabinin @ 2016-11-07 16:27 UTC (permalink / raw)
To: Alexander Viro, Tejun Heo, Rafael J. Wysocki, Pavel Machek,
Oleg Nesterov
Cc: linux-pm, linux-fsdevel, linux-kernel, stable, Linus Torvalds,
Andrew Morton
On 09/30/2016 11:50 AM, Andrey Ryabinin wrote:
> It could be not possible to freeze coredumping task when it waits
> for 'core_state->startup' completion, because threads are frozen
> in get_signal() before they got a chance to complete 'core_state->startup'.
>
> Use freezer_do_not_count() to tell freezer to ignore coredumping
> task while it waits for core_state->startup completion.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: stable@vger.kernel.org
> ---
Ping. Can someone apply this please?
> fs/coredump.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 281b768..eb9c92c 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -1,6 +1,7 @@
> #include <linux/slab.h>
> #include <linux/file.h>
> #include <linux/fdtable.h>
> +#include <linux/freezer.h>
> #include <linux/mm.h>
> #include <linux/stat.h>
> #include <linux/fcntl.h>
> @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
> if (core_waiters > 0) {
> struct core_thread *ptr;
>
> + freezer_do_not_count();
> wait_for_completion(&core_state->startup);
> + freezer_count();
> /*
> * Wait for all the threads to become inactive, so that
> * all the thread context (extended register state, like
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-09-30 8:50 [PATCH] coredump: fix unfreezable coredumping task Andrey Ryabinin
` (3 preceding siblings ...)
2016-11-07 16:27 ` Andrey Ryabinin
@ 2016-11-07 22:26 ` Andrew Morton
2016-11-08 9:36 ` Andrey Ryabinin
4 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2016-11-07 22:26 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Alexander Viro, Tejun Heo, Rafael J. Wysocki, Pavel Machek,
Oleg Nesterov, linux-pm, linux-fsdevel, linux-kernel, stable
On Fri, 30 Sep 2016 11:50:34 +0300 Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> It could be not possible to freeze coredumping task when it waits
> for 'core_state->startup' completion, because threads are frozen
> in get_signal() before they got a chance to complete 'core_state->startup'.
>
> Use freezer_do_not_count() to tell freezer to ignore coredumping
> task while it waits for core_state->startup completion.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: stable@vger.kernel.org
The changelog provides no reason why this patch should be merged into
-stable. Nor into anything else, really.
Please (as always) provide a full description of the bug's end-user
visible effects.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] coredump: fix unfreezable coredumping task
2016-11-07 22:26 ` Andrew Morton
@ 2016-11-08 9:36 ` Andrey Ryabinin
0 siblings, 0 replies; 10+ messages in thread
From: Andrey Ryabinin @ 2016-11-08 9:36 UTC (permalink / raw)
To: Andrew Morton
Cc: Alexander Viro, Tejun Heo, Rafael J. Wysocki, Pavel Machek,
Oleg Nesterov, linux-pm, linux-fsdevel, linux-kernel, stable
On 11/08/2016 01:26 AM, Andrew Morton wrote:
> On Fri, 30 Sep 2016 11:50:34 +0300 Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>> It could be not possible to freeze coredumping task when it waits
>> for 'core_state->startup' completion, because threads are frozen
>> in get_signal() before they got a chance to complete 'core_state->startup'.
>>
>> Use freezer_do_not_count() to tell freezer to ignore coredumping
>> task while it waits for core_state->startup completion.
>>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Cc: stable@vger.kernel.org
>
> The changelog provides no reason why this patch should be merged into
> -stable. Nor into anything else, really.
>
> Please (as always) provide a full description of the bug's end-user
> visible effects.
>
Sorry. Added the following paragraph:
Inability to freeze a task during suspend will cause suspend to fail.
Also CRIU uses cgroup freezer during dump operation. So with unfreezable
task CRIU dump will fail because it wait for transition from 'FREEZING'
to 'FROZEN' state which will never happen.
----
From: Andrey Ryabinin <aryabinin@virtuozzo.com>
Subject: coredump: fix unfreezable coredumping task
It could be not possible to freeze coredumping task when it waits
for 'core_state->startup' completion, because threads are frozen
in get_signal() before they got a chance to complete 'core_state->startup'.
Inability to freeze a task during suspend will cause suspend to fail.
Also CRIU uses cgroup freezer during dump operation. So with unfreezable
task CRIU dump will fail because it wait for transition from 'FREEZING'
to 'FROZEN' state which will never happen.
Use freezer_do_not_count() to tell freezer to ignore coredumping
task while it waits for core_state->startup completion.
Link: http://lkml.kernel.org/r/1475225434-3753-1-git-send-email-aryabinin@virtuozzo.com
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Tejun Heo <tj@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
fs/coredump.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/coredump.c b/fs/coredump.c
index 281b768..eb9c92c 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -1,6 +1,7 @@
#include <linux/slab.h>
#include <linux/file.h>
#include <linux/fdtable.h>
+#include <linux/freezer.h>
#include <linux/mm.h>
#include <linux/stat.h>
#include <linux/fcntl.h>
@@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
if (core_waiters > 0) {
struct core_thread *ptr;
+ freezer_do_not_count();
wait_for_completion(&core_state->startup);
+ freezer_count();
/*
* Wait for all the threads to become inactive, so that
* all the thread context (extended register state, like
--
^ permalink raw reply related [flat|nested] 10+ messages in thread