add PR_DETACH prctl command
Subsystems (6)
RFC 1/1 add PR_DETACH prctl command
Hi.
The attched patch adds the PR_DETACH prctl command. It is needed for those rare but unfortunate cases, where you can't daemonize your process before creating a thread. The effect of this command is similar to the fork() and then exit() on parent, except that:
1. PID does not change
2. Threads are not destroyed
It would be nice to know what people think about such an approach.
fs/proc/array.c +1 -1
@@ -462,7 +462,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,/* convert nsec -> ticks */ start_time =nsec_to_clock_t (start_time); - seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \ + seq_printf(m, "%d (%s) %c %d %d %d %d %d %llu %lu \ %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \ %lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n", pid_nr_ns(pid, ns),
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +2 -1
@@ -1194,7 +1194,7 @@ struct task_struct {volatile long state;/* -1 unrunnable, 0 runnable, >0 stopped */ void *stack;atomic_t usage; -unsigned int flags;/* per process flags, defined below */ +u64 flags;/* per process flags, defined below */ unsigned int ptrace;int lock_depth;/* BKL lock depth */
@@ -1746,6 +1746,7 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *#define PF_MUTEX_TESTER 0x20000000/* Thread belongs to the rt mutex tester */ #define PF_FREEZER_SKIP 0x40000000/* Freezer should not count it as freezable */ #define PF_FREEZER_NOSIG 0x80000000/* Freezer won't send signals to it */ +#define PF_DETACH 0x100000000ULL/* Detach from parent */ /* * Only the _current_ task can read/write to tsk->flags, but other
kernel/cpu.c +1 -1
@@ -181,7 +181,7 @@ static inline void check_for_tasks(int cpu) (!cputime_eq(p->utime, cputime_zero) || !cputime_eq(p->stime, cputime_zero)))printk (KERN_WARNING "Task %s (pid = %d) is on cpu %d " -"(state = %ld, flags = %x)\n" , +"(state = %ld, flags = %llx)\n" , p->comm,task_pid_nr (p), cpu, p->state, p->flags); }
kernel/exit.c +35 -0
@@ -1507,6 +1507,38 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)return retval; } +static int wait_task_detached (struct wait_opts *wo,struct task_struct *p) +{ +int retval =0 ; +pid_t pid =task_pid_vnr (p); +uid_t uid =__task_cred (p)->uid; + +get_task_struct (p); +if (unlikely (wo->wo_flags &WNOWAIT )) { +read_unlock (&tasklist_lock); +return wait_noreap_copyout (wo, p, pid, uid,CLD_DETACHED , + p->exit_code >>8 ); + } + + p->flags &= ~PF_DETACH ; +if (!ptrace_reparented (p)) + p->parent = init_pid_ns.child_reaper; + p->real_parent = init_pid_ns.child_reaper; + p->exit_signal =SIGCHLD ; +list_move_tail (&p->sibling, &p->real_parent->children); + +read_unlock (&tasklist_lock); +if (wo->wo_stat) + retval =put_user (p->exit_code, wo->wo_stat); + +if (!retval) + retval =wait_noreap_copyout (wo, p, pid, uid,CLD_DETACHED , + p->exit_code >>8 ); +else +put_task_struct (p); +return retval; +} +/* * Consider @p for a wait by @parent. *
@@ -1549,6 +1581,9 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,if (p->exit_state ==EXIT_DEAD )return 0 ; +if (p->flags &PF_DETACH ) +return wait_task_detached (wo, p); +/* * We don't reap group leaders with subthreads. */
kernel/signal.c +3 -3
@@ -1450,10 +1450,10 @@ int do_notify_parent(struct task_struct *tsk, int sig)BUG_ON (sig ==-1 ); -/* do_notify_parent_cldstop should have been called instead. */ -BUG_ON (task_is_stopped_or_traced (tsk)); +/* do_notify_parent_cldstop should have been called instead. */ +BUG_ON (task_is_stopped_or_traced (tsk)); -BUG_ON (!task_ptrace (tsk) && +BUG_ON (!task_ptrace (tsk) && (tsk->flags &PF_EXITING ) && (tsk->group_leader != tsk || !thread_group_empty (tsk))); info.si_signo = sig;
kernel/sys.c +16 -0
@@ -1736,6 +1736,22 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : + error = -EPERM ; +/* if parent is init, or not a group leader - bail */ +if (me->real_parent == init_pid_ns.child_reaper) +break ; +if (me->group_leader != me) +break ; +if (arg2 & ~0xff ) +break ; +write_lock_irq (&tasklist_lock); + me->exit_code = arg2 <<8 ; + me->flags |=PF_DETACH ; +do_notify_parent (me, me->exit_signal); +write_unlock_irq (&tasklist_lock); + error =0 ; +break ;default : error = -EINVAL ;break ;
Quoted 4 lines · Stas Sergeev
Hi.
The attched patch adds the
PR_DETACHprctl command.
Hi. The patch doesn't look right at first glance, but to me this is not the main problem.
Quoted 9 lines
It is needed for those rare but unfortunate cases, where you can't daemonize your process before creating a thread. The effect of this command is similar to the
fork()and thenexit()on parent, except that:
1. PID does not change
2. Threads are not destroyedIt would be nice to know what people think about such an approach.
Well. You should somehow convince people we need this ;) This is the main problem.
I am not going to discuss this, I never know when it comes to the new feautures. And you need the authoritative ack, probably you can ask Linus + Roland directly.
As for the patch itself,
Quoted 4 lines
kernel/exit.c +4
-0+static int wait_task_detached (struct wait_opts *wo,struct task_struct *p) +{ +int retval =0 ; +pid_t pid =task_pid_vnr (p);
kernel/exit.c static int wait_task_continued(struct wait_opts *wo, struct task_struct *p) · line 1523
Quoted 10 lines
kernel/exit.c +10
-0+uid_t uid =__task_cred (p)->uid; + +get_task_struct (p); +if (unlikely (wo->wo_flags &WNOWAIT )) { +read_unlock (&tasklist_lock); +return wait_noreap_copyout (wo, p, pid, uid,CLD_DETACHED , + p->exit_code >>8 ); + } + + p->flags &= ~PF_DETACH ;
Only current can change its ->flags, this is racy
kernel/exit.c static int wait_task_continued(struct wait_opts *wo, struct task_struct *p) · line 1528
Quoted 5 lines
kernel/exit.c +5
-0+if (!ptrace_reparented (p)) + p->parent = init_pid_ns.child_reaper; + p->real_parent = init_pid_ns.child_reaper; + p->exit_signal =SIGCHLD ; +list_move_tail (&p->sibling, &p->real_parent->children);
No, we can't do this under read_lock(tasklist). And you forgot about threads, they also have ->real_parent == old_parent.
The usage of ->exit_code doesn't look right, espeicaily if it is traced.
And other problems afaics....
kernel/exit.c static int wait_consider_task(struct wait_opts *wo, int ptrace, · line 1585
Quoted 6 lines
kernel/exit.c +2
-0@@ -1549,6 +1581,9 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,if (p->exit_state ==EXIT_DEAD )return 0 ; +if (p->flags &PF_DETACH ) +return wait_task_detached (wo, p);
What if it is already dead? We are goint to reparent it, but init won't notice the new zombie.
And what if do_wait() was called without WEXITED? say, the old parent does waitpid(WSTOPPED).
kernel/signal.c int do_notify_parent(struct task_struct *tsk, int sig) · line 1457
Quoted 12 lines
kernel/signal.c +3
-3@@ -1450,10 +1450,10 @@ int do_notify_parent(struct task_struct *tsk, int sig)BUG_ON (sig ==-1 ); -/* do_notify_parent_cldstop should have been called instead. */ -BUG_ON (task_is_stopped_or_traced (tsk)); +/* do_notify_parent_cldstop should have been called instead. */ +BUG_ON (task_is_stopped_or_traced (tsk)); -BUG_ON (!task_ptrace (tsk) && +BUG_ON (!task_ptrace (tsk) && (tsk->flags &PF_EXITING ) && (tsk->group_leader != tsk || !thread_group_empty (tsk)));
Afaics, you are trying to hide the problem.... The code below can make tsk detached if real_parent ignores SIGCHLD.
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1742
Quoted 10 lines
kernel/sys.c +4
-0@@ -1736,6 +1736,22 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : + error = -EPERM ; +/* if parent is init, or not a group leader - bail */ +if (me->real_parent == init_pid_ns.child_reaper)
This is not exactly right. What if the child of init's sub-thread does PR_DETACH?
Oleg.
Quoted 2 lines · Oleg Nesterov
The attched patch adds the
PR_DETACHprctl command. Hi. The patch doesn't look right at first glance,
I know, thanks for the review. That's basically an RFC material.
Quoted 1 line
Well. You should somehow convince people we need this ;)
I don't have to: I need this patch, and that's the main motivation for me. :) Though of course I'll offer it for inclusion when its ready, but I am developing it mostly for my project.
google reveals that many people were confused by the fact that daemon() silently drops all the threads, and the man page says nothing about that nasty habit. And I really think there is no other way to daemonize the process with threads, than to use something like this patch, or is there?
Quoted 1 line
Only current can change its ->flags, this is racy
Oh my, add a new lock only for that? :((
Add another thread_struct member only for that?
Abuse ->exit_state only for that?
Nothing looks good...
Quoted 7 lines
kernel/exit.c static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
Quoted 5 lines
kernel/exit.c +5
-0+if (!ptrace_reparented (p)) + p->parent = init_pid_ns.child_reaper; + p->real_parent = init_pid_ns.child_reaper; + p->exit_signal =SIGCHLD ; +list_move_tail (&p->sibling,&p->real_parent->children);No, we can't do this under
read_lock(tasklist). And you forgot about threads, they also have ->real_parent ==old_parent.
Thanks, will fix.
Quoted 1 line
The usage of ->exit_code doesn't look right, espeicaily if it is traced.
Could you please elaborate on that? I am using the ->exit_code to pass the (fake) exit code to the parent. The argument of my PR_DETACH is an exit code to pass. What is a problem with that?
Quoted 5 lines
What if it is already dead? We are goint to reparent it, but init won't notice the new zombie.
And what if
do_wait()was called without WEXITED? say, the old parent does waitpid(WSTOPPED).
Will fix.
Quoted 9 lines
Quoted 9 lines
kernel/signal.c +2
-2@@ -1450,10 +1450,10 @@ int do_notify_parent(struct task_struct *tsk, int sig)BUG_ON (sig ==-1 ); -/* do_notify_parent_cldstop should have been called instead. */ -BUG_ON (task_is_stopped_or_traced (tsk)); +/* do_notify_parent_cldstop should have been called instead. */ +BUG_ON (task_is_stopped_or_traced (tsk));
Quoted 5 lines
Review int do_notify_parent(struct task_struct *tsk, int sig) · line 1457
Quoted 3 lines
(quoted diff) +1
-1- BUG_ON(!task_ptrace(tsk)&& + BUG_ON(!task_ptrace(tsk)&& (tsk->flags& PF_EXITING)&& (tsk->group_leader != tsk || !thread_group_empty(tsk)));Afaics, you are trying to hide the problem.... The code below can make tsk detached if
real_parentignores SIGCHLD.
Will fix the problem with parent ignoring SIGCHLD, thanks. Though could you please clarify whether or not you see the above hunk wrong? It is there just because the group is not empty when the leader does PR_DETACH, so I adjusted the sanity check.
Quoted 4 lines
Quoted 4 lines
kernel/sys.c +0
-0@@ -1736,6 +1736,22 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else
Quoted 8 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1742
Quoted 6 lines
kernel/sys.c +4
-0error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : + error = -EPERM ; +/* if parent is init, or not a group leader - bail */ +if (me->real_parent == init_pid_ns.child_reaper)This is not exactly right. What if the child of init's sub-thread does
PR_DETACH?
Will fix.
Thanks for your review! I'll update the patch.
Quoted 6 lines · Stas Sergeev
Quoted 1 line · Oleg Nesterov
Well. You should somehow convince people we need this ;)
I don't have to: I need this patch, and that's the main motivation for me. :)
OK,
Quoted 5 lines
Though of course I'll offer it for inclusion when its ready, but I am developing it mostly for my project.
google reveals that many people were confused by the fact thatdaemon()silently drops all the threads, and the man page says nothing about that nasty habit.
I think it does... from man daemon
This function forks, and if the fork(2) succeeds, the parent calls _exit(2),
and of course fork() doesn't clone the sub-threads.
Quoted 3 lines
And I really think there is no other
way to daemonize the process with threads, than to use something like this patch, or is there?
Depends on what "daemonize" means. Even with this patch, setsid() after PR_DETACH can fail because we do not change the pids and the caller can still be pgrp leader.
And. What if the parent of PR_DETACH caller blocks or ignores SIGCHLD or simply doesn't call do_wait()? The caller can run with PR_DETACH set without any effect "forever".
There are other problems. For example, if the caller is ptraced it can silently disappear from parent's radar. If you fix this, then the actual reparenting can be delayed unpredictably whatever the real parent does.
So, to be honest, I do not think this idea will be accepted, and I don't really understand your motivation. But once again, I never argue with the "we need this feature" requests, no need to convince me.
Quoted 5 lines
Quoted 1 line
Only current can change its ->flags, this is racy
Oh my, add a new lock only for that? :((
Add anotherthread_structmember only for that?
Abuse ->exit_state only for that?
Nothing looks good...
Yep. But this is minor, if nothing else you can use signal->flags. In fact, I do not understand why only the group leader can do prctl(PR_DETACH) and why this flag is per-thread.
Quoted 5 lines
Quoted 1 line
The usage of ->exit_code doesn't look right, espeicaily if it is traced.
Could you please elaborate on that? I am using the ->exit_code to pass the (fake) exit code to the parent. The argument of my
PR_DETACHis an exit code to pass. What is a problem with that?
The problem is that ptrace uses this ->exit_code member as well. Suppose that the (ptraced) task calls PR_DETACH and, say, recieves a signal after that. See ptrace_signal().
Quoted 14 lines
Quoted 9 lines
Quoted 9 lines
kernel/signal.c +2
-2@@ -1450,10 +1450,10 @@ int do_notify_parent(struct task_struct *tsk, int sig)BUG_ON (sig ==-1 ); -/* do_notify_parent_cldstop should have been called instead. */ -BUG_ON (task_is_stopped_or_traced (tsk)); +/* do_notify_parent_cldstop should have been called instead. */ +BUG_ON (task_is_stopped_or_traced (tsk));Quoted 5 lines
Review int do_notify_parent(struct task_struct *tsk, int sig) · line 1457
Quoted 3 lines
(quoted diff) +1
-1- BUG_ON(!task_ptrace(tsk)&& + BUG_ON(!task_ptrace(tsk)&& (tsk->flags& PF_EXITING)&& (tsk->group_leader != tsk || !thread_group_empty(tsk)));Afaics, you are trying to hide the problem.... The code below can make tsk detached if
real_parentignores SIGCHLD.
Quoted 5 lines
Will fix the problem with parent ignoring SIGCHLD, thanks. Though could you please clarify whether or not you see the above hunk wrong? It is there just because the group is not empty when the leader does
PR_DETACH, so I adjusted the sanity check.
I understand why you added PF_EXITING. And, once again, this is not right afaics. The current condition is more or less "random" and mostly historical, although correct. If we want to take PF_EXITING into account, we should just add BUG_ON(!(tsk->flags & PF_EXITING)). IOW, it is just wrong to call this function unless this tsk exits.
Oleg.
Quoted 6 lines · Oleg Nesterov
Quoted 3 lines
And I really think there is no other
way to daemonize the process with threads, than to use something like this patch, or is there?Depends on what "daemonize" means. Even with this patch,
setsid()afterPR_DETACHcan fail because we do not change the pids and the caller can still be pgrp leader.
Yes, I am using TIOCNOTTY ioctl instead.
It doesn't detach the entire group from tty though, but the plan is to implement also TIOCNOTTY_GRP, in case PR_DETACH is done.
Quoted 3 lines
And. What if the parent of
PR_DETACHcaller blocks or ignores SIGCHLD or simply doesn't calldo_wait()? The caller can run withPR_DETACHset without any effect "forever".
I am currently rewriting the patch to solve this all.
What I am trying to do now, is to reparent directly in prctl(), but delay the list_move_tail(&p->sibling, &p->real_parent->children); to the wait() call. If this is a feasible solution, I'll post the new patch.
Quoted 3 lines
So, to be honest, I do not think this idea will be accepted, and I don't really understand your motivation. But once again, I never argue with the "we need this feature" requests, no need to convince me.
Lets see if the clean implementation is possible first. :)
Quoted 3 lines
The problem is that ptrace uses this ->exit_code member as well. Suppose that the (ptraced) task calls
PR_DETACHand, say, recieves a signal after that. Seeptrace_signal().
Also do_signal_stop() seems to alter it.
Do you mean right now it can't happen that multiple events alter the exit_code, and the parent notices only the last one? In this case I need to add a separate variable.
Quoted 5 lines
I understand why you added
PF_EXITING. And, once again, this is not right afaics. The current condition is more or less "random" and mostly historical, although correct. If we want to takePF_EXITINGinto account, we should just addBUG_ON(!(tsk->flags& PF_EXITING)). IOW, it is just wrong to call this function unless this tsk exits.
OK, I'll address also this.
Thanks for your time, I am hoping to post the patch that addresses the pointed problems.
Quoted 5 lines · Stas Sergeev
I am currently rewriting the patch to solve this all.
What I am trying to do now, is to reparent directly inprctl(), but delay thelist_move_tail(&p->sibling, &p->real_parent->children); to thewait()call.
Ooh... good luck ;) but I expect this won't be simple. In fact, I do not understand how it is possible to do this correctly.
Quoted 4 lines
Quoted 3 lines
The problem is that ptrace uses this ->exit_code member as well. Suppose that the (ptraced) task calls
PR_DETACHand, say, recieves a signal after that. Seeptrace_signal().Also
do_signal_stop()seems to alter it.
Yes. It is not immediately obvious but this is in fact for ptrace. Even if this thread is not traced, the tracer can attach later.
Quoted 2 lines
Do you mean right now it can't happen that multiple events alter the
exit_code, and the parent notices only the last one?
Yes.
Quoted 1 line
In this case I need to add a separate variable.
I'd suggest you to always report 0 as the exit status. Much simpler. Then you can do another patch if you really want to report arg2.
Oleg.
Hi Oleg.
I found some time to get back to that patch and to address all of the problems you pointed. What do you think about the attached patch? I didn't expect it would became that big.
commit 1a19a1ed5f1ab86e3fb029f201383627a6b2bbd5
Author: Stas <stas@stas.(none)> Date: Thu Mar 31 19:58:17 2011 +0400
implement PR_DETACH
fs/proc/array.c +4 -3
@@ -139,9 +139,10 @@ static const char *task_state_array[] = {"t (tracing stop)" ,/* 8 */ "Z (zombie)" ,/* 16 */ "X (dead)" ,/* 32 */ -"x (dead)" ,/* 64 */ -"K (wakekill)" ,/* 128 */ - "W (waking)", /* 256 */ +"d (detached)" ,/* 64 */ +"x (dead)" ,/* 128 */ +"K (wakekill)" ,/* 256 */ + "W (waking)", /* 512 */ };static inline const char *get_task_state (struct task_struct *tsk)
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/init_task.h +2 -0
@@ -158,6 +158,8 @@ extern struct cred init_cred; .parent = &tsk, \ .children = LIST_HEAD_INIT(tsk.children), \ .sibling = LIST_HEAD_INIT(tsk.sibling), \ + .detached_children = LIST_HEAD_INIT(tsk.detached_children),\ + .detached_sibling = LIST_HEAD_INIT(tsk.detached_sibling), \ .group_leader = &tsk, \ RCU_INIT_POINTER(.real_cred, &init_cred), \ RCU_INIT_POINTER(.cred, &init_cred), \
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +16 -5
@@ -186,13 +186,14 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)/* in tsk->exit_state */ #define EXIT_ZOMBIE 16#define EXIT_DEAD 32 +#define EXIT_DETACHED 64/* in tsk->state again */ -#define TASK_DEAD 64 -#define TASK_WAKEKILL 128 -#define TASK_WAKING 256 -#define TASK_STATE_MAX 512 +#define TASK_DEAD 128 +#define TASK_WAKEKILL 256 +#define TASK_WAKING 512 +#define TASK_STATE_MAX 1024 -#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW" +#define TASK_STATE_TO_CHAR_STR "RSDTtZXdxKW"extern char ___assert_task_state[1 -2 *!!(sizeof (TASK_STATE_TO_CHAR_STR )-1 !=ilog2 (TASK_STATE_MAX )+1 )];
@@ -1260,6 +1261,8 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +int exit_flags; +int detach_code;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
@@ -1292,7 +1295,10 @@ struct task_struct {*/ struct list_head children;/* list of my children */ struct list_head sibling;/* linkage in my parent's children list */ +struct list_head detached_children;/* list of my detached children */ +struct list_head detached_sibling;/* linkage in my parent's detached children list */ struct task_struct *group_leader;/* threadgroup leader */ +int num_waiters;/* detached task may have 2 */ /* * ptraced is the list of tasks this task is using ptrace on.
@@ -1747,6 +1753,10 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *#define PF_FREEZER_SKIP 0x40000000/* Freezer should not count it as freezable */ #define PF_FREEZER_NOSIG 0x80000000/* Freezer won't send signals to it */ +/* exit flags */ +#define EF_RETCODE_READ 0x00000001/* parent read(ed) exit code */ +#define EF_DCODE_READ 0x00000002/* parent read(ed) detach code */ +/* * Only the _current_ task can read/write to tsk->flags, but other * tasks can access tsk->flags in readonly mode for example
@@ -2096,6 +2106,7 @@ extern int kill_pgrp(struct pid *pid, int sig, int priv);extern int kill_pid (struct pid *pid,int sig,int priv);extern int kill_proc_info (int ,struct siginfo *,pid_t );extern int do_notify_parent (struct task_struct *,int ); +extern int do_signal_parent (struct task_struct *,int ,int ,int );extern void __wake_up_parent (struct task_struct *p,struct task_struct *parent);extern void force_sig (int ,struct task_struct *);extern int send_sig (int ,struct task_struct *,int );
kernel/exit.c +170 -30
300 lines not on this page · raw message
kernel/fork.c +4 -0
@@ -1070,6 +1070,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,copy_flags (clone_flags, p);INIT_LIST_HEAD (&p->children);INIT_LIST_HEAD (&p->sibling); +INIT_LIST_HEAD (&p->detached_children); +INIT_LIST_HEAD (&p->detached_sibling);rcu_copy_process (p); p->vfork_done =NULL ;spin_lock_init (&p->alloc_lock);
@@ -1233,6 +1235,8 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->exit_flags =0 ; + p->num_waiters =1 ;/* * Ok, make it visible to the rest of the system.
kernel/signal.c +36 -23
91 lines not on this page · raw message
kernel/sys.c +45 -0
57 lines not on this page · raw message
Hi Stas,
Quoted 5 lines · Stas Sergeev
I found some time to get back to that patch and to address all of the problems you pointed. What do you think about the attached patch? I didn't expect it would became that big.
fs/proc/array.c | 7 - include/asm-generic/siginfo.h | 3 include/linux/init_task.h | 2 include/linux/prctl.h | 2 include/linux/sched.h | 21 +++- kernel/exit.c | 200 +++++++++++++++++++++++++++++++++++------- kernel/fork.c | 4 kernel/signal.c | 59 +++++++----- kernel/sys.c | 45 +++++++++ 9 files changed, 281 insertions(+), 62 deletions(-)
Eek! Not only it is big. It is complex and changes a lot of core kernel code.
Sorry Stas, I am not going to try to review it carefully. As I said, you need to convince lkml we need this feature first. And iirc you are not going to suggest this change for everyone.
I guess, the main complication is that you are trying to ensure the old parent can do wait() without -ECHLD... This complicates everything soooooooooo much. I _feel_ this can be simplified.... but in any case we need the nasty complications. And for what?
I only looked at sys_prctl() code, and almost every line looks wrong. Hmm... in fact, the changes in exit.c look wrong too, but I didn't really try to understand them.
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1745
Quoted 10 lines
kernel/sys.c +6
-0@@ -1736,6 +1737,50 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +struct task_struct *p, *old_parent; +int notif =DEATH_REAP ; + error = -EPERM ; +/* not detaching from init */ +if (me->real_parent == init_pid_ns.child_reaper)
2 problems. You shouldn't use init_pid_ns, you need the task's namespace. Also, the task can be the child of /sbin/init's sub-thread.
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1754
Quoted 6 lines
kernel/sys.c +6
-0+write_lock_irq (&tasklist_lock); + old_parent = me->real_parent; + me->detach_code = arg2 <<8 ; +if (!task_detached (me)) + notif =do_signal_parent (me, me->exit_signal, +CLD_DETACHED , arg2);
This is simply wrong. We reparent the whole thread group, we should always notify the old parent. Or never. but this shouldn't depend on the thread.
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1758
Quoted 4 lines
kernel/sys.c +4
-0+if (notif !=DEATH_REAP ) { +list_add_tail (&me->detached_sibling, + &me->real_parent->detached_children); + me->exit_state =EXIT_DETACHED ;
No, no, we can't set ->exit_state != 0. This means the task is dead.
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1764
Quoted 2 lines
kernel/sys.c +2
-0+if (!ptrace_reparented (me)) + me->parent = init_pid_ns.child_reaper;
Again, this shouldn't use init_pid_ns.child_reaper. But the main problem, you can't trust ptrace_reparented(). What if the old parent ptraces this task?
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1769
Quoted 2 lines
kernel/sys.c +2
-0+/* detaching makes us a group leader */ + me->group_leader = me;
How? Now, we can't change ->group_leader, this is simply not possible and very wrong. If nothing else, think about tid/tgid, but there are a lot more problems.
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1777
Quoted 6 lines
kernel/sys.c +6
-0+ while_each_thread(me, p) { +if (p->real_parent != old_parent) +continue ; +if (!ptrace_reparented (p)) + p->parent = init_pid_ns.child_reaper; + p->real_parent = init_pid_ns.child_reaper;
The same problems as above, pluse "p->real_parent != old_parent" looks bogus.
Well. Once again, I never argue with new features, but you need to convince lkml. Probably it is simple to implement PR_DETACH so that the task just "disappears" from the old_parent's radar. Otherwise we need more complications, but I'd rather add the fake TASK_ZOMBIE task_struct for that. This will be much, much simply although not pretty anyway.
Oleg.
Quoted 1 line · Oleg Nesterov
I only looked at
sys_prctl()code, and almost every line looks wrong.
Well, the lines you pointed, were also in the
previous patch, and, as you didn't complained
back then, I thought they were fine. :)
Quoted 5 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1758
Quoted 4 lines
kernel/sys.c +4
-0+if (notif !=DEATH_REAP ) { +list_add_tail (&me->detached_sibling, + &me->real_parent->detached_children); + me->exit_state =EXIT_DETACHED ;No, no, we can't set ->exit_state != 0. This means the task is dead.
Does this really break things? I mean, there are probably no checks like "if (task->exit_state)", every time the exact value is checked. And EXIT_DETACHED is only set for the short period: the wait() from parent (either new or old) will remove it.
Quoted 5 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1769
Quoted 2 lines
kernel/sys.c +2
-0+/* detaching makes us a group leader */ + me->group_leader = me;How? Now, we can't change ->group_leader, this is simply not possible and very wrong. If nothing else, think about tid/tgid, but there are a lot more problems.
OK, thats why in the previous patch I was allowing only the group leader to detach, but you complained. Should I return that back?
Quoted 8 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1777
Quoted 6 lines
kernel/sys.c +6
-0+ while_each_thread(me, p) { +if (p->real_parent != old_parent) +continue ; +if (!ptrace_reparented (p)) + p->parent = init_pid_ns.child_reaper; + p->real_parent = init_pid_ns.child_reaper;The same problems as above, pluse "
p->real_parent!=old_parent" looks bogus.
Just an extra care. Should I just remove that check?
Quoted 3 lines
Well. Once again, I never argue with new features, but you need to convince lkml. Probably it is simple to implement
PR_DETACHso that the task just "disappears" from the old_parent's radar.
Yes, that worked for me too:
https://lkml.org/lkml/2010/12/25/37
Yes, I know there are bugs too. :) But, at least the patch is just few lines.
Quoted 4 lines
Otherwise
we need more complications, but I'd rather add the fakeTASK_ZOMBIEtask_structfor that. This will be much, much simply although not pretty anyway.
Well, maybe the patch looks more complex than it actually is. How it works:
- num_waiters is set to 1 by fork(). Then PR_DETACH may increment it if the old parent does not ignore the SIGCHLD. Both the wait_task_zombie() and wait_task_detached() do decrement that counter, and when it is zero, the task is reaped. Also, if the old parent terminates without wait(), it decrements that counter, and, if needed, reaps the task.
- exit_state is set to EXIT_DETACHED if the parent doesn't ignore SIGCHLD. Then, if old parent wait()s, exit_state gets reset to 0. But if the process exits, exit_state gets set to EXIT_ZOMBIE, and then, by the use of the num_waiters counter, I make sure both parents waited, before releasing the task.
There were some rearrangements in the exit.c code, that are not directly related to the new feature. I can split them to the separate patches, if that will help.
As for convincing LKML... Well, when the code is right, maybe. :))
Quoted 6 lines · Stas Sergeev
Quoted 1 line · Oleg Nesterov
I only looked at
sys_prctl()code, and almost every line looks wrong.Well, the lines you pointed, were also in the
previous patch, and, as you didn't complained
back then, I thought they were fine. :)
Of course, I forgot completely what the previous patch did ;) Perhaps I missed them before, or perhaps I stopped looking after I noticed other problems.
Quoted 7 lines
Quoted 5 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1758
Quoted 4 lines
kernel/sys.c +4
-0+if (notif !=DEATH_REAP ) { +list_add_tail (&me->detached_sibling, + &me->real_parent->detached_children); + me->exit_state =EXIT_DETACHED ;No, no, we can't set ->exit_state != 0. This means the task is dead.
Does this really break things? I mean, there are
probably no checks like "if (task->exit_state)",
There are. zap_other_threads() for example. More importantly, we can have more of them. exit_state != 0 means: this task has passed exit_notify(), we shouldn't break this.
Quoted 8 lines
Quoted 5 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1769
Quoted 2 lines
kernel/sys.c +2
-0+/* detaching makes us a group leader */ + me->group_leader = me;How? Now, we can't change ->group_leader, this is simply not possible and very wrong. If nothing else, think about tid/tgid, but there are a lot more problems.
OK, thats why in the previous patch I was allowing only the group leader to detach, but you complained. Should I return that back?
Ah, I _seem_ to recall... Yes, it seems strange that only group leader can do PR_DETACH. If we implement this, I think any thread should be allowed to call prctl(DETACH). But why do we need to change the leader?
Quoted 7 lines
Quoted 3 lines
Well. Once again, I never argue with new features, but you need to convince lkml. Probably it is simple to implement
PR_DETACHso that the task just "disappears" from the old_parent's radar.Yes, that worked for me too:
https://lkml.org/lkml/2010/12/25/37
Yes, I know there are bugs too. :) But, at least the patch is just few lines.
I didn't look at the patch above, but probably it makes more sense. At least for the start.
Quoted 6 lines
Quoted 4 lines
Otherwise
we need more complications, but I'd rather add the fakeTASK_ZOMBIEtask_structfor that. This will be much, much simply although not pretty anyway.Well, maybe the patch looks more complex than it actually is. How it works:
I didn't mean it is very hard to understand the intent. What is really hard (at least to me) to see if it is correct or not.
And in any case it adds a lot of the code, and complicates the things.
So, just in case, I have to admit: yes, personally I dislike this new feature ;) But, again, I am not going to argue.
Quoted 3 lines
There were some rearrangements in the
exit.ccode, that are not directly related to the new feature. I can split them to the separate patches, if that will help.
Yes, this always help.
Quoted 1 line
As for convincing LKML... Well, when the code is right, maybe. :))
Maybe ;)
But, otoh. It is quite possible you will get the quick nack...
Oleg.
Quoted 2 lines · Oleg Nesterov
Quoted 1 line
+ if (
me->real_parent== init_pid_ns.child_reaper)Also, the task can be the child of /sbin/init's sub-thread.
Hmm, how to check then? Should I add the "exact_parent" just for that? Or traverse the sibling list? How bad. :(
Quoted 3 lines
There are.
zap_other_threads()for example. More importantly, we can have more of them.exit_state!= 0 means: this task has passedexit_notify(), we shouldn't break this.
OK.
Quoted 3 lines
Ah, I _seem_ to recall... Yes, it seems strange that only group leader can do
PR_DETACH. If we implement this, I think any thread should be allowed to call prctl(DETACH). But why do we need to change the leader?
OK, I didn't know it is safe to leave it unchanged.
Quoted 2 lines
I didn't look at the patch above, but probably it makes more sense. At least for the start.
IIRC strace didn't like the fact that wait() fails, and that's why I started to add the complexity.
Quoted 2 lines
I didn't mean it is very hard to understand the intent. What is really hard (at least to me) to see if it is correct or not.
OK, I'll try to break it into a small chunks then. But the fact that such a seemingly simple functionality requires so many changes, is IMHO bad by itself. And it is also non-trivial to implement mostly because of the ptrace hooks that are everywhere. Some cleanups are certainly needed in that area. :)
Quoted 2 lines
So, just in case, I have to admit: yes, personally I dislike this new feature ;)
Do you dislike a feature by itself, or by its implementation? The feature by itself does nothing but to allow the daemon() to work with threads, which can't be bad IMO, and, in some cases, the inability to do so is difficult to work around. Of course, these cases have nothing to do with the good design, but rather with the vendor-provided poorly-coded drivers and libs. In all the better cases you have the trivial workarounds, but why to search for the workarounds at all, in the first place? Why not to have the daemon() to "just work" with threads? :)
Quoted 4 lines
Quoted 1 line
As for convincing LKML... Well, when the code is right, maybe. :))
Maybe ;)
But, otoh. It is quite possible you will get the quick nack...
Well, with such a rate of additions into a thread_struct, I'll probably nack it myself soon, but again, I am surprised that such a simple task requires so many untrivial changes.
Quoted 6 lines · Stas Sergeev
Quoted 2 lines · Oleg Nesterov
Quoted 1 line
+ if (
me->real_parent== init_pid_ns.child_reaper)Also, the task can be the child of /sbin/init's sub-thread.
Hmm, how to check then? Should I add the "
exact_parent" just for that? Or traverse the sibling list? How bad. :(
You can check same_thread_group(real_parent, ns->child_reaper)
Quoted 4 lines
Quoted 2 lines
I didn't look at the patch above, but probably it makes more sense. At least for the start.
IIRC strace didn't like the fact that
wait()fails, and that's why I started to add the complexity.
Of course, the task should never escape ptrace, but this is another issue?
And yes, it is not good if the child simply disappears, but the semantics is "strange" in any case.
Quoted 3 lines
OK, I'll try to break it into a small chunks then. But the fact that such a seemingly simple functionality requires so many changes, is IMHO bad by itself.
Indeed. You are trying to do something unnatural ;)
Quoted 4 lines
And it
is also non-trivial to implement mostly because of the ptrace hooks that are everywhere. Some cleanups are certainly needed in that area. :)
Not sure ptrace is the main problem... To me, it is the problem, yes, but mostly from the semantics pov.
Quoted 3 lines
Quoted 2 lines
So, just in case, I have to admit: yes, personally I dislike this new feature ;)
Do you dislike a feature by itself, or by its implementation?
Both ;)
Quoted 2 lines
The feature by itself does nothing but to allow the
daemon()to work with threads, which can't be bad IMO,
Well. It is bad even if the patch was correct. This complicates and slows down the code. The code should be maintained. And for what?
Quoted 2 lines
Of course,
these cases have nothing to do with the good design,
Indeed. You suggest the exotic and non-portable feature, almost nobody will use it. But every user should pay for that. This is not fair.
Quoted 3 lines
but why to
search for the workarounds at all, in the first place? Why not to have thedaemon()to "just work" with threads? :)
The kernel is already huge. I simply can't understand why do you think this is good idea to add more bloat for this feature.
And this is not enough to daemonize anyway. setsid() won't work.
Stas, please do not try to convince me, you can't. OTOH, let me repeat, you do not need to convince me. I'd suggest you to discuss this with Linus. If he agrees - then we can look at your patch seriously and discuss the implementation details. Everything can be implemented.
Oleg.
Quoted 4 lines · Oleg Nesterov
Quoted 3 lines
OK, I'll try to break it into a small chunks then. But the fact that such a seemingly simple functionality requires so many changes, is IMHO bad by itself.
Indeed. You are trying to do something unnatural ;)
Well, the fact that fork/exit was always used to detach from parent, doesn't mean that it was natural: a single syscall looks more natural. :)
Quoted 3 lines
Indeed. You suggest the exotic and non-portable feature, almost nobody will use it. But every user should pay for that. This is not fair.
If that is true, then of course this should not be ever applied. But why do you think so? What gets slowed down? wait()? But if the detached_sibling list is empty, then why?
Quoted 2 lines
The kernel is already huge. I simply can't understand why do you think this is good idea to add more bloat for this feature.
That depends on the final code (if it will ever be produced). Maybe it can be very simple? :) I don't want to add bloat, I want a small and simple patch.
Quoted 1 line
And this is not enough to daemonize anyway.
setsid()won't work.
But TIOCNOTTY will.
Quoted 3 lines
Stas, please do not try to convince me, you can't. OTOH, let me repeat, you do not need to convince me. I'd suggest you to discuss this with Linus. If he agrees - then we can look at your patch
Well, if the patch is bloated or buggy, there is no reason to discuss it with Linus. :) If the patch is small and simple, that alone will give it lots of credits.
Quoted 3 lines
Hmm... Again, I didn't really read the patch... But iiuc, the detached child can exit and init can reap it, after that the old parent gets ECHLD?
Yes. Because making sure that both parents wait()ed, probably cannot be reliably implemented without the write_lock_irq(&tasklist_lock), while the current code uses only read lock. Or you really need a fake task_struct...
Quoted 3 lines
do_wait()from parent should
always work or it should always return ECHLD, but it should not depend on/dev/random. This is really weird, imho.
OK, in this case, I guess, the fake task_struct is the last chance.
Quoted 4 lines
does
list_del_init(&p->detached_sibling). This is too late. If the old parent has already calledwait_task_detached()and cleared ->detaching, this child will add the unnecessary cost to every subsequentdo_wait()call.
OK, will add the forgotten "if (->detaching)" check before calling wait_task_detached(), thanks.
Quoted 2 lines
If the old parent exits,
list_del_init()
can crash the kernel.
Why?
Yes, I can read Russian, as you asked in another e-mail. :) Let me clarify once again: I don't want to add neither bloat, nor the slowdown on the common path. But I haven't yet convinced myself that this is unavoidable. If you haven't noticed, I shrunk the patch 3 times, and fixed most of the bugs already. :)) And I am not even trying to convince you (or Linus) in anything, before the pach is made simple. If it can't be made simple, then I'll leave it to my project only. That was the intention anyway. :)
Quoted 6 lines · Oleg Nesterov
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
+ if (
me->real_parent== init_pid_ns.child_reaper)Also, the task can be the child of /sbin/init's sub-thread.
Hmm, how to check then? Should I add the "
exact_parent" just for that? Or traverse the sibling list? How bad. :(You can check
same_thread_group(real_parent, ns->child_reaper)
But real_parent==ns->child_reaper in our case, so what does this check give?
Quoted 3 lines
acked, I'd try to nack this;)
do_wait()from parent should always work or it should always return ECHLD, but it should not depend on/dev/random. This is really weird, imho.
OK, this can be fixed by delaying the wait() from init till the old parent wait()s or die. Will fix.
Hi Oleg.
Here are the splitted patches.
What do you think?
I admit that I haven't found yet the solutions to all the problems you pointed yesterday, namely to the check of "real_parent == init" and ptrace_reparented, so ignore these 2 for now.
But probably now you can have a look into the exit.c part? The first 2 patches are just the rearrangements and should not incur any functional changes. The third one is an implementation of pr_detach.
This time, the child is allowed to disappear from parent's radar in case the old parent was slow to wait(), and the process have exited and the new parent have wait()ed. This is probably fine and not worth the complications, what do you think?
Thanks for your time!
include/linux/sched.h +1 -0
@@ -2096,6 +2096,7 @@ extern int kill_pgrp(struct pid *pid, int sig, int priv);extern int kill_pid (struct pid *pid,int sig,int priv);extern int kill_proc_info (int ,struct siginfo *,pid_t );extern int do_notify_parent (struct task_struct *,int ); +extern int do_signal_parent (struct task_struct *,int ,int ,int );extern void __wake_up_parent (struct task_struct *p,struct task_struct *parent);extern void force_sig (int ,struct task_struct *);extern int send_sig (int ,struct task_struct *,int );
kernel/signal.c +36 -23
91 lines not on this page · raw message
kernel/exit.c +44 -17
98 lines not on this page · raw message
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/init_task.h +2 -0
@@ -158,6 +158,8 @@ extern struct cred init_cred; .parent = &tsk, \ .children = LIST_HEAD_INIT(tsk.children), \ .sibling = LIST_HEAD_INIT(tsk.sibling), \ + .detached_children = LIST_HEAD_INIT(tsk.detached_children),\ + .detached_sibling = LIST_HEAD_INIT(tsk.detached_sibling), \ .group_leader = &tsk, \ RCU_INIT_POINTER(.real_cred, &init_cred), \ RCU_INIT_POINTER(.cred, &init_cred), \
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +4 -0
@@ -1260,6 +1260,8 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +int detach_code; +int detaching;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
@@ -1292,6 +1294,8 @@ struct task_struct {*/ struct list_head children;/* list of my children */ struct list_head sibling;/* linkage in my parent's children list */ +struct list_head detached_children;/* list of my detached children */ +struct list_head detached_sibling;/* linkage in my parent's detached children list */ struct task_struct *group_leader;/* threadgroup leader */ /*
kernel/exit.c +50 -0
74 lines not on this page · raw message
kernel/fork.c +3 -0
@@ -1070,6 +1070,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,copy_flags (clone_flags, p);INIT_LIST_HEAD (&p->children);INIT_LIST_HEAD (&p->sibling); +INIT_LIST_HEAD (&p->detached_children); +INIT_LIST_HEAD (&p->detached_sibling);rcu_copy_process (p); p->vfork_done =NULL ;spin_lock_init (&p->alloc_lock);
@@ -1233,6 +1235,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->detaching =0 ;/* * Ok, make it visible to the rest of the system.
kernel/sys.c +37 -0
@@ -28,6 +28,7 @@#include <linux/suspend.h> #include <linux/tty.h> #include <linux/signal.h> +#include <linux/tracehook.h> #include <linux/cn_proc.h> #include <linux/getcpu.h> #include <linux/task_io_accounting_ops.h>
@@ -1736,6 +1737,42 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +struct task_struct *p; +struct pid_namespace *pid_ns =task_active_pid_ns (me); +int notif =DEATH_REAP ; + error = -EPERM ; +/* not detaching from init */ +if (me->real_parent == pid_ns->child_reaper) +break ; +if (arg2 & ~0x7f ) +break ; +write_lock_irq (&tasklist_lock); + me->detach_code = arg2 <<8 ; + notif =do_signal_parent (me, me->exit_signal, +CLD_DETACHED , arg2); +if (notif !=DEATH_REAP ) { +list_add_tail (&me->detached_sibling, + &me->real_parent->detached_children); + me->detaching =1 ; + } +if (!ptrace_reparented (me)) + me->parent = pid_ns->child_reaper; + me->real_parent = pid_ns->child_reaper; +list_move_tail (&me->sibling, + &me->real_parent->children); +/* reparent threads */ + p = me; + while_each_thread(me, p) { +if (!ptrace_reparented (p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper; + } + me->exit_signal =SIGCHLD ; +write_unlock_irq (&tasklist_lock); + error =0 ; +break ; + }default : error = -EINVAL ;break ;
Quoted 4 lines · Stas Sergeev
Hi Oleg.
This time, the child is allowed to disappear from parent's radar in case the old parent was slow to
wait(),
Hmm... Again, I didn't really read the patch... But iiuc, the detached child can exit and init can reap it, after that the old parent gets ECHLD?
You know, even if the patch was correct and this feature was acked, I'd try to nack this ;) do_wait() from parent should always work or it should always return ECHLD, but it should not depend on /dev/random. This is really weird, imho.
OK, even ignoring prctl() code he patch is buggy. __unhash_process() does list_del_init(&p->detached_sibling). This is too late. If the old parent has already called wait_task_detached() and cleared ->detaching, this child will add the unnecessary cost to every subsequent do_wait() call. If the old parent exits, list_del_init() can crash the kernel.
I didn't read the patch further.
Oleg.
Hi Oleg.
Here's the patch that addresses your concerns
about the late deleting from list.
Also, the patch is shrunk twice.
I think it is about to be trivial this time.
I still haven't solved the problems with checking
parent and checking ptrace, so ignore them for
now (or give me the hints:)
Do we still have other bugs here?
include/linux/sched.h +1 -0
@@ -2096,6 +2096,7 @@ extern int kill_pgrp(struct pid *pid, int sig, int priv);extern int kill_pid (struct pid *pid,int sig,int priv);extern int kill_proc_info (int ,struct siginfo *,pid_t );extern int do_notify_parent (struct task_struct *,int ); +extern int do_signal_parent (struct task_struct *,int ,int ,int );extern void __wake_up_parent (struct task_struct *p,struct task_struct *parent);extern void force_sig (int ,struct task_struct *);extern int send_sig (int ,struct task_struct *,int );
kernel/signal.c +36 -23
91 lines not on this page · raw message
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +2 -0
@@ -1260,6 +1260,8 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +int detach_code; +int detaching;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
kernel/exit.c +55 -1
74 lines not on this page · raw message
kernel/fork.c +1 -0
@@ -1233,6 +1233,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->detaching =0 ;/* * Ok, make it visible to the rest of the system.
kernel/sys.c +35 -0
@@ -28,6 +28,7 @@#include <linux/suspend.h> #include <linux/tty.h> #include <linux/signal.h> +#include <linux/tracehook.h> #include <linux/cn_proc.h> #include <linux/getcpu.h> #include <linux/task_io_accounting_ops.h>
@@ -1736,6 +1737,40 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +struct task_struct *p; +struct pid_namespace *pid_ns =task_active_pid_ns (me); +int notif =DEATH_REAP ; + error = -EPERM ; +/* not detaching from init */ +if (me->real_parent == pid_ns->child_reaper) +break ; +if (arg2 & ~0x7f ) +break ; +write_lock_irq (&tasklist_lock); + me->detach_code = arg2 <<8 ; + notif =do_signal_parent (me, me->exit_signal, +CLD_DETACHED , arg2); +if (notif !=DEATH_REAP ) + me->detaching =1 ; +else +list_move_tail (&me->sibling, + &me->real_parent->children); +if (!ptrace_reparented (me)) + me->parent = pid_ns->child_reaper; + me->real_parent = pid_ns->child_reaper; +/* reparent threads */ + p = me; + while_each_thread(me, p) { +if (!ptrace_reparented (p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper; + } + me->exit_signal =SIGCHLD ; +write_unlock_irq (&tasklist_lock); + error =0 ; +break ; + }default : error = -EINVAL ;break ;
Hi Stas,
Quoted 5 lines · Stas Sergeev
Here's the patch that addresses your concerns about the late deleting from list.
Also, the patch is shrunk twice.
I think it is about to be trivial this time.
But it is very wrong at first glance...
Quoted 3 lines
I still haven't solved the problems with checking parent and checking ptrace, so ignore them for now (or give me the hints:)
Not sure I understand your question...
But, once again. You should not use ptrace_reparented(). Reparanted or not, the ptraced thread must not change its ->parent.
Sorry Stas, I have no time to read the patch, just one thing...
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1758
Quoted 7 lines
kernel/sys.c +6
-0+ me->detach_code = arg2 <<8 ; +if (notif !=DEATH_REAP ) + me->detaching =1 ; +else +list_move_tail (&me->sibling, + &me->real_parent->children);
This is simply wrong unless the caller is the group_leader. Probably there was some confusion, I thought we already discussed this. But this is minor...
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1767
Quoted 4 lines
kernel/sys.c +4
-0+ while_each_thread(me, p) { +if (!ptrace_reparented (p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper;
Eek. Even ignoring ptrace, this is weird. We change parent/real_parent, but we do not do list_move_tail(sibling) until wait_task_detached() ! No, I think we should not do this even if this was correct. I'll try to nack this in any case, even if there were no immediate problems ;) IMHO, this is insane.
But this is wrong. Well. Suppose that the caller of PR_DETACH exits before the old parent does do_wait(). What /sbin/init (who is the new parent) can do after it gets SIGCHLD? If can't see this zombie. Nor the old parent can release this task due to ->detaching. Eventually /sbin/init can reap it if it does, say, waitpid(-1), but still this is wrong.
Or. Suppose that the old parent exits after its child does PR_DETACH. You changed forget_original_parent() but this is not enough, note that find_new_reaper() can pick the live sub-thread. In this case the child will be moved to init's ->children list, and after that we are changing ->real_parent back.
Oleg.
Quoted 4 lines · Oleg Nesterov
Quoted 3 lines
I still haven't solved the problems with checking
parent and checking ptrace, so ignore them for
now (or give me the hints:)Not sure I understand your question...
I wonder how to check whether the child was reparented to init, and not is a child on init's subthread. You suggested to check same_thread_group(real_parent, ns->child_reaper), but I don't understand that suggestion because in our case real_parent==ns->child_reaper, yet it is not enough, as it could be of CLONE_PARENT (I suppose).
Quoted 10 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1767
Quoted 4 lines
kernel/sys.c +4
-0+ while_each_thread(me, p) { +if (!ptrace_reparented (p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper;Eek. Even ignoring ptrace, this is weird. We change parent/real_parent, but we do not do
list_move_tail(sibling)untilwait_task_detached()! No, I think we should not do this even if this was correct. I'll try to nack this in any case, even if there were no immediate problems ;) IMHO, this is insane.
Quoted 6 lines
But this is wrong. Well. Suppose that the caller of
PR_DETACHexits before the old parent doesdo_wait(). What/sbin/init(who is the new parent) can do after it gets SIGCHLD? If can't see this zombie. Nor the old parent can release this task due to ->detaching. Eventually/sbin/initcan reap it if it does, say, waitpid(-1), but still this is wrong.
No, the idea was like that: the old parent either wait()s or exits, then init became a "real" parent of that process, and reaps it immediately. I think that's natural: the very same will happen if the child didn't do pr_detach. If it exits, its current parent have to either wait(), or exit. If it doesn't do so - zombie. I decided that if I still allow wait() for the old parent, I'd better leave that logic untouched. So: until the old parent wait()s or exits, we have zombie, and that's what I supposed to implement.
Quoted 5 lines
Or. Suppose that the old parent exits after its child does
PR_DETACH. You changedforget_original_parent()but this is not enough, note thatfind_new_reaper()can pick the live sub-thread. In this case the child will be moved to init's ->children list, and after that we are changing ->real_parent back.
How? I think I prevented that with this:
---
+ p->detaching = 0;
+ continue;
---
So it should not go down to get reparented back.
Or am I missing something?
Thanks for your time!
Quoted 9 lines · Stas Sergeev
Quoted 4 lines · Oleg Nesterov
Quoted 3 lines
I still haven't solved the problems with checking
parent and checking ptrace, so ignore them for
now (or give me the hints:)Not sure I understand your question...
I wonder how to check whether the child was
reparented to init, and not is a child on init's
subthread.
I don't understand the "and not is a child on init's subthread". If the child was created by init's sub-thread, it is the child of the whole thread group.
->real_parent points to thread, yes. But the parent is the whole process, not thread. The only reason for this oddity is __WNOTHREAD.
Quoted 2 lines
You suggested to check
same_thread_group(real_parent, ns->child_reaper),
Yes. This means: our parent is the init process.
Quoted 16 lines
Quoted 10 lines
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1767
Quoted 4 lines
kernel/sys.c +4
-0+ while_each_thread(me, p) { +if (!ptrace_reparented (p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper;Eek. Even ignoring ptrace, this is weird. We change parent/real_parent, but we do not do
list_move_tail(sibling)untilwait_task_detached()! No, I think we should not do this even if this was correct. I'll try to nack this in any case, even if there were no immediate problems ;) IMHO, this is insane.Quoted 6 lines
But this is wrong. Well. Suppose that the caller of
PR_DETACHexits before the old parent doesdo_wait(). What/sbin/init(who is the new parent) can do after it gets SIGCHLD? If can't see this zombie. Nor the old parent can release this task due to ->detaching. Eventually/sbin/initcan reap it if it does, say, waitpid(-1), but still this is wrong.
Quoted 3 lines
No, the idea was like that: the old parent either wait()s or exits, then init became a "real" parent of that process, and reaps it immediately.
I understand this, but
Quoted 1 line
I think that's natural:
I strongly disagree, this is not natural. I mean, ->real_parent and the head of ->sibling list should match each other.
For example. Ignoring other problems, you are doing list_move() in do_wait() pathes. This happens to work now because everything is protected by the global tasklist. But we are going to change the locking, it should be per-process.
Quoted 1 line
If it exits,
If it exits, it notifies the new ->real_parent == init. init receives SIGCHLD, but do_wait() can't see this process on ->children lists.
Quoted 2 lines
its current parent
have to eitherwait(), or exit. If it doesn't do so - zombie.
Indeed. And, if the old parent does wait(), this simply does list_move_tail(p->sibling), a zombie won't be reaped. And nobody will reap it, until init does waitpid(-1). This was my point, this is wrong.
kernel/exit.c static void forget_original_parent(struct task_struct *father) · line 799
Quoted 10 lines
Quoted 5 lines
Or. Suppose that the old parent exits after its child does
PR_DETACH. You changedforget_original_parent()but this is not enough, note thatfind_new_reaper()can pick the live sub-thread. In this case the child will be moved to init's ->children list, and after that we are changing ->real_parent back.How? I think I prevented that with this:
---kernel/exit.c +2
-0+ p->detaching =0 ; +continue ;
Yes, thanks, I didn't notice "continue". But then this is wrong again. This can race with wait_task_detached() called by our sub-thread, it can clear ->detaching before we check it.
Oleg.
Quoted 5 lines · Oleg Nesterov
I don't understand the "and not is a child on init's subthread". If the child was created by init's sub-thread, it is the child of the whole thread group.
->real_parent points to thread, yes.
OK, sorry, I thought you mean the CLONE_PARENT case, where the real_parent points _not_ to thread. That's why the confusion.
Quoted 2 lines
But the parent is the whole
process, not thread. The only reason for this oddity is__WNOTHREAD.
OK, now I see what problem you are pointing to.
Will fix.
Quoted 8 lines
kernel/exit.c static void forget_original_parent(struct task_struct *father) · line 799
Quoted 5 lines
How? I think I prevented that with this:
---kernel/exit.c +2
-0+ p->detaching =0 ; +continue ;Yes, thanks, I didn't notice "continue". But then this is wrong again. This can race with
wait_task_detached()called by our sub-thread, it can clear ->detaching before we check it.
But the above code is under write_lock_irq(&tasklist_lock), so why would it race?
Quoted 10 lines · Stas Sergeev
Quoted 6 lines · Oleg Nesterov
kernel/exit.c static void forget_original_parent(struct task_struct *father) · line 799
Quoted 3 lines
kernel/exit.c +2
-0+ p->detaching =0 ; +continue ;Yes, thanks, I didn't notice "continue". But then this is wrong again. This can race with
wait_task_detached()called by our sub-thread, it can clear ->detaching before we check it.But the above code is under
write_lock_irq(&tasklist_lock), so why would it race?
wait_task_detached() clears detaching, then it drops tasklist and takes it for writing. forget_original_parent() can come in between.
Oleg.
Hi Oleg, here's the patch that should address
the mentioned problems. Or does it add more? :)
I try to delay the notification of init till the detaching is complete.
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/init_task.h +2 -0
@@ -158,6 +158,8 @@ extern struct cred init_cred; .parent = &tsk, \ .children = LIST_HEAD_INIT(tsk.children), \ .sibling = LIST_HEAD_INIT(tsk.sibling), \ + .detached_children = LIST_HEAD_INIT(tsk.detached_children),\ + .detached_sibling = LIST_HEAD_INIT(tsk.detached_sibling), \ .group_leader = &tsk, \ RCU_INIT_POINTER(.real_cred, &init_cred), \ RCU_INIT_POINTER(.cred, &init_cred), \
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +4 -0
@@ -1260,6 +1260,8 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +int detach_code; +int detaching;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
@@ -1292,6 +1294,8 @@ struct task_struct {*/ struct list_head children;/* list of my children */ struct list_head sibling;/* linkage in my parent's children list */ +struct list_head detached_children;/* list of my detached children */ +struct list_head detached_sibling;/* linkage in my parent's detached children list */ struct task_struct *group_leader;/* threadgroup leader */ /*
kernel/exit.c +71 -2
109 lines not on this page · raw message
kernel/fork.c +3 -0
@@ -1070,6 +1070,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,copy_flags (clone_flags, p);INIT_LIST_HEAD (&p->children);INIT_LIST_HEAD (&p->sibling); +INIT_LIST_HEAD (&p->detached_children); +INIT_LIST_HEAD (&p->detached_sibling);rcu_copy_process (p); p->vfork_done =NULL ;spin_lock_init (&p->alloc_lock);
@@ -1233,6 +1235,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->detaching =0 ;/* * Ok, make it visible to the rest of the system.
kernel/sys.c +40 -0
@@ -28,6 +28,7 @@#include <linux/suspend.h> #include <linux/tty.h> #include <linux/signal.h> +#include <linux/tracehook.h> #include <linux/cn_proc.h> #include <linux/getcpu.h> #include <linux/task_io_accounting_ops.h>
@@ -1736,6 +1737,45 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +struct task_struct *p; +struct pid_namespace *pid_ns =task_active_pid_ns (me); +int notif =DEATH_REAP ; + error = -EPERM ; +/* not detaching from init */ +if (same_thread_group (me->real_parent, + pid_ns->child_reaper)) +break ; +if (arg2 & ~0x7f ) +break ; +write_lock_irq (&tasklist_lock); + me->detach_code = arg2 <<8 ; + notif =do_signal_parent (me, me->exit_signal, +CLD_DETACHED , arg2); +if (notif !=DEATH_REAP &&thread_group_leader (me)) { +list_add_tail (&me->detached_sibling, + &me->real_parent->detached_children); + me->detaching =1 ; + } +if (!task_ptrace (me)) + me->parent = pid_ns->child_reaper; + me->real_parent = pid_ns->child_reaper; +if (thread_group_leader (me)) { +list_move_tail (&me->sibling, + &me->real_parent->children); +/* reparent threads */ + p = me; + while_each_thread(me, p) { +if (!task_ptrace (p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper; + } + } + me->exit_signal =SIGCHLD ; +write_unlock_irq (&tasklist_lock); + error =0 ; +break ; + }default : error = -EINVAL ;break ;
Hi Oleg.
I updated patch to fix the race between wait_task_detached() and wait_task_zombie() by using is_detaching flag. Here's the patch.
What problems do remain here?
include/linux/sched.h +1 -0
@@ -2096,6 +2096,7 @@ extern int kill_pgrp(struct pid *pid, int sig, int priv);extern int kill_pid (struct pid *pid,int sig,int priv);extern int kill_proc_info (int ,struct siginfo *,pid_t );extern int do_notify_parent (struct task_struct *,int ); +extern int do_signal_parent (struct task_struct *,int ,int ,int );extern void __wake_up_parent (struct task_struct *p,struct task_struct *parent);extern void force_sig (int ,struct task_struct *);extern int send_sig (int ,struct task_struct *,int );
kernel/signal.c +36 -23
91 lines not on this page · raw message
kernel/exit.c +44 -17
98 lines not on this page · raw message
commit b05a48839018ec852ed4c7bd867534df47643d82
Author: Stas <stas@stas.(none)> Date: Thu Apr 7 12:04:19 2011 +0400
implement PR_DETACH
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/init_task.h +2 -0
@@ -158,6 +158,8 @@ extern struct cred init_cred; .parent = &tsk, \ .children = LIST_HEAD_INIT(tsk.children), \ .sibling = LIST_HEAD_INIT(tsk.sibling), \ + .detached_children = LIST_HEAD_INIT(tsk.detached_children),\ + .detached_sibling = LIST_HEAD_INIT(tsk.detached_sibling), \ .group_leader = &tsk, \ RCU_INIT_POINTER(.real_cred, &init_cred), \ RCU_INIT_POINTER(.cred, &init_cred), \
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +5 -0
@@ -1260,6 +1260,9 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +int detach_code; +int detaching; + int is_detaching:1;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
@@ -1292,6 +1295,8 @@ struct task_struct {*/ struct list_head children;/* list of my children */ struct list_head sibling;/* linkage in my parent's children list */ +struct list_head detached_children;/* list of my detached children */ +struct list_head detached_sibling;/* linkage in my parent's detached children list */ struct task_struct *group_leader;/* threadgroup leader */ /*
kernel/exit.c +73 -2
111 lines not on this page · raw message
kernel/fork.c +4 -0
@@ -1070,6 +1070,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,copy_flags (clone_flags, p);INIT_LIST_HEAD (&p->children);INIT_LIST_HEAD (&p->sibling); +INIT_LIST_HEAD (&p->detached_children); +INIT_LIST_HEAD (&p->detached_sibling);rcu_copy_process (p); p->vfork_done =NULL ;spin_lock_init (&p->alloc_lock);
@@ -1233,6 +1235,8 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->detaching =0 ; + p->is_detaching =0 ;/* * Ok, make it visible to the rest of the system.
kernel/sys.c +41 -0
53 lines not on this page · raw message
Quoted 5 lines · Stas Sergeev
I updated patch to fix the race between
wait_task_detached()andwait_task_zombie()by usingis_detachingflag. Here's the patch.
What problems do remain here?
Stas, sorry, I simply have no time to read this patch. Not to mention, you know that I dislike it in any case.
I only quickly looked at the changes in sys_prctl and they are definitely wrong.
kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 1772
Quoted 10 lines
kernel/sys.c +10
-0+ me->real_parent = pid_ns->child_reaper; +if (thread_group_leader (me)) { +list_move_tail (&me->sibling, + &me->real_parent->children); +/* reparent threads */ + p = me; + while_each_thread(me, p) { +if (!task_ptrace (p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper;
Why do you check thread_group_leader() ???? What if the caller is not the leader?
I told this many times, and I got lost. If you reparent, you should always reparent the whole group.
Also, you are doing do_signal_parent(me). I do not know what it does (once again, I didn't read the patch), but this looks suspicious. A sub-thread shouldn't notify the parent.
Oleg.
Quoted 5 lines · Oleg Nesterov
Why do you check
thread_group_leader()???? What if the caller is not the leader?I told this many times, and I got lost. If you reparent, you should always reparent the whole group.
Arrgh, OK, hope I realize that now, sorry! There were lots of other problems, so I was constantly forgetting about this one. Or, maybe, I couldn't get away with the fact that any sub-thread can reparent the whole group. OK, will definitely fix this time. :)) Thanks!
Hi Oleg.
In the attached patch I finally reparent the whole group. How does that look?
commit c95ea73afce29a9c47bab29787d6eb014a8de9e6
Author: Stas <stas@stas.(none)> Date: Mon Apr 11 15:06:33 2011 +0400
implement PR_DETACH
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/init_task.h +2 -0
@@ -158,6 +158,8 @@ extern struct cred init_cred; .parent = &tsk, \ .children = LIST_HEAD_INIT(tsk.children), \ .sibling = LIST_HEAD_INIT(tsk.sibling), \ + .detached_children = LIST_HEAD_INIT(tsk.detached_children),\ + .detached_sibling = LIST_HEAD_INIT(tsk.detached_sibling), \ .group_leader = &tsk, \ RCU_INIT_POINTER(.real_cred, &init_cred), \ RCU_INIT_POINTER(.cred, &init_cred), \
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +5 -0
@@ -1260,6 +1260,9 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +int detach_code; +int detaching; + int is_detaching:1;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
@@ -1292,6 +1295,8 @@ struct task_struct {*/ struct list_head children;/* list of my children */ struct list_head sibling;/* linkage in my parent's children list */ +struct list_head detached_children;/* list of my detached children */ +struct list_head detached_sibling;/* linkage in my parent's detached children list */ struct task_struct *group_leader;/* threadgroup leader */ /*
kernel/exit.c +73 -2
111 lines not on this page · raw message
kernel/fork.c +4 -0
@@ -1070,6 +1070,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,copy_flags (clone_flags, p);INIT_LIST_HEAD (&p->children);INIT_LIST_HEAD (&p->sibling); +INIT_LIST_HEAD (&p->detached_children); +INIT_LIST_HEAD (&p->detached_sibling);rcu_copy_process (p); p->vfork_done =NULL ;spin_lock_init (&p->alloc_lock);
@@ -1233,6 +1235,8 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->detaching =0 ; + p->is_detaching =0 ;/* * Ok, make it visible to the rest of the system.
kernel/sys.c +36 -0
@@ -28,6 +28,7 @@#include <linux/suspend.h> #include <linux/tty.h> #include <linux/signal.h> +#include <linux/tracehook.h> #include <linux/cn_proc.h> #include <linux/getcpu.h> #include <linux/task_io_accounting_ops.h>
@@ -1736,6 +1737,41 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +struct task_struct *p, *leader; +int notif; +struct pid_namespace *pid_ns =task_active_pid_ns (me); + error = -EPERM ; +/* not detaching from init */ +if (same_thread_group (me->real_parent, + pid_ns->child_reaper)) +break ; +if (arg2 & ~0x7f ) +break ; +write_lock_irq (&tasklist_lock); + leader = me->group_leader; + leader->detach_code = arg2 <<8 ; + notif =do_signal_parent (leader, leader->exit_signal, +CLD_DETACHED , arg2); +if (notif !=DEATH_REAP ) { +list_add_tail (&leader->detached_sibling, + &leader->real_parent->detached_children); + leader->detaching =1 ; + leader->is_detaching =1 ; + } + p = leader; + do { + if (!task_ptrace(p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper; + } while_each_thread(leader, p); + list_move_tail(&leader->sibling, + &leader->real_parent->children); + leader->exit_signal = SIGCHLD; + write_unlock_irq(&tasklist_lock); + error = 0; + break; + } default: error = -EINVAL; break;
Re: [path][rfc] add PR_DETACH prctl command [1/3]
In reply to Oleg NesterovReposting per Oleg's request.
The attached patch adds do_signal_parent() function. It is the same as do_notify_parent(), but can be used when the child is not terminating, but (for example) detaches with PR_DETACH.
include/linux/sched.h +1 -0
@@ -2096,6 +2096,7 @@ extern int kill_pgrp(struct pid *pid, int sig, int priv);extern int kill_pid (struct pid *pid,int sig,int priv);extern int kill_proc_info (int ,struct siginfo *,pid_t );extern int do_notify_parent (struct task_struct *,int ); +extern int do_signal_parent (struct task_struct *,int ,int ,int );extern void __wake_up_parent (struct task_struct *p,struct task_struct *parent);extern void force_sig (int ,struct task_struct *);extern int send_sig (int ,struct task_struct *,int );
kernel/signal.c +36 -23
91 lines not on this page · raw message
Re: [path][rfc] add PR_DETACH prctl command [2/3]
In reply to Oleg NesterovThe attached patch adds the can_wait_task() and can_wait_task_ptrace() functions by splitting out the checks from wait_consider_task().
int ret = wait_consider_task(wo, 0, p);
gets then replaced by
ret = can_wait_task(wo, p); if (!ret) continue; ret = wait_consider_task(wo, 0, p);
kernel/exit.c +44 -17
@@ -1507,21 +1507,11 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)return retval; } -/* -* Consider @p for a wait by @parent. -* -* -ECHILD should be in ->notask_error before the first call. -* Returns nonzero for a final return, when we have unlocked tasklist_lock. -* Returns zero if the search for a child should continue; -* then ->notask_error is 0 if @p is an eligible child, -* or another error from security_task_wait(), or still -ECHILD. -*/ -static int wait_consider_task (struct wait_opts *wo,int ptrace, -struct task_struct *p) +static int can_wait_task_common (struct wait_opts *wo,struct task_struct *p) {int ret =eligible_child (wo, p);if (!ret) -return ret; +return 0 ; ret =security_task_wait (p);if (unlikely (ret <0 )) {
@@ -1537,7 +1527,25 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,return 0 ; } -if (likely (!ptrace) &&unlikely (task_ptrace (p))) { +if (p->exit_state ==EXIT_DEAD ) +return 0 ; + +return 1 ; +} + +static int can_wait_task_ptrace (struct wait_opts *wo,struct task_struct *p) +{ +/* don't worry, gcc will optimize away this function :) */ +return can_wait_task_common (wo, p); +} + +static int can_wait_task (struct wait_opts *wo,struct task_struct *p) +{ +int ret =can_wait_task_common (wo, p); +if (!ret) +return 0 ; + +if (unlikely (task_ptrace (p))) {/* * This child is hidden by ptrace. * We aren't allowed to see it now, but eventually we will.
@@ -1546,9 +1554,21 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,return 0 ; } -if (p->exit_state ==EXIT_DEAD ) -return 0 ; +return 1 ; +} +/* +* Consider @p for a wait by @parent. +* +* -ECHILD should be in ->notask_error before the first call. +* Returns nonzero for a final return, when we have unlocked tasklist_lock. +* Returns zero if the search for a child should continue; +* then ->notask_error is 0 if @p is an eligible child, +* or another error from security_task_wait(), or still -ECHILD. +*/ +static int wait_consider_task (struct wait_opts *wo,int ptrace, +struct task_struct *p) +{/* * We don't reap group leaders with subthreads. */
@@ -1578,10 +1598,14 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,*/ static int do_wait_thread (struct wait_opts *wo,struct task_struct *tsk) { +int ret;struct task_struct *p;list_for_each_entry (p, &tsk->children, sibling) { -int ret =wait_consider_task (wo,0 , p); + ret =can_wait_task (wo, p); +if (!ret) +continue ; + ret =wait_consider_task (wo,0 , p);if (ret)return ret; }
@@ -1594,7 +1618,10 @@ static int ptrace_do_wait(struct wait_opts *wo, struct task_struct *tsk)struct task_struct *p;list_for_each_entry (p, &tsk->ptraced, ptrace_entry) { -int ret =wait_consider_task (wo,1 , p); +int ret =can_wait_task_ptrace (wo, p); +if (!ret) +continue ; + ret =wait_consider_task (wo,1 , p);if (ret)return ret; }
Re: [path][rfc] add PR_DETACH prctl command [3/3]
In reply to Oleg NesterovThe attached patch implements the PR_DETACH prctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normal wait(). If the process then exits, the notification of the new parent is delayed till the old parent does either wait() to read the detach code, or exits.
commit c95ea73afce29a9c47bab29787d6eb014a8de9e6
Author: Stas <stas@stas.(none)> Date: Mon Apr 11 15:06:33 2011 +0400
implement PR_DETACH
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/init_task.h +2 -0
@@ -158,6 +158,8 @@ extern struct cred init_cred; .parent = &tsk, \ .children = LIST_HEAD_INIT(tsk.children), \ .sibling = LIST_HEAD_INIT(tsk.sibling), \ + .detached_children = LIST_HEAD_INIT(tsk.detached_children),\ + .detached_sibling = LIST_HEAD_INIT(tsk.detached_sibling), \ .group_leader = &tsk, \ RCU_INIT_POINTER(.real_cred, &init_cred), \ RCU_INIT_POINTER(.cred, &init_cred), \
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +5 -0
@@ -1260,6 +1260,9 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +int detach_code; +int detaching; + int is_detaching:1;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
@@ -1292,6 +1295,8 @@ struct task_struct {*/ struct list_head children;/* list of my children */ struct list_head sibling;/* linkage in my parent's children list */ +struct list_head detached_children;/* list of my detached children */ +struct list_head detached_sibling;/* linkage in my parent's detached children list */ struct task_struct *group_leader;/* threadgroup leader */ /*
kernel/exit.c +73 -2
111 lines not on this page · raw message
kernel/fork.c +4 -0
@@ -1070,6 +1070,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,copy_flags (clone_flags, p);INIT_LIST_HEAD (&p->children);INIT_LIST_HEAD (&p->sibling); +INIT_LIST_HEAD (&p->detached_children); +INIT_LIST_HEAD (&p->detached_sibling);rcu_copy_process (p); p->vfork_done =NULL ;spin_lock_init (&p->alloc_lock);
@@ -1233,6 +1235,8 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->detaching =0 ; + p->is_detaching =0 ;/* * Ok, make it visible to the rest of the system.
kernel/sys.c +36 -0
@@ -28,6 +28,7 @@#include <linux/suspend.h> #include <linux/tty.h> #include <linux/signal.h> +#include <linux/tracehook.h> #include <linux/cn_proc.h> #include <linux/getcpu.h> #include <linux/task_io_accounting_ops.h>
@@ -1736,6 +1737,41 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +struct task_struct *p, *leader; +int notif; +struct pid_namespace *pid_ns =task_active_pid_ns (me); + error = -EPERM ; +/* not detaching from init */ +if (same_thread_group (me->real_parent, + pid_ns->child_reaper)) +break ; +if (arg2 & ~0x7f ) +break ; +write_lock_irq (&tasklist_lock); + leader = me->group_leader; + leader->detach_code = arg2 <<8 ; + notif =do_signal_parent (leader, leader->exit_signal, +CLD_DETACHED , arg2); +if (notif !=DEATH_REAP ) { +list_add_tail (&leader->detached_sibling, + &leader->real_parent->detached_children); + leader->detaching =1 ; + leader->is_detaching =1 ; + } + p = leader; + do { + if (!task_ptrace(p)) + p->parent = pid_ns->child_reaper; + p->real_parent = pid_ns->child_reaper; + } while_each_thread(leader, p); + list_move_tail(&leader->sibling, + &leader->real_parent->children); + leader->exit_signal = SIGCHLD; + write_unlock_irq(&tasklist_lock); + error = 0; + break; + } default: error = -EINVAL; break;
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 6 lines · On Tue, 19 Apr 2011 18:54:34 +0400 Stas Sergeev <[email protected]> wrote:
The attached patch implements the
PR_DETACHprctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normalwait(). If the process then exits, the notification of the new parent is delayed till the old parent does eitherwait()to read the detach code, or exits.
What is the use case for this and why won't existing process group interfaces do the job ?
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 8 lines · Alan Cox
Quoted 6 lines
The attached patch implements the
PR_DETACHprctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normalwait(). If the process then exits, the notification of the new parent is delayed till the old parent does eitherwait()to read the detach code, or exits.What is the use case for this and why won't existing process group interfaces do the job ?
The use case is to daemonize the process with threads. You first need to detach it from its parent, then use TIOCNOTTY ioctl to detach from the tty (TIOCNOTTY_GRP doesn't seem to exist too, though, but might be easy to implement). There are a few workarounds to that that I am aware of, but what exactly interfaces do you have in mind? I have found nothing that allows to do the same without a workarounds like this:
https://lkml.org/lkml/2011/4/8/278
The current way of detaching, which is a fork/exit combo, loses all threads, so, when you call daemon() and you had threads, you are a toast.
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 4 lines
The use case is to daemonize the process with threads. You first need to detach it from its parent, then use TIOCNOTTY ioctl to detach from the tty (
TIOCNOTTY_GRPdoesn't seem to exist too, though, but might be easy to implement).
Quoted 8 lines
There are a few workarounds to that that I am aware of, but what exactly interfaces do you have in mind? I have found nothing that allows to do the same without a workarounds like this:
https://lkml.org/lkml/2011/4/8/278
The current way of detaching, which is a fork/exit combo, loses all threads, so, when you calldaemon()and you had threads, you are a toast.
Yes - you need to detach then create the threads.
The reason I ask is you appear to add overhead to various hot paths and you add 48 bytes to each task struct if I read the code right. Thats half a megabyte on a server running a pile of java gunge !
Alan
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 14 lines · Alan Cox
Quoted 13 lines
The use case is to daemonize the process with threads. You first need to detach it from its parent, then use TIOCNOTTY ioctl to detach from the tty (
TIOCNOTTY_GRPdoesn't seem to exist too, though, but might be easy to implement). There are a few workarounds to that that I am aware of, but what exactly interfaces do you have in mind? I have found nothing that allows to do the same without a workarounds like this:
https://lkml.org/lkml/2011/4/8/278
The current way of detaching, which is a fork/exit combo, loses all threads, so, when you calldaemon()and you had threads, you are a toast.
Quoted 3 lines
Yes - you need to detach then create the threads.
The reason I ask is you appear to add overhead to various hot paths
... and complicates this code.
Quoted 3 lines
and
you add 48 bytes to each task struct if I read the code right. Thats half a megabyte on a server running a pile of java gunge !
... to add the non-portable (and _imho_ unneeded) feauture.
IOW, personally I do not like this change in any case. But I am not going to argue if someone acks this new feauture.
I'll try to check these patches from the correctness pov tomorrow, but to be honest I hope someone will nack them before I start ;)
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 3 lines · Oleg Nesterov
I'll try to check these patches from the correctness pov tomorrow, but to be honest I hope someone will nack them before I start ;)
OK, I briefly looked at 3/3. It looks certainly wrong.
notif = do_signal_parent(...);
if (notif != DEATH_REAP) {
....do_signal_parent() must not return DEATH_REAP (this means that leader->exit_signal becomes -1), but this can happen and this is bug.
I can be wrong, but iirc this was already discussed, probably when you sent the very first version which had the same problem. That is why (in particular) do_notify_parent() does
BUG_ON(tsk->group_leader != tsk || !thread_group_empty(tsk))
but you simply removed this check to hide the problem.
Also. I didn't actually read the patch yet, but iiuc: if a task T does PR_DETACH and then exits, init can't reap it until the old parent does wait. This can confuse the poor admin, he can see the zombies with ppid = 1 and there is no way to understand why.
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 11 lines · Oleg Nesterov
Quoted 2 lines
I'll try to check these patches from the correctness pov tomorrow, but to be honest I hope someone will nack them before I start ;)
OK, I briefly looked at 3/3. It looks certainly wrong.
notif = do_signal_parent(...); if (notif != DEATH_REAP) { ....
do_signal_parent()must not returnDEATH_REAP(this means thatleader->exit_signalbecomes -1), but this can happen and this is bug.
Could you please clarify this a bit: according to the comments in signal.c:
---
* We are exiting and our parent doesn't care. POSIX.1
* defines special semantics for setting SIGCHLD to SIG_IGN
* or setting the SA_NOCLDWAIT flag: we should be reaped
* automatically and not left for our parent's wait4 call.
---
That's how I understand it: if DEATH_REAP is returned, the parent ignores SIGCHILD, and in this case I am not allowing
it to read the detach code with wait(). What is the bug?
Quoted 4 lines
Also. I didn't actually read the patch yet, but iiuc: if a task T does
PR_DETACHand then exits, init can't reap it until the old parent does wait. This can confuse the poor admin, he can see the zombies with ppid = 1 and there is no way to understand why.
Oh my. :)) I guess you are not going to suggest a solution to that "problem", other than to rip the patch? :)
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 13 lines · Stas Sergeev
Quoted 11 lines · Oleg Nesterov
Quoted 2 lines
I'll try to check these patches from the correctness pov tomorrow, but to be honest I hope someone will nack them before I start ;)
OK, I briefly looked at 3/3. It looks certainly wrong.
notif = do_signal_parent(...); if (notif != DEATH_REAP) { ....
do_signal_parent()must not returnDEATH_REAP(this means thatleader->exit_signalbecomes -1), but this can happen and this is bug.
Quoted 11 lines
Could you please clarify this a bit: according to the comments in
signal.c:
---
* We are exiting and our parent doesn't care. POSIX.1
* defines special semantics for setting SIGCHLD toSIG_IGN
* or setting theSA_NOCLDWAITflag: we should be reaped
* automatically and not left for our parent's wait4 call.
---
That's how I understand it: ifDEATH_REAPis returned, the parent ignores SIGCHILD, and in this case I am not allowing
it to read the detach code withwait(). What is the bug?
Indeed. But, once again, that is why do_notify_parent() expects the dead tsk! Please note that if it returns DEATH_REAP it sets ->exit_signal = -1. And this is _only_ allowed if the leader is already dead and we are going to reap it.
Quoted 6 lines
Quoted 4 lines
Also. I didn't actually read the patch yet, but iiuc: if a task T does
PR_DETACHand then exits, init can't reap it until the old parent does wait. This can confuse the poor admin, he can see the zombies with ppid = 1 and there is no way to understand why.Oh my. :)) I guess you are not going to suggest a solution to that "problem", other than to rip the patch? :)
Cough... well, the mentioned solution looks very nice to me ;)
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 14 lines · Oleg Nesterov
Quoted 14 lines
Quoted 3 lines
do_signal_parent()must not returnDEATH_REAP(this means thatleader->exit_signalbecomes -1), but this can happen and this is bug.Could you please clarify this a bit: according to the comments in
signal.c:
---
* We are exiting and our parent doesn't care. POSIX.1
* defines special semantics for setting SIGCHLD toSIG_IGN
* or setting theSA_NOCLDWAITflag: we should be reaped
* automatically and not left for our parent's wait4 call.
---
That's how I understand it: ifDEATH_REAPis returned, the parent ignores SIGCHILD, and in this case I am not allowing
it to read the detach code withwait(). What is the bug?
Quoted 4 lines
Indeed. But, once again, that is why
do_notify_parent()expects the dead tsk! Please note that if it returnsDEATH_REAPit sets ->exit_signal = -1. And this is _only_ allowed if the leader is already dead and we are going to reap it.
Ah, so, by saying "do_signal_parent() must not return DEATH_REAP (this means that leader->exit_signal becomes -1)", you actually meant "do_signal_parent(), when returning DEATH_REAP, must not
set ->exit_signal = -1, because only do_notify_parent()
can do that"? In other words, do_signal_parent() can return DEATH_REAP after all?
If so - will fix, thanks.
Re: [path][rfc] add PR_DETACH prctl command [3/3]
Quoted 17 lines · Stas Sergeev
Quoted 14 lines · Oleg Nesterov
Quoted 14 lines
Quoted 3 lines
do_signal_parent()must not returnDEATH_REAP(this means thatleader->exit_signalbecomes -1), but this can happen and this is bug.Could you please clarify this a bit: according to the comments in
signal.c:
---
* We are exiting and our parent doesn't care. POSIX.1
* defines special semantics for setting SIGCHLD toSIG_IGN
* or setting theSA_NOCLDWAITflag: we should be reaped
* automatically and not left for our parent's wait4 call.
---
That's how I understand it: ifDEATH_REAPis returned, the parent ignores SIGCHILD, and in this case I am not allowing
it to read the detach code withwait(). What is the bug?
Quoted 8 lines
Quoted 4 lines
Indeed. But, once again, that is why
do_notify_parent()expects the dead tsk! Please note that if it returnsDEATH_REAPit sets ->exit_signal = -1. And this is _only_ allowed if the leader is already dead and we are going to reap it.Ah, so, by saying "
do_signal_parent()must not returnDEATH_REAP(this means thatleader->exit_signalbecomes -1)", you actually meant "do_signal_parent(), when returningDEATH_REAP, must not
set ->exit_signal = -1,
Yes.
Quoted 2 lines
because only
do_notify_parent()
can do that"?
because we can only do this if we are going to reap the task
Quoted 1 line
If so - will fix, thanks.
Stas, please do not trim CC. I am very glad Alan looked at this patch, I hope he will participate. Better yet, add Linus too as I already asked ;)
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [3/3]
In reply to Alan CoxQuoted 12 lines · Alan Cox
Quoted 4 lines
The use case is to daemonize the process with threads. You first need to detach it from its parent, then use TIOCNOTTY ioctl to detach from the tty (
TIOCNOTTY_GRPdoesn't seem to exist too, though, but might be easy to implement).Quoted 8 lines
There are a few workarounds to that that I am aware of, but what exactly interfaces do you have in mind? I have found nothing that allows to do the same without a workarounds like this:
https://lkml.org/lkml/2011/4/8/278
The current way of detaching, which is a fork/exit combo, loses all threads, so, when you calldaemon()and you had threads, you are a toast.
Quoted 1 line
Yes - you need to detach then create the threads.
That's painful sometimes:
https://lkml.org/lkml/2011/4/8/266
Quoted 3 lines
The reason I ask is you appear to add overhead to various hot paths and you add 48 bytes to each task struct if I read the code right. Thats half a megabyte on a server running a pile of java gunge !
I don't think there is a real code overhead: the new list is always empty (unless you detach). But the memory overhead
is real. Can be shrunk though.
Well, there is always an option to leave that patch for my own project if people dislike it too much.
But I'd like to know the margins, i.e. how much memory is acceptable? Eg, if it only uses 8 extra bytes (one pointer), is it acceptable then, or not in any case? What is the policy?
Re: [path][rfc] add PR_DETACH prctl command [1/2]
In reply to Alan CoxHi Alan, Oleg.
I changed the patch the way you asked.
It now consumes much fewer per-thread bytes (2 bytes + padding), and does much fewer suspicious things on the hot path. And no zombies with ppid=1.
The attached patch adds do_signal_parent() function. It is the same as do_notify_parent(), but can be used when the child is not terminating, but (for example) detaches with PR_DETACH.
include/linux/sched.h +1 -0
@@ -2096,6 +2096,7 @@ extern int kill_pgrp(struct pid *pid, int sig, int priv);extern int kill_pid (struct pid *pid,int sig,int priv);extern int kill_proc_info (int ,struct siginfo *,pid_t );extern int do_notify_parent (struct task_struct *,int ); +extern int do_signal_parent (struct task_struct *,int ,int ,int );extern void __wake_up_parent (struct task_struct *p,struct task_struct *parent);extern void force_sig (int ,struct task_struct *);extern int send_sig (int ,struct task_struct *,int );
kernel/signal.c +38 -23
93 lines not on this page · raw message
Re: [path][rfc] add PR_DETACH prctl command [2/2]
In reply to Alan CoxThe attached patch implements the PR_DETACH prctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normal wait().
Can be used to daemonize process with threads.
commit fede2db736225c61f3b60031e1b967b622e532af
Author: Stas <stas@stas.(none)> Date: Wed Apr 20 17:04:50 2011 +0400
implement PR_DETACH
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +3 -0
@@ -1260,6 +1260,9 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +uint8_t detach_code; +uint8_t detaching; + int pr_detached:1;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
kernel/exit.c +55 -3
88 lines not on this page · raw message
kernel/fork.c +1 -0
@@ -1233,6 +1233,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->pr_detached =0 ;/* * Ok, make it visible to the rest of the system.
kernel/signal.c +9 -0
@@ -1518,6 +1518,15 @@ int do_notify_parent(struct task_struct *tsk, int sig) {int sicode, sistatus, ret; +if (tsk->pr_detached) { +/* if parent did not read detach code, promote to zombie */ +if (tsk->detaching) +return sig; +/* otherwise terminate */ + tsk->exit_signal =-1 ; +return DEATH_REAP ; + } +BUG_ON (!task_ptrace (tsk) && (tsk->group_leader != tsk || !thread_group_empty (tsk)));
kernel/sys.c +30 -0
@@ -28,6 +28,7 @@#include <linux/suspend.h> #include <linux/tty.h> #include <linux/signal.h> +#include <linux/tracehook.h> #include <linux/cn_proc.h> #include <linux/getcpu.h> #include <linux/task_io_accounting_ops.h>
@@ -1736,6 +1737,35 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +int notif; +struct task_struct *leader; +struct pid_namespace *pid_ns; + error = -EPERM ; +if (arg2 & ~0x7f ) +break ; +write_lock_irq (&tasklist_lock); + pid_ns =task_active_pid_ns (me); + leader = me->group_leader; +if (leader->pr_detached) + goto unlock; +/* not detaching from init */ +if (same_thread_group (leader->real_parent, + pid_ns->child_reaper)) + goto unlock; + leader->detach_code = arg2; + notif =do_signal_parent (leader, leader->exit_signal, +CLD_DETACHED , arg2); +if (notif !=DEATH_REAP ) + leader->detaching =1 ; +else + leader->detaching =0 ; + leader->pr_detached =1 ; + error =0 ; +unlock: +write_unlock_irq (&tasklist_lock); +break ; + }default : error = -EINVAL ;break ;
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 6 lines · Stas Sergeev
The attached patch implements the
PR_DETACHprctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normalwait().
Can be used to daemonize process with threads.
Now I do not understand this patch at all.
Stas, I already asked you, please describe the semantics of PR_DETACH. Looking at the code, I can only see it was changed again, but I have no idea what is the supposed behaviour.
At first glance, this version does not reparent the caller to init, but still PR_DETACH checks ->real_parent != /sbin/init for unknown reason...
IIUC, PR_DETACH simply fools ->real_parent so that it think this child exits, while the child in fact runs after that but do_wait() can't see it.
Why? I don't understand the point.
And. To hide the pr_detached task from do_wait(). you changed do_notify_parent() to returnd DEATH_REAP. I guess you did this to ensure exit_notify() paths will set EXIT_DEAD and thus do_wait() can't see this child again. This looks very ugly I must admit. And not 100% correct, we notify the parent twice if ->detaching != 0. Also, because of this logic, once wait_task_detached() drops tasklist it can race with exit_notify() doing release_task(). Yes, task_struct can't go away, but we can't trust task_pid_vnr(p). Hmm, the change in reparent_leader doesn't look right, at least in case we reparent to sub-thread.
And of course, this breaks ptrace _completely_. I didn't read the patch further, perhaps there are more problems.
Stas, I am sorry, I am tired ;) You are sending more and more versions with the different semantics and they all are buggy. Please add Linus and ask him if he is going to accept the new feature, and please describe exactly what you are trying to achieve. Until then I do not want to spend my time trying to understand yet another implementation of the hack which I personally dislike.
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 8 lines · Oleg Nesterov
Quoted 5 lines
The attached patch implements the
PR_DETACHprctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normalwait().
Can be used to daemonize process with threads.At first glance, this version does not reparent the caller to init, but still
PR_DETACHchecks ->real_parent !=/sbin/initfor unknown reason...
The reason is that the task could have been reparented to ini by some other means (parent died) - in this case no detaching.
Quoted 2 lines
IIUC,
PR_DETACHsimply fools ->real_parent so that it think this child exits, while the child in fact runs after that butdo_wait()can't see it.
It can check si_code to see what actually happened.
Quoted 1 line
Why? I don't understand the point.
It was the same also when reparenting was done: the original parent was only able to read the detach code, and nothing more. _Nothing_ was changed, the semantic is completely the same! The implementation changed only.
Quoted 2 lines
And. To hide the
pr_detachedtask fromdo_wait(). you changeddo_notify_parent()to returndDEATH_REAP.
No, its hidden by the check in wait_consider_task(). do_notify_parent() was changed only to not allow the second notification to the same parent. Again, this is to completely preserve the old semantic, where's the original parent was always notified only once. So, the original parent gets only one notification on PR_DETACH, and on exit() - it does not. Nothing was changed.
Quoted 4 lines
I guess you did this to ensure
exit_notify()paths will setEXIT_DEADand thusdo_wait()can't see this child again. This looks very ugly I must admit. And not 100% correct, we notify the parent twice if ->detaching != 0.
No: the do_dignal_parent() is not called in that case too.
Quoted 2 lines
task_pid_vnr(p). Hmm, the change inreparent_leaderdoesn't look right, at least in case we reparent to sub-thread.
Why not? Whereever we reparented, I allow reparenting again. But, more importantly, I unhide that task in wait() and allow the notifications again, so without this change it can't work.
Quoted 1 line
And of course, this breaks ptrace _completely_.
Why? What exactly breaks?
Quoted 2 lines
Stas, I am sorry, I am tired ;) You are sending more and more versions with the different semantics and they all are buggy.
I am not that sure the last ones are very buggy. Of course some bugs are possible, but at least that semantic was a kind of "agreed on", and didn't change at all.
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 7 lines · Stas Sergeev
Quoted 5 lines · Oleg Nesterov
Quoted 5 lines
The attached patch implements the
PR_DETACHprctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normalwait().
Can be used to daemonize process with threads.
Quoted 5 lines
Quoted 3 lines
At first glance, this version does not reparent the caller to init, but still
PR_DETACHchecks ->real_parent !=/sbin/initfor unknown reason...The reason is that the task could have been reparented to ini by some other means (parent died)
This is clear,
- in this case no
Quoted 1 line
detaching.
Why? Now that we do not reparent PR_DETACH could works in this case too. Nevermind, this is minor and probably makes sense, forget.
I still do not understand the point of PR_DETACH. Why do you think it is needed without reparenting? OK, I do not really care ;)
Quoted 9 lines
Quoted 2 lines
IIUC,
PR_DETACHsimply fools ->real_parent so that it think this child exits, while the child in fact runs after that butdo_wait()can't see it.It can check
si_codeto see what actually happened.Quoted 1 line
Why? I don't understand the point.
It was the same also when reparenting was done: the original parent was only able to read the detach code, and nothing more. _Nothing_ was changed, the semantic is completely the same!
The same? what about, say, ppid?
Quoted 5 lines
Quoted 2 lines
And. To hide the
pr_detachedtask fromdo_wait(). you changeddo_notify_parent()to returndDEATH_REAP.No, its hidden by the check in
wait_consider_task().do_notify_parent()was changed only to not allow the second notification to the same parent.
Not only. Please look at your own code ;) wait_consider_task() checks exit_state == EXIT_ZOMBIE before p->pr_detached, and thus do_notify_parent() haas to return DEATH_REAP so that the caller will set EXIT_DEAD. Otherwise the old parent could see EXIT_ZOMBIE && pr_detached task again.
Quoted 3 lines
Again, this is
to completely preserve the old semantic, where's the original parent was always notified only once.
Of course, it should be notified only once. but afaics it can be notified twice.
Quoted 4 lines
Quoted 2 lines
This looks very ugly I must admit. And not 100% correct, we notify the parent twice if ->detaching != 0.
No: the
do_dignal_parent()is not called in that case too.
OK, I misread this code. In fact I missed something else, can't recall what I meant. Probably I was wrong anyway.
But. What if the child stops after PR_DETACH? It will notify the parent anyway, no? And this can even happen after wait(WEXITED) succeeded.
Quoted 3 lines
Quoted 2 lines
task_pid_vnr(p). Hmm, the change inreparent_leaderdoesn't look right, at least in case we reparent to sub-thread.Why not? Whereever we reparented, I allow reparenting again.
Even if the child reparents to the original parent's sub-thread?
Quoted 2 lines
But, more importantly, I unhide that task in
wait()and allow the notifications again, so without this change it can't work.
This is clear but see above.
Quoted 2 lines
Quoted 1 line
And of course, this breaks ptrace _completely_.
Why? What exactly breaks?
Everything. ptrace relies on do_wait(). wait_consider_task() doesn't work if pr_detached (except for WEXITED of course).
Quoted 3 lines
Quoted 2 lines
Stas, I am sorry, I am tired ;) You are sending more and more versions with the different semantics and they all are buggy.
I am not that sure the last ones are very buggy.
Well, I am not sure.
Quoted 2 lines
but at least that semantic
was a kind of "agreed on", and didn't change at all.
Cough. So far nobody except you agrees with this semantics ;)
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 2 lines · Oleg Nesterov
I still do not understand the point of
PR_DETACH. Why do you think it is needed without reparenting? OK, I do not really care ;)
Hmm, but that's really interesting, I wonder why do you ask this. In my eyes, the reparenting was just a "trick", which is now avoided for good. And I saved a lot of per-thread memory by throwing that, and shrunk the patch twice. Why do you think no-reparenting changed something real? Could this change any use case?
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
And. To hide the
pr_detachedtask fromdo_wait(). you changeddo_notify_parent()to returndDEATH_REAP.No, its hidden by the check in
wait_consider_task().do_notify_parent()was changed only to not allow the second notification to the same parent.Not only. Please look at your own code ;)
wait_consider_task()checksexit_state==EXIT_ZOMBIEbeforep->pr_detached, and thusdo_notify_parent()haas to returnDEATH_REAPso that the caller will setEXIT_DEAD. Otherwise the old parent could see EXIT_ZOMBIE&&pr_detachedtask again.
Yes, but that's not to hide from do_wait(). At least as far as I understand, exit_notify() does release_task() in this case, so that's not hiding: I literally terminate the child this way.
Or am I missing something?
Quoted 2 lines
But. What if the child stops after
PR_DETACH? It will notify the parent anyway, no?
Ah, so that should be disallowed too, will fix.
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
task_pid_vnr(p). Hmm, the change inreparent_leaderdoesn't look right, at least in case we reparent to sub-thread.Why not? Whereever we reparented, I allow reparenting again.
Even if the child reparents to the original parent's sub-thread?
OK. :)
Quoted 2 lines
Everything. ptrace relies on
do_wait().wait_consider_task()doesn't work ifpr_detached(except for WEXITED of course).
OK.
Quoted 2 lines
Quoted 1 line
I am not that sure the last ones are very buggy.
Well, I am not sure.
OK, at least the above 2 bugs are trivial to fix... :)) Thanks!
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 6 lines · Stas Sergeev
Quoted 2 lines · Oleg Nesterov
I still do not understand the point of
PR_DETACH. Why do you think it is needed without reparenting? OK, I do not really care ;)Hmm, but that's really interesting, I wonder why do you ask this.
Because I do not understand why do we need this feature. And I strongly dislike the complications this (wrong) patch adds.
Quoted 2 lines
In my eyes, the reparenting was just a
"trick",
Sure, I din't like the previous semantics too.
Once again, last time... No need to convince me. Please convince someone who can ack this hack authoritatively. I can't anyway. And I'm afraid nobody except Linus can decide whether we need it or not.
Quoted 7 lines
From my side - nack. What can I do if I do not agree with you?
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
And. To hide the
pr_detachedtask fromdo_wait(). you changeddo_notify_parent()to returndDEATH_REAP.No, its hidden by the check in
wait_consider_task().do_notify_parent()was changed only to not allow the second notification to the same parent.
Quoted 8 lines
Quoted 4 lines
Not only. Please look at your own code ;)
wait_consider_task()checksexit_state==EXIT_ZOMBIEbeforep->pr_detached, and thusdo_notify_parent()haas to returnDEATH_REAPso that the caller will setEXIT_DEAD. Otherwise the old parent could see EXIT_ZOMBIE&&pr_detachedtask again.Yes, but that's not to hide from
do_wait(). At least as far as I understand,exit_notify()doesrelease_task()in this case, so that's not hiding: I literally terminate the child this way.
Yes. But there is a window before release_task() does this, we should change task->state.
But I forgot where did we start... perhaps I missed something or meant something else. Perhaps I disliked the fact wait_consider_task() checks EXIT_ZOMBIE before pr_detached... Nevermind.
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 1 line · Oleg Nesterov
Once again, last time... No need to convince me.
Please, I did _never_ tried to convince you in anything. The question was only why you asked "Why do you think it is needed without reparenting?", as if without reparenting it does something different. That was a question about the semantic only.
Re: [path][rfc] add PR_DETACH prctl command [2/2]
In reply to Oleg NesterovHi Oleg.
Attaching the fixed patch.
Changes:
- in wait_task_consider(), pr_detach hides the process only in !ptrace case
- reparenting within the group is no longer re-enables detaching
- in do_notify_parent_cldstop(), do not allow notifications of real_parent (still allow for ptrace parent)
- scared by your question about reparenting, I reserved the flags argument for the future extensions. :) Reparenting may depend on a flag.
The attached patch implements the PR_DETACH prctl command. It detaches the entire process group from its parent, allowing the parent to still read the detach code with normal wait().
Can be used to daemonize process with threads.
commit 0654195220b22090ed8f0648ce7d176374932dfa
Author: Stas <stas@stas.(none)> Date: Thu Apr 21 13:48:48 2011 +0400
implement PR_DETACH
include/asm-generic/siginfo.h +2 -1
@@ -218,7 +218,8 @@ typedef struct siginfo {#define CLD_TRAPPED (__SI_CHLD|4)/* traced child has trapped */ #define CLD_STOPPED (__SI_CHLD|5)/* child has stopped */ #define CLD_CONTINUED (__SI_CHLD|6)/* stopped child has continued */ -#define NSIGCHLD 6 +#define CLD_DETACHED (__SI_CHLD|7)/* child has detached */ +#define NSIGCHLD 7/* * SIGPOLL si_codes
include/linux/prctl.h +2 -0
@@ -102,4 +102,6 @@#define PR_MCE_KILL_GET 34 +#define PR_DETACH 35 +#endif /* _LINUX_PRCTL_H */
include/linux/sched.h +3 -0
@@ -1260,6 +1260,9 @@ struct task_struct {/* task state */ int exit_state;int exit_code, exit_signal; +uint8_t detach_code; +uint8_t detaching; + int pr_detached:1;int pdeath_signal;/* The signal sent when the parent dies */ /* ??? */ unsigned int personality;
kernel/exit.c +63 -7
112 lines not on this page · raw message
kernel/fork.c +1 -0
@@ -1233,6 +1233,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, p->exit_signal = (clone_flags &CLONE_THREAD ) ?-1 : (clone_flags &CSIGNAL ); p->pdeath_signal =0 ; p->exit_state =0 ; + p->pr_detached =0 ;/* * Ok, make it visible to the rest of the system.
kernel/signal.c +12 -0
@@ -1518,6 +1518,15 @@ int do_notify_parent(struct task_struct *tsk, int sig) {int sicode, sistatus, ret; +if (tsk->pr_detached) { +/* if parent did not read detach code, promote to zombie */ +if (tsk->detaching) +return sig; +/* otherwise terminate */ + tsk->exit_signal =-1 ; +return DEATH_REAP ; + } +BUG_ON (!task_ptrace (tsk) && (tsk->group_leader != tsk || !thread_group_empty (tsk)));
@@ -1547,6 +1556,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk, int why)if (task_ptrace (tsk)) parent = tsk->parent;else { +/* pr_detached task is hidden from real_parent */ +if (tsk->pr_detached) +return ; tsk = tsk->group_leader; parent = tsk->real_parent; }
kernel/sys.c +34 -0
@@ -28,6 +28,7 @@#include <linux/suspend.h> #include <linux/tty.h> #include <linux/signal.h> +#include <linux/tracehook.h> #include <linux/cn_proc.h> #include <linux/getcpu.h> #include <linux/task_io_accounting_ops.h>
@@ -1736,6 +1737,39 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,else error =PR_MCE_KILL_DEFAULT ;break ; +case PR_DETACH : { +int notif; +struct task_struct *leader; +struct pid_namespace *pid_ns; +unsigned long flags = arg3; + error = -EPERM ; +if (arg2 & ~0x7f ) +break ; +/* reserve flags for the future extensions */ +if (flags) +break ; +write_lock_irq (&tasklist_lock); + pid_ns =task_active_pid_ns (me); + leader = me->group_leader; +if (leader->pr_detached) + goto unlock; +/* not detaching from init */ +if (same_thread_group (leader->real_parent, + pid_ns->child_reaper)) + goto unlock; + leader->detach_code = arg2; + notif =do_signal_parent (leader, leader->exit_signal, +CLD_DETACHED , arg2); +if (notif !=DEATH_REAP ) + leader->detaching =1 ; +else + leader->detaching =0 ; + leader->pr_detached =1 ; + error =0 ; +unlock: +write_unlock_irq (&tasklist_lock); +break ; + }default : error = -EINVAL ;break ;
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 7 lines · Stas Sergeev
Attaching the fixed patch.
Changes:
- inwait_task_consider(),pr_detachhides the process only in !ptrace case
...
kernel/exit.c static int wait_consider_task(struct wait_opts *wo, int ptrace, · line 1609
Quoted 10 lines
kernel/exit.c +7
-0@@ -1555,6 +1600,14 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,if (p->exit_state ==EXIT_ZOMBIE && !delay_group_leader (p))return wait_task_zombie (wo, p); +if (unlikely (p->pr_detached)) { +if (p->detaching) +return wait_task_detached (wo, p); +/* pr_detached tasks are hidden from parent */ +if (!ptrace) +return 0 ; + }
Hmm... I guess this should fix ptrace? How? I dont understand this at all.
Stas, I bet you didn't test your patch. If the old parent traces the child WEXITED should not succeed until the child really exits. Otherwise it is easy to escape ptrace.
Oh. And I bet there are other problems. Say, exec can change the leader... Easy to fix, but note that we need more and more stupid pr_detached special cases.
Stas, sorry. I am not going to looks at the next versions. Until you convince lkml we need this feature.
Oleg.
Re: [path][rfc] add PR_DETACH prctl command [2/2]
Quoted 1 line · Oleg Nesterov
Stas, I bet you didn't test your patch.
Not with ptrace, yes. :(
Quoted 1 line
If the old parent traces the child
What is "old parent" without reparenting?
Quoted 6 lines
Oh. And I bet there are other problems. Say, exec can change the leader... Easy to fix, but note that we need more and more stupid
pr_detachedspecial cases.Stas, sorry. I am not going to looks at the next versions. Until you convince lkml we need this feature.
OK, if you think there are really to much cases that I missed, then this patch might be really more intrusive than I suspected.
I got tired of it too, so fare well. :) Thanks for your time.
Quoted 10 lines · Stas Sergeev
Hi.
The attched patch adds the
PR_DETACHprctl command. It is needed for those rare but unfortunate cases, where you can't daemonize your process before creating a thread. The effect of this command is similar to thefork()and thenexit()on parent, except that:
1. PID does not change
2. Threads are not destroyed
Quoted 2 lines
It would be nice to know what people think about such an approach.
I can't comment on the patch itself, but, if your application knows it might have to daemonize after spinning up threads, why not simply fork() immediately on startup, and have the parent simply wait forever for either the child to die or for a daemonize signal from the child? If done early enough it shouldn't tie up too much memory, and it wouldn't require any of these invasive kernel changes. As an added bonus, it's portable to all unixen :)
Quoted 7 lines · Bryan Donlan
I can't comment on the patch itself, but, if your application knows it might have to daemonize after spinning up threads, why not simply
fork()immediately on startup, and have the parent simply wait forever for either the child to die or for a daemonize signal from the child? If done early enough it shouldn't tie up too much memory, and it wouldn't require any of these invasive kernel changes. As an added bonus, it's portable to all unixen :)
Yes, thats almost always true. Except when you deal with the vendor-provided poorly-coded drivers and libs, where you get the drivers initialized only via the lib. And the initialization process must be finished before the boot-up can continue, but that's not the whole story: only the process that initialized that lib, can then work with it. And of course that lib creates a dozen of threads... OK, you've got the idea. :)
But anyway. Yes, not everyone have to deal with such a nightmare, this is very rare. But people being confused by the fact that daemon() silently loses threads, are not rare. So I just wonder: even if the workaround is simple, why searching for the workaround at all? If you need 2 syscalls to detach from parent, and you loose the threads in a process, then why not to have a single call, that detaches without loosing threads?
But in any case, I am mostly inclinced to leave that patch for my project only.
2011/4/8 Stas Sergeev <[email protected]>:
Quoted 10 lines
Quoted 8 lines · Bryan Donlan
I can't comment on the patch itself, but, if your application knows it might have to daemonize after spinning up threads, why not simply
fork()immediately on startup, and have the parent simply wait forever for either the child to die or for a daemonize signal from the child? If done early enough it shouldn't tie up too much memory, and it wouldn't require any of these invasive kernel changes. As an added bonus, it's portable to all unixen :)
Quoted 8 lines
Yes, thats almost always true. Except when you deal with the vendor-provided poorly-coded drivers and libs, where you get the drivers initialized only via the lib. And the initialization process must be finished before the boot-up can continue, but that's not the whole story: only the process that initialized that lib, can then work with it. And of course that lib creates a dozen of threads... OK, you've got the idea. :)
Still, you can workaround this by either:
a) Load the vendor library via dlopen()
b) Use a separate launcher executable to handle the fork-and-wait
Either of these approaches are far simpler than patching the kernel, more portable, and much, much easier to get right. In fact, here's a quick and dirty implementation of the latter:
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
void sigusr1_handler(int unused)
{
(void)unused;
_exit(0);
}int main(int argc, char **argv)
{
int waitstatus;
pid_t child, parent = getpid();
(void)argc;
signal(SIGUSR1, sigusr1_handler);
child = fork();
if (child < 0) {
perror("Fork failed");
return EXIT_FAILURE;
}
if (child == 0) {
char envbuf[20];
sprintf(envbuf, "%d", (int)parent);
setenv("LAUNCHER_PID", envbuf, 1);
execvp(argv[1], &argv[1]);
perror("Launching child failed");
_exit(EXIT_FAILURE);
}
wait(&waitstatus);
if (WIFEXITED(waitstatus))
_exit(WEXITSTATUS(waitstatus));
_exit(256);
}Just kill(atoi(getenv("LAUNCHER_PID")), SIGUSR1) to detach. Much easier than doing some very racy things in the kernel, no? It's certainly more obvious that this ought to be correct in the face of races with its parent :)
Quoted 1 line · Bryan Donlan
Still, you can workaround this by either:
Yes, sure.
Quoted 1 line
a) Load the vendor library via
dlopen()
Too much to dlsym(), and by the way, what does this give? I _have to_ init that lib early at bootup, so I don't see how dlopen will help, could you clarify?
Quoted 1 line
b) Use a separate launcher executable to handle the fork-and-wait
Yes, that's certainly possible, except that I was trying the semaphore instead of a signal, as in your example, but its the same.
Quoted 4 lines
Just
kill(atoi(getenv("LAUNCHER_PID")), SIGUSR1)to detach. Much easier than doing some very racy things in the kernel, no? It's certainly more obvious that this ought to be correct in the face of races with its parent :)
But what races do you mean? Yes, the workaround is a workaround, and it works. And is simpler to implement. :) But what problems do you see with the PR_DETACH approach, except for the bugs in my patch? :) I mean, the fact that daemon() silently loses threads, sounds like a limitation, after all.
2011/4/8 Stas Sergeev <[email protected]>:
Quoted 11 lines
Quoted 2 lines · Bryan Donlan
Still, you can workaround this by either:
Yes, sure.
Quoted 1 line
a) Load the vendor library via
dlopen()Too much to
dlsym(), and by the way, what does this give? I_haveto_init that lib early at bootup, so I don't see how dlopen will help, could you clarify?
Using dlopen will allow you to perform the fork() prior to running the library's initialization code.
Quoted 11 lines
Quoted 4 lines
Just
kill(atoi(getenv("LAUNCHER_PID")), SIGUSR1)to detach. Much easier than doing some very racy things in the kernel, no? It's certainly more obvious that this ought to be correct in the face of races with its parent :)But what races do you mean? Yes, the workaround is a workaround, and it works. And is simpler to implement. :) But what problems do you see with the
PR_DETACHapproach, except for the bugs in my patch? :) I mean, the fact thatdaemon()silently loses threads, sounds like a limitation, after all.
As I said, I can't comment on the patch - I'm not familiar with that part of the kernel, so I don't know what kind of races may be lurking. But based on Oleg's comments, it seems clear to me that implementing your PR_DETACH is quite a complex thing to be doing. It is true that being able to daemonize() without losing threads would be a nice thing to have; I just have my doubts that it's worth the potential bugs that such a change might introduce, when it's only needed in a somewhat rare case, and when perfectly good workarounds exist in userspace.
Quoted 2 lines · Bryan Donlan
Using dlopen will allow you to perform the
fork()prior to running the library's initialization code.
But I don't even need that: I init it manually, there is no automatic ctors there.
Quoted 4 lines
As I said, I can't comment on the patch - I'm not familiar with that part of the kernel, so I don't know what kind of races may be lurking. But based on Oleg's comments, it seems clear to me that implementing your
PR_DETACHis quite a complex thing to be doing.
Yes, that's true, but treat that as my personal tour to the kernel internals. I wasn't realizing the complexity initially, and now its almost done, so I wonder what the result will look like, it will it worth all the troubles.
Quoted 5 lines
It is true that
being able todaemonize()without losing threads would be a nice thing to have; I just have my doubts that it's worth the potential bugs that such a change might introduce, when it's only needed in a somewhat rare case, and when perfectly good workarounds exist in userspace.
Yes, I share that concerns too. And it is very unlikely that I will propose that patch for inclusion, but I think it is possible to code that up small and simple, in which case maybe even Oleg will not hate it all that much. ;))