[PATCH] skip increamenting nr for TASK_UNINTERRUPTIBLE

[PATCH] skip increamenting nr for TASK_UNINTERRUPTIBLE

[Vaibhav Shinde]

Dear All,

We found below issue in coredump for uninterruptible task -

>From 1c46f0327d98ad593d8913f9f1dad352f8f44304 Mon Sep 17 00:00:00 2001
From: Ajeet Yadav <ajeet.yadav.77@gmail.com>
Date: Sat, 21 Dec 2013 17:06:05 +0530
Subject: [PATCH] skip increamenting nr for TASK_UNINTERRUPTIBLE

In coredump case, where thread_1 faults while thread_2 is in
TASK_UNINTERRUPTIBLE state, it cannot handle the SIGKILL.
Thus the process hangs on event.
The coredump routine freezes until the thread state is
uninterruptible.

Solution: Continue for coredump, without waiting for uninterruptible
 thread, as it will get killed as soon as it returns from
 uninterruptible state.
 Therefore do not increament thread count for threads with
 TASK_UNINTERRUPTIBLE.

Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
Signed-off-by: Vaibhav Shinde <v.bhav.shinde@gmail.com>
---
 fs/coredump.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 447b02c..54b0664 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -281,7 +281,8 @@ static int zap_process(struct task_struct *start,
int exit_code)
  if (t != current && t->mm) {
  sigaddset(&t->pending.signal, SIGKILL);
  signal_wake_up(t, 1);
- nr++;
+ if(!(t->state & TASK_UNINTERRUPTIBLE))
+ nr++;
  }
  } while_each_thread(start, t);

-- 
1.7.9.5

Re: [PATCH] skip increamenting nr for TASK_UNINTERRUPTIBLE

[Linus Torvalds]

Again, adding Oleg to the cc. And I don't think this is correct, since
SIGKILL can actually kill uninterruptible processes too
(TASK_WAKEKILL) so the basic premise of the patch is incorrect. And
again, we do have that whole issue with exit_mm() serialization.

                   Linus

On Sat, Dec 21, 2013 at 4:18 AM, Vaibhav Shinde <v.bhav.shinde@gmail.com> wrote:
> Dear All,
>
> We found below issue in coredump for uninterruptible task -
>
> From 1c46f0327d98ad593d8913f9f1dad352f8f44304 Mon Sep 17 00:00:00 2001
> From: Ajeet Yadav <ajeet.yadav.77@gmail.com>
> Date: Sat, 21 Dec 2013 17:06:05 +0530
> Subject: [PATCH] skip increamenting nr for TASK_UNINTERRUPTIBLE
>
> In coredump case, where thread_1 faults while thread_2 is in
> TASK_UNINTERRUPTIBLE state, it cannot handle the SIGKILL.
> Thus the process hangs on event.
> The coredump routine freezes until the thread state is
> uninterruptible.
>
> Solution: Continue for coredump, without waiting for uninterruptible
>  thread, as it will get killed as soon as it returns from
>  uninterruptible state.
>  Therefore do not increament thread count for threads with
>  TASK_UNINTERRUPTIBLE.
>
> Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
> Signed-off-by: Vaibhav Shinde <v.bhav.shinde@gmail.com>
> ---
>  fs/coredump.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 447b02c..54b0664 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -281,7 +281,8 @@ static int zap_process(struct task_struct *start,
> int exit_code)
>   if (t != current && t->mm) {
>   sigaddset(&t->pending.signal, SIGKILL);
>   signal_wake_up(t, 1);
> - nr++;
> + if(!(t->state & TASK_UNINTERRUPTIBLE))
> + nr++;
>   }
>   } while_each_thread(start, t);
>
> --
> 1.7.9.5

Re: [PATCH] skip increamenting nr for TASK_UNINTERRUPTIBLE

[Oleg Nesterov]

On 12/21, Linus Torvalds wrote:
>
> Again, adding Oleg to the cc. And I don't think this is correct,

me too, but I can't reply right now, will do tomorrow,


> > In coredump case, where thread_1 faults while thread_2 is in
> > TASK_UNINTERRUPTIBLE state, it cannot handle the SIGKILL.

Yes, and we have to wait. We can not simply ignore its state.
Not to mention, every another_task->state check is racy.

> > Thus the process hangs on event.
> > The coredump routine freezes until the thread state is
> > uninterruptible.
> >
> > Solution: Continue for coredump, without waiting for uninterruptible
> >  thread, as it will get killed as soon as it returns from
> >  uninterruptible state.
> >  Therefore do not increament thread count for threads with
> >  TASK_UNINTERRUPTIBLE.
> >
> > Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
> > Signed-off-by: Vaibhav Shinde <v.bhav.shinde@gmail.com>
> > ---
> >  fs/coredump.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 447b02c..54b0664 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -281,7 +281,8 @@ static int zap_process(struct task_struct *start,
> > int exit_code)
> >   if (t != current && t->mm) {
> >   sigaddset(&t->pending.signal, SIGKILL);
> >   signal_wake_up(t, 1);
> > - nr++;
> > + if(!(t->state & TASK_UNINTERRUPTIBLE))
> > + nr++;
> >   }
> >   } while_each_thread(start, t);
> >
> > --
> > 1.7.9.5

Re: [PATCH] skip increamenting nr for TASK_UNINTERRUPTIBLE

[Oleg Nesterov]

Vaibhav,

again, I think that everything was explained by Linus, let me
add some details.

> > In coredump case, where thread_1 faults while thread_2 is in
> > TASK_UNINTERRUPTIBLE state, it cannot handle the SIGKILL.
> > Thus the process hangs on event.
> > The coredump routine freezes until the thread state is
> > uninterruptible.

Yes. But why we should even try to "fix" coredump in this case?

> > Solution: Continue for coredump, without waiting for uninterruptible
> >  thread,

This can't work, please see below.

> > as it will get killed as soon as it returns from
> >  uninterruptible state.

Not necessarily. It can play with ->mm before it notices the pending
SIGKILL. And, if nothing else, the coredumping paths do not even take
mmap_sem because we assume that the dumper is the only user.

But even if this doesn't happen,

> >  Therefore do not increament thread count for threads with
> >  TASK_UNINTERRUPTIBLE.

This is very wrong too. This means that we can start the coredump before
the _accounted_ thread exits (because a skipped thread can exit first and
decrement the counter). This also means that coredump_finish() can race
with the unaccounted threads.

> >   sigaddset(&t->pending.signal, SIGKILL);
> >   signal_wake_up(t, 1);
> > - nr++;
> > + if(!(t->state & TASK_UNINTERRUPTIBLE))
> > + nr++;

Again, we can't simply check t->state & TASK_UNINTERRUPTIBLE. This can
be false positive or it can sleep in TASK_UNINTERRUPTIBLE right after
the check. And even "& TASK_UNINTERRUPTIBLE" is wrong, please look at
TASK_KILLABLE.

Oleg.