+ exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree

+ exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree

[akpm]

Subject: + exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree
To: guillaume@morinfr.org,davem@davemloft.net,matt.helsley@gmail.com,oleg@redhat.com
From: akpm@linux-foundation.org
Date: Mon, 24 Feb 2014 13:53:29 -0800


The patch titled
     Subject: kernel/exit.c: call proc_exit_connector() after exit_state is set
has been added to the -mm tree.  Its filename is
     exitc-call-proc_exit_connector-after-exit_state-is-set.patch

This patch should soon appear at
    http://ozlabs.org/~akpm/mmots/broken-out/exitc-call-proc_exit_connector-after-exit_state-is-set.patch
and later at
    http://ozlabs.org/~akpm/mmotm/broken-out/exitc-call-proc_exit_connector-after-exit_state-is-set.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: Guillaume Morin <guillaume@morinfr.org>
Subject: kernel/exit.c: call proc_exit_connector() after exit_state is set

The process events connector delivers a notification when a process exits.
 This is really convenient for a process that spawns and wants to monitor
its children through an epoll-able() interface.

Unfortunately, there is a small window between when the event is delivered
and the child become wait()-able.

This is creates a race if the parent wants to make sure that it knows
about the exit, e.g

pid_t pid = fork();
if (pid > 0) {
	register_interest_for_pid(pid);
	if (waitpid(pid, NULL, WNOHANG) > 0)
	{
	  /* We might have raced with exit() */
	}
	return;
}

/* Child */
execve(...)

register_interest_for_pid() would be telling the the connector socket
reader to pay attention to events related to pid.

Though this is not a bug, I think it would make the connector a bit more
usable if this race was closed by simply moving the call to
proc_exit_connector() from just before exit_notify() to right after.

Signed-off-by: Guillaume Morin <guillaume@morinfr.org>
Cc: Matt Helsley <matt.helsley@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/exit.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN kernel/exit.c~exitc-call-proc_exit_connector-after-exit_state-is-set kernel/exit.c
--- a/kernel/exit.c~exitc-call-proc_exit_connector-after-exit_state-is-set
+++ a/kernel/exit.c
@@ -804,7 +804,6 @@ void do_exit(long code)
 
 	module_put(task_thread_info(tsk)->exec_domain->module);
 
-	proc_exit_connector(tsk);
 
 	/*
 	 * FIXME: do that only when needed, using sched_exit tracepoint
@@ -812,6 +811,7 @@ void do_exit(long code)
 	flush_ptrace_hw_breakpoint(tsk);
 
 	exit_notify(tsk, group_dead);
+	proc_exit_connector(tsk);
 #ifdef CONFIG_NUMA
 	task_lock(tsk);
 	mpol_put(tsk->mempolicy);
_

Patches currently in -mm which might be from guillaume@morinfr.org are

exitc-call-proc_exit_connector-after-exit_state-is-set.patch

Re: + exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree

[Oleg Nesterov]

> The process events connector delivers a notification when a process exits.
>  This is really convenient for a process that spawns and wants to monitor
> its children through an epoll-able() interface.
>
> Unfortunately, there is a small window between when the event is delivered
> and the child become wait()-able.
>
> This is creates a race if the parent wants to make sure that it knows
> about the exit, e.g
>
> pid_t pid = fork();
> if (pid > 0) {
> 	register_interest_for_pid(pid);
> 	if (waitpid(pid, NULL, WNOHANG) > 0)
> 	{
> 	  /* We might have raced with exit() */
> 	}

Just in case... Even with this patch the code above is still "racy" if the
child is multi-threaded. Plus it should obviously filter-out subthreads.
And afaics there is no way to make it reliable, even if you change the
code above so that waitpid() is called only after the last thread exits
WNOHANG still can fail.

Not that I am not arguing with this change. Although I hope that someone
can confirm that netlink_broadcast() is safe even if release_task(current)
was already called, so that the caller has no pids, sighand, is not visible
via /proc/, etc.

Oleg.

Re: + exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree

[Guillaume Morin]

On 25 Feb 16:10, Oleg Nesterov wrote:
> > pid_t pid = fork();
> > if (pid > 0) {
> > 	register_interest_for_pid(pid);
> > 	if (waitpid(pid, NULL, WNOHANG) > 0)
> > 	{
> > 	  /* We might have raced with exit() */
> > 	}
> 
> Just in case... Even with this patch the code above is still "racy" if the
> child is multi-threaded. Plus it should obviously filter-out subthreads.
> And afaics there is no way to make it reliable, even if you change the
> code above so that waitpid() is called only after the last thread exits
> WNOHANG still can fail.
> Not that I am not arguing with this change. Although I hope that someone
> can confirm that netlink_broadcast() is safe even if release_task(current)
> was already called, so that the caller has no pids, sighand, is not visible
> via /proc/, etc.

I was too succinct, I think.  What I am trying to do is to close a race
when a short-lived *process* dies before register_interest_for_pid()
interprets the connector message correctly, (i.e realizes this is an
exit message for a pid that the parent created).

For example, let's say that the parent has an independent thread that
just reads from the netlink socket or uses a BPF filter to see only the
events it cares about.  In that case, it's possible that the exit
connector message will be discarded (either by a reader thread or the
BPF filter) before the parent realizes it should care about messages
about a new pid (the child pid)

You clarified for me that a ptraced process is a case where this race
could still happen.  That's a good point.  Fortunately, in the case of a
short-lived process, this is not a common scenario.

If we ignore the ptrace() case, I am not sure I see the problem with
multithreaded processes.  Even if the main thread exits right away, what is
important is that:
- *either* the exit connector message of the last thread that dies is be
  seen after register_interest_for_pid completes
- *or* that waitpid(WNOHANG) succeeds right after
  register_interest_for_pid()

You seem to say it's possible for all threads to have completed
exit_notify() and sent their exit message to the connector before
register_interest_for_pid() does its job and still have waitpid(WNOHANG)
fails.  Is it correct?  If so, could you give a bit more details on how
this could happen?

My understanding is that if all threads exited before waitpid() is
called, exit->state will be set to EXIT_ZOMBIE for the pid and that
delay_group_leader() will be false (because all sub-threads have
exited), so that waitpid(WNOHANG) will successfully reap the process.
What am I missing?

Guillaume.

-- 
Guillaume Morin <guillaume@morinfr.org>

Re: + exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree

[Oleg Nesterov]

On 02/27, Guillaume Morin wrote:
>
> On 25 Feb 16:10, Oleg Nesterov wrote:
> > > pid_t pid = fork();
> > > if (pid > 0) {
> > > 	register_interest_for_pid(pid);
> > > 	if (waitpid(pid, NULL, WNOHANG) > 0)
> > > 	{
> > > 	  /* We might have raced with exit() */
> > > 	}
> >
> > Just in case... Even with this patch the code above is still "racy" if the
> > child is multi-threaded. Plus it should obviously filter-out subthreads.
> > And afaics there is no way to make it reliable, even if you change the
> > code above so that waitpid() is called only after the last thread exits
> > WNOHANG still can fail.
> > Not that I am not arguing with this change. Although I hope that someone
> > can confirm that netlink_broadcast() is safe even if release_task(current)
> > was already called, so that the caller has no pids, sighand, is not visible
> > via /proc/, etc.
>
> I was too succinct, I think.  What I am trying to do is to close a race
> when a short-lived *process* dies before register_interest_for_pid()
> interprets the connector message correctly, (i.e realizes this is an
> exit message for a pid that the parent created).

Yes, I misunderstood the changelog, thanks.

Anyway, I only tried to say that "a small window between when the event
is delivered and the child become wait()-able." is not closed by this
patch. Sorry for not being clear enough.

> You clarified for me that a ptraced process is a case where this race
> could still happen.  That's a good point.  Fortunately, in the case of a
> short-lived process, this is not a common scenario.

OK.

> You seem to say it's possible for all threads to have completed
> exit_notify() and sent their exit message to the connector before
> register_interest_for_pid() does its job and still have waitpid(WNOHANG)
> fails.  Is it correct?

And I indeed said this, but I was wrong ;) Sorry. somehow I forgot
that with this patch release_task(sub_thread) is always called before
proc_exit_connector() (and I even asked if this is safe above).

However, I still do not see how you can ensure that all threads have
already exited to rely on WNOHANG.

Nevermind. Please consider this trivial example:

	tfunc(void *)
	{
		for (;;)
			pause();
	}

	int main(void)
	{
		pthread_create(tfunc);
		pthread_exit();
	}

The main thread can exit and call proc_exit_connector() before
register_interest_for_pid(), but WNOHANG obviously can't succeed.

So I am still not sure this patch can solve the problem you described.
But let me repeat just in case: I am not arguing with this change.

Oleg.

Re: + exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree

[Guillaume Morin]

On 27 Feb 17:47, Oleg Nesterov wrote:
> Anyway, I only tried to say that "a small window between when the event
> is delivered and the child become wait()-able." is not closed by this
> patch. Sorry for not being clear enough.

Ok, thanks for it making it clear.

> Nevermind. Please consider this trivial example:
> 
> 	tfunc(void *)
> 	{
> 		for (;;)
> 			pause();
> 	}
> 
> 	int main(void)
> 	{
> 		pthread_create(tfunc);
> 		pthread_exit();
> 	}
> 
> The main thread can exit and call proc_exit_connector() before
> register_interest_for_pid(), but WNOHANG obviously can't succeed.
 
What matters is not the exit message of the main thread but the exit
message from the last threaded dying.  In your example, it's fine that
waitpid fails since the process is still around.  If you kill it, the
connector will get a connector message for the thread you created in
main().

> But let me repeat just in case: I am not arguing with this change.

That was my understanding from my message.  But I wanted to respond to
make sure I understood your points correctly.

Thanks for your help.  I appreciate it.

Guillaume.

-- 
Guillaume Morin <guillaume@morinfr.org>

Re: + exitc-call-proc_exit_connector-after-exit_state-is-set.patch added to -mm tree

[Oleg Nesterov]

On 02/27, Guillaume Morin wrote:
>
> On 27 Feb 17:47, Oleg Nesterov wrote:
>
> > Nevermind. Please consider this trivial example:
> >
> > 	tfunc(void *)
> > 	{
> > 		for (;;)
> > 			pause();
> > 	}
> >
> > 	int main(void)
> > 	{
> > 		pthread_create(tfunc);
> > 		pthread_exit();
> > 	}
> >
> > The main thread can exit and call proc_exit_connector() before
> > register_interest_for_pid(), but WNOHANG obviously can't succeed.
>
> What matters is not the exit message of the main thread but the exit
> message from the last threaded dying.  In your example, it's fine that
> waitpid fails since the process is still around.  If you kill it, the
> connector will get a connector message for the thread you created in
> main().

Yes sure. But how can you know that you should take this sub-thread
into account and this is the last thread?

OK... you can probably look at every PROC_EVENT_EXIT which has the
right ->process_tgid and use wait(WNOHANG) every time to detect the
group exit.

OK, thanks, I seem to understand now. Initially I wrongly thought
that your point is literally "tgid should me reapable right after
PROC_EVENT_EXIT with this patch".

Thanks!

Oleg.