On Wed, Jul 17, 2019 at 10:56 AM Christian Brauner <christ...@brauner.io> wrote:
>
> On Wed, Jul 17, 2019 at 01:21:00PM -0400, Joel Fernandes wrote:
> > From: Suren Baghdasaryan <sur...@google.com>
> >
> > There is a race between reading task->exit_state in pidfd_poll and writing
> > it after do_notify_parent calls do_notify_pidfd. Expected sequence of
> > events is:
> >
> > CPU 0 CPU 1
> > ------------------------------------------------
> > exit_notify
> > do_notify_parent
> > do_notify_pidfd
> > tsk->exit_state = EXIT_DEAD
> > pidfd_poll
> > if (tsk->exit_state)
> >
> > However nothing prevents the following sequence:
> >
> > CPU 0 CPU 1
> > ------------------------------------------------
> > exit_notify
> > do_notify_parent
> > do_notify_pidfd
> > pidfd_poll
> > if (tsk->exit_state)
> > tsk->exit_state = EXIT_DEAD
> >
> > This causes a polling task to wait forever, since poll blocks because
> > exit_state is 0 and the waiting task is not notified again. A stress
> > test continuously doing pidfd poll and process exits uncovered this bug,
> > and the below patch fixes it.
> >
> > To fix this, we set tsk->exit_state before calling do_notify_pidfd.
> >
> > Cc: kernel-t...@android.com
> > Signed-off-by: Suren Baghdasaryan <sur...@google.com>
> > Signed-off-by: Joel Fernandes (Google) <j...@joelfernandes.org>
>
> That means in such a situation other users will see EXIT_ZOMBIE where
> they didn't see that before until after the parent failed to get
> notified.
I'm a little nervous about that myself even though in my stress
testing this worked fine. I think the safest change would be to move
do_notify_pidfd() out of do_notify_parent() and call it after
tsk->exit_state is finalized. The downside of that is that there are 4
places we call do_notify_parent(), so instead of calling
do_notify_pidfd() one time from do_notify_parent() we will be calling
it 4 times now.
Also my original patch had memory barriers to ensure correct ordering
of tsk->exit_state writes before reads. In this final version Joel
removed them, so I suppose he found out they are not needed. Joel,
please clarify.
Thanks!
> That's a rather subtle internal change. I was worried about
> __ptrace_detach() since it explicitly checks for EXIT_ZOMBIE but it
> seems to me that this is fine since we hold write_lock_irq(&tasklist_lock);
> at the point when we do set p->exit_signal.
>
> Acked-by: Christian Brauner <christ...@brauner.io>
>
> Once Oleg confirms that I'm right not to worty I'll pick this up.
>
> Thanks!
> Christian
>
> >
> > ---
> > kernel/exit.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index a75b6a7f458a..740ceacb4b76 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -720,6 +720,7 @@ static void exit_notify(struct task_struct *tsk, int
> > group_dead)
> > if (group_dead)
> > kill_orphaned_pgrp(tsk->group_leader, NULL);
> >
> > + tsk->exit_state = EXIT_ZOMBIE;
> > if (unlikely(tsk->ptrace)) {
> > int sig = thread_group_leader(tsk) &&
> > thread_group_empty(tsk) &&
> > @@ -1156,10 +1157,11 @@ static int wait_task_zombie(struct wait_opts *wo,
> > struct task_struct *p)
> > ptrace_unlink(p);
> >
> > /* If parent wants a zombie, don't release it now */
> > - state = EXIT_ZOMBIE;
> > + p->exit_state = EXIT_ZOMBIE;
> > if (do_notify_parent(p, p->exit_signal))
> > - state = EXIT_DEAD;
> > - p->exit_state = state;
> > + p->exit_state = EXIT_DEAD;
> > +
> > + state = p->exit_state;
> > write_unlock_irq(&tasklist_lock);
> > }
> > if (state == EXIT_DEAD)
> > --
> > 2.22.0.657.g960e92d24f-goog
> >
