[PATCH] exit.c: call proc_exit_connector() after exit_state is set

[PATCH] exit.c: call proc_exit_connector() after exit_state is set

[Guillaume Morin]

From: Guillaume Morin <guillaume@morinfr.org>

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@gmail.com
---
diff --git a/kernel/exit.c b/kernel/exit.c
index 1e77fc6..9b0ac8c 100644
--- a/kernel/exit.c
+++ b/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);



-- 
Guillaume Morin <guillaume@morinfr.org>

Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set

[Linus Torvalds]

[ Going through old emails, adding relevant people, ie Oleg for
kernel/exit.c and David because he seems to be the go-to person for
connector issues judging by commits ]

Patch looks sane to me, and logically that exit_connector thing would
pair with exit_notify(), but I'd like some comments on it. That
usability issue/race has been there since forever afaik. We probably
should never have added that process events connector thing, but since
we did and people apparently use it..

Comments?

             Linus

On Mon, Feb 24, 2014 at 11:27 AM, Guillaume Morin <guillaume@morinfr.org> wrote:
> From: Guillaume Morin <guillaume@morinfr.org>
>
> 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@gmail.com
> ---
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 1e77fc6..9b0ac8c 100644
> --- a/kernel/exit.c
> +++ b/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);
>
>
>
> --
> Guillaume Morin <guillaume@morinfr.org>

Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set

[Oleg Nesterov]

On 03/15, Linus Torvalds wrote:
>
> [ Going through old emails, adding relevant people, ie Oleg for
> kernel/exit.c and David because he seems to be the go-to person for
> connector issues judging by commits ]
>
> Patch looks sane to me, and logically that exit_connector thing would
> pair with exit_notify(), but I'd like some comments on it.

Please see the (confusing, my fault) discussion

	http://marc.info/?t=139327886600003

But in short, personally I agree with this change too. Just it was not
clear from the changelog (to me ;) which problem this change actually
tries to solve, because in general the task is not waitable after it
reports PROC_EVENT_EXIT.

So I think the patch is fine, but let me repeat:

	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.

not that I expect this should not work, but still.

> That
> usability issue/race has been there since forever afaik. We probably
> should never have added that process events connector thing, but since
> we did and people apparently use it..

Yes... BTW, Guillaume, I forgot to mention that perhaps you can use
signalfd(SIGCHLD) instead of connector, this is epoll-able too. SIGCHLD
doesn't queue, so it can't tell you which child has exited, but WNOHANG
should work.

Oleg.

Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set

[Linus Torvalds]

On Sat, Mar 15, 2014 at 12:12 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> So I think the patch is fine, but let me repeat:
>
>         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.
>
> not that I expect this should not work, but still.

Ok. I think people use netlink_broadcast from irq context (well,
strictly speaking looks lik ebottom half) judging by the GFP_ATOMIC
users, so that had better work regardless.

> Yes... BTW, Guillaume, I forgot to mention that perhaps you can use
> signalfd(SIGCHLD) instead of connector, this is epoll-able too. SIGCHLD
> doesn't queue, so it can't tell you which child has exited, but WNOHANG
> should work.

Yeah, that sounds like a good approach if SIGCHLD is the only thing needed.

Anyway, I see that the patch is apparently in -mm, and I think your
netlink_broadcast issue should be fine, so things are moving along.
It's not like this is a new issue or even so much an outright "bug" as
a misfeature.

             Linus