Re: [PATCH RFC v1] pidfd: fix a race in setting exit_state for pidfd polling

[Suren Baghdasaryan]

On Wed, Jul 17, 2019 at 10:56 AM Christian Brauner <christian@xxxxxxxxxx> wrote:
>
> On Wed, Jul 17, 2019 at 01:21:00PM -0400, Joel Fernandes wrote:
> > From: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> >
> > 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-team@xxxxxxxxxxx
> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
>
> 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 <christian@xxxxxxxxxx>
>
> 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
> >