Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set
On Sat, Mar 15, 2014 at 12:12 PM, Oleg Nesterov 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set
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=13932788663 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set
[ 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 wrote:
> From: Guillaume Morin
>
> 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
> Cc: matt.hels...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set
[ 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 guilla...@morinfr.org wrote:
From: Guillaume Morin guilla...@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 guilla...@morinfr.org
Cc: matt.hels...@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 guilla...@morinfr.org
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set
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=13932788663 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] exit.c: call proc_exit_connector() after exit_state is set
On Sat, Mar 15, 2014 at 12:12 PM, Oleg Nesterov o...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] exit.c: call proc_exit_connector() after exit_state is set
From: Guillaume Morin
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
Cc: matt.hels...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] exit.c: call proc_exit_connector() after exit_state is set
From: Guillaume Morin guilla...@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 guilla...@morinfr.org
Cc: matt.hels...@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 guilla...@morinfr.org
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
