[PATCH 1/1] prctl: pdeath_signal sent when parent thread (instead of parent process) dies

[PATCH 1/1] prctl: pdeath_signal sent when parent thread (instead of parent process) dies

[Filipe Brandenburger]

On a case where a child process uses prctl(PR_SET_PDEATHSIG, signo) to
request that it is sent a signal when the parent process dies, if that
child process was forked by a thread of a multi-threaded process, then
it will receive the signal when the thread terminates, when in fact it
is interested on whether the parent process died instead.

When a process is forked, it is added to the children to the task_struct
that forked it, so that will be the thread that did it. When the thread
terminates, forget_original_parent() will reparent the children by
assigning them a new reaper. In the case of a multi-threaded process,
the new reaper will be another thread in the same thread group.

The patch just adds another check to the case where pdeath_signal is set
in forget_original_parent() to verify whether the tgid of the original
and new parent process are the same and to skip sending the signal in
that particular case.

For more discussion and test cases executed:
https://bugzilla.kernel.org/show_bug.cgi?id=43300

Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
---
 kernel/exit.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 910a071..ed439cd 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -806,7 +806,8 @@ static void forget_original_parent(struct task_struct *father)
 				BUG_ON(t->ptrace);
 				t->parent = t->real_parent;
 			}
-			if (t->pdeath_signal)
+			if (t->pdeath_signal &&
+			    !same_thread_group(father, reaper))
 				group_send_sig_info(t->pdeath_signal,
 						    SEND_SIG_NOINFO, t);
 		} while_each_thread(p, t);
-- 
1.7.7.6

Re: [PATCH 1/1] prctl: pdeath_signal sent when parent thread (instead of parent process) dies

[Oleg Nesterov]

On 05/29, Filipe Brandenburger wrote:
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -806,7 +806,8 @@ static void forget_original_parent(struct task_struct *father)
>  				BUG_ON(t->ptrace);
>  				t->parent = t->real_parent;
>  			}
> -			if (t->pdeath_signal)
> +			if (t->pdeath_signal &&
> +			    !same_thread_group(father, reaper))
>  				group_send_sig_info(t->pdeath_signal,
>  						    SEND_SIG_NOINFO, t);
>  		} while_each_thread(p, t);

Filipe, you can't even imagine how much do I like this change
personally ;) Although I think that pdeath_signal code should
be moved into reparent_leader(). I suggested this many times.

But I was told there are users which depend on current behaviour,
they really want to know when the parent _thread_ exits.

Why? I have no idea. And I agree this is ugly, but we can't
break user-space.

Oleg.

Re: [PATCH 1/1] prctl: pdeath_signal sent when parent thread (instead of parent process) dies

[Filipe Brandenburger]

Hi Oleg,

On Fri, Jun 1, 2012 at 3:02 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> Filipe, you can't even imagine how much do I like this change
> personally ;) Although I think that pdeath_signal code should
> be moved into reparent_leader(). I suggested this many times.

Yes, that would really make sense!

> But I was told there are users which depend on current behaviour,
> they really want to know when the parent _thread_ exits.

Hmmm... but is it the parent thread inside the same process or the
thread of the parent process that forked it?

The former would kind of make sense to me, even though I would
probably implement it through another "option" parameter such as
PR_SET_TDEATHSIG or similar... the latter, doesn't really make any
sense to me...

> Why? I have no idea. And I agree this is ugly, but we can't
> break user-space.

At the very least, then, I think the man page for prctl(2) needs to be
updated, since PR_SET_PDEATHSIG and PR_GET_PDEATHSIG both refer
literally to the parent process.

Cheers,
Filipe

Re: [PATCH 1/1] prctl: pdeath_signal sent when parent thread (instead of parent process) dies

[Oleg Nesterov]

Hi Filipe,

On 06/01, Filipe Brandenburger wrote:
>
> On Fri, Jun 1, 2012 at 3:02 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Filipe, you can't even imagine how much do I like this change
> > personally ;) Although I think that pdeath_signal code should
> > be moved into reparent_leader(). I suggested this many times.
>
> Yes, that would really make sense!

Forgot to mention, and we should make ->pdeath_signal per-process.

> > But I was told there are users which depend on current behaviour,
> > they really want to know when the parent _thread_ exits.
>
> Hmmm... but is it the parent thread inside the same process or the
> thread of the parent process that forked it?

It is the parent thread inside the same process which forked the
child initially. Assuming you are asking about the exiting "father"
thread.

> doesn't really make any
> sense to me...

Same here ;) that is why I tried to change this logic too.

Parent/child relationship is process-wide. task->parent points to
the thread, but this is just the technical detail and it is only
needed because we have __WNOTHREAD (see do_wait).

IOW. I think we should move ->pdeath_signal from task_struct to
signal_struct, and then do

	--- x/kernel/exit.c
	+++ x/kernel/exit.c
	@@ -770,6 +770,9 @@ static void reparent_leader(struct task_
		if (same_thread_group(p->real_parent, father))
			return;
	 
	+	if (p->signal->pdeath_signal)
	+		group_send_sig_info(p->signal->pdeath_signal, SEND_SIG_NOINFO, p);
	+
		/* We don't want people slaying init.  */
		p->exit_signal = SIGCHLD;
	 
	@@ -806,9 +809,6 @@ static void forget_original_parent(struc
					BUG_ON(t->ptrace);
					t->parent = t->real_parent;
				}
	-			if (t->pdeath_signal)
	-				group_send_sig_info(t->pdeath_signal,
	-						    SEND_SIG_NOINFO, t);
			} while_each_thread(p, t);
			reparent_leader(father, p, &dead_children);
		}

Oleg.

[PATCHv2 0/1] prctl: move pdeath_signal from task_struct to signal_struct

[Filipe Brandenburger]

Hello,

This is a second version of the patch that I previously submitted under
title "prctl: pdeath_signal sent when parent thread (instead of parent
process) dies". As per the discussion on the original patch, I reworked
it to move pdeath_signal to signal_struct and make it truly per-process.

On Bugzilla #43300, there is some extra discussion and three test cases
including a transcript of their output running on the unpatched kernel
and on the patched kernel. The three test cases check three corner cases
where the current per-task pdeath_signal shows some behavior that is
counter intuitive and goes against what is in the documentation (the
prctl(2) manpage states that PR_SET_PDEATHSIG sets "the parent process
death signal of the calling process" which makes it clear that it is
supposed to be per-process.)

A link to the Bugzilla report:
https://bugzilla.kernel.org/show_bug.cgi?id=43300

Thanks,
Filipe


Filipe Brandenburger (1):
  prctl: move pdeath_signal from task_struct to signal_struct

 fs/exec.c                  |    2 +-
 include/linux/sched.h      |    7 ++++++-
 kernel/cred.c              |    2 +-
 kernel/exit.c              |    8 +++++---
 kernel/fork.c              |    1 -
 kernel/sys.c               |    5 +++--
 security/apparmor/domain.c |    2 +-
 security/selinux/hooks.c   |    2 +-
 security/smack/smack_lsm.c |    2 +-
 9 files changed, 19 insertions(+), 12 deletions(-)

-- 
1.7.7.6

[PATCHv2 1/1] prctl: move pdeath_signal from task_struct to signal_struct

[Filipe Brandenburger]

There are some cases involving multi-threaded processes where pdeath_signal
(which can be set using prctl(PR_SET_PDEATHSIG, signo)) doesn't act correctly
in respect of processes vs. threads. In particular:

1) If a child process is forked from a thread of the parent process and the
child process uses prctl(PR_SET_PDEATHSIG, signo) to set pdeath_signal, then
it will receive a signal when the thread that forked it terminates instead
of the parent process itself.

2) When setting prctl(PR_SET_PDEATHSIG, signo) from a thread of the child
process, if the thread terminates before the parent process then pdeath_signal
will be cancelled as it was associated with that task only.

3) When setting prctl(PR_SET_PDEATHSIG, signo) from a thread of a process and
then querying it with prctl(PR_GET_PDEATHSIG, &signo) from another thread it
will return 0 as no pdeath_signal is not set for that task.

Documentation clearly states that PR_SET_PDEATHSIG and PR_GET_PDEATHSIG should
act on processes, in particular case #1 is very counter intuitive because the
child process should not care whether the parent is multi-threaded or not.

This patch moves pdeath_signal from task_struct to signal_struct in order to
make it effectively per-process even on multi-threaded processes.

Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
---
 fs/exec.c                  |    2 +-
 include/linux/sched.h      |    7 ++++++-
 kernel/cred.c              |    2 +-
 kernel/exit.c              |    8 +++++---
 kernel/fork.c              |    1 -
 kernel/sys.c               |    5 +++--
 security/apparmor/domain.c |    2 +-
 security/selinux/hooks.c   |    2 +-
 security/smack/smack_lsm.c |    2 +-
 9 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index a79786a..67ce5ee 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1151,7 +1151,7 @@ void setup_new_exec(struct linux_binprm * bprm)
 	/* install the new credentials */
 	if (!uid_eq(bprm->cred->uid, current_euid()) ||
 	    !gid_eq(bprm->cred->gid, current_egid())) {
-		current->pdeath_signal = 0;
+		current->signal->pdeath_signal = 0;
 	} else {
 		would_dump(bprm, bprm->file);
 		if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f34437e..de22765 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -554,6 +554,12 @@ struct signal_struct {
 	unsigned int		flags; /* see SIGNAL_* flags below */
 
 	/*
+	 * Support for PR_SET_PDEATHSIG.
+	 * If non-zero, it contains the signal sent when the parent dies.
+	 */
+	int pdeath_signal;
+
+	/*
 	 * PR_SET_CHILD_SUBREAPER marks a process, like a service
 	 * manager, to re-parent orphan (double-forking) child processes
 	 * to this process instead of 'init'. The service manager is
@@ -1285,7 +1291,6 @@ struct task_struct {
 /* task state */
 	int exit_state;
 	int exit_code, exit_signal;
-	int pdeath_signal;  /*  The signal sent when the parent dies  */
 	unsigned int jobctl;	/* JOBCTL_*, siglock protected */
 	/* ??? */
 	unsigned int personality;
diff --git a/kernel/cred.c b/kernel/cred.c
index de728ac..128f46e 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -496,7 +496,7 @@ int commit_creds(struct cred *new)
 	    !cap_issubset(new->cap_permitted, old->cap_permitted)) {
 		if (task->mm)
 			set_dumpable(task->mm, suid_dumpable);
-		task->pdeath_signal = 0;
+		task->signal->pdeath_signal = 0;
 		smp_wmb();
 	}
 
diff --git a/kernel/exit.c b/kernel/exit.c
index 34867cc..bcb471e 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -770,6 +770,11 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p,
 	if (same_thread_group(p->real_parent, father))
 		return;
 
+	/* Check if prctl(PR_SET_PDEATHSIG, ...) was set for this process. */
+	if (p->signal->pdeath_signal)
+		group_send_sig_info(p->signal->pdeath_signal,
+				    SEND_SIG_NOINFO, p);
+
 	/* We don't want people slaying init.  */
 	p->exit_signal = SIGCHLD;
 
@@ -806,9 +811,6 @@ static void forget_original_parent(struct task_struct *father)
 				BUG_ON(t->ptrace);
 				t->parent = t->real_parent;
 			}
-			if (t->pdeath_signal)
-				group_send_sig_info(t->pdeath_signal,
-						    SEND_SIG_NOINFO, t);
 		} while_each_thread(p, t);
 		reparent_leader(father, p, &dead_children);
 	}
diff --git a/kernel/fork.c b/kernel/fork.c
index ab5211b..4c9f9b8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1402,7 +1402,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	else
 		p->exit_signal = (clone_flags & CSIGNAL);
 
-	p->pdeath_signal = 0;
 	p->exit_state = 0;
 
 	p->nr_dirtied = 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 9ff89cb..fd9ee49 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2007,11 +2007,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 				error = -EINVAL;
 				break;
 			}
-			me->pdeath_signal = arg2;
+			me->signal->pdeath_signal = arg2;
 			error = 0;
 			break;
 		case PR_GET_PDEATHSIG:
-			error = put_user(me->pdeath_signal, (int __user *)arg2);
+			error = put_user(me->signal->pdeath_signal,
+					 (int __user *)arg2);
 			break;
 		case PR_GET_DUMPABLE:
 			error = get_dumpable(me->mm);
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index b81ea10..0eac0d7 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -564,7 +564,7 @@ void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
 	    (unconfined(new_cxt->profile)))
 		return;
 
-	current->pdeath_signal = 0;
+	current->signal->pdeath_signal = 0;
 
 	/* reset soft limits and set hard limits for the new profile */
 	__aa_transition_rlimits(profile, new_cxt->profile);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 372ec65..e839600 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2195,7 +2195,7 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
 	flush_unauthorized_files(bprm->cred, current->files);
 
 	/* Always clear parent death signal on SID transitions. */
-	current->pdeath_signal = 0;
+	current->signal->pdeath_signal = 0;
 
 	/* Check whether the new SID can inherit resource limits from the old
 	 * SID.  If not, reset all soft limits to the lower of the current
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ee0bb57..3c85790 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -496,7 +496,7 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
 	struct task_smack *bsp = bprm->cred->security;
 
 	if (bsp->smk_task != bsp->smk_forked)
-		current->pdeath_signal = 0;
+		current->signal->pdeath_signal = 0;
 }
 
 /**
-- 
1.7.7.6

Re: [PATCHv2 1/1] prctl: move pdeath_signal from task_struct to signal_struct

[Oleg Nesterov]

This was already discussed at least twice. Personally I like this change,
the current pdeath_signal logic doesn't look sane imho. I guess it simply
was not fixed when the kernel started to support threads.

>From the correctness pov,
Reviewed-by: Oleg Nesterov <oleg@redhat.com>

However. This code is very, very old. I simply do not know if we can
change the current behaviour or not. In particular, Albert Cahalan
didn't like this change when it was last discussed because it can
break his application.

Filipe, I think you need to convince Linus ;)

On 06/11, Filipe Brandenburger wrote:
>
> There are some cases involving multi-threaded processes where pdeath_signal
> (which can be set using prctl(PR_SET_PDEATHSIG, signo)) doesn't act correctly
> in respect of processes vs. threads. In particular:
>
> 1) If a child process is forked from a thread of the parent process and the
> child process uses prctl(PR_SET_PDEATHSIG, signo) to set pdeath_signal, then
> it will receive a signal when the thread that forked it terminates instead
> of the parent process itself.
>
> 2) When setting prctl(PR_SET_PDEATHSIG, signo) from a thread of the child
> process, if the thread terminates before the parent process then pdeath_signal
> will be cancelled as it was associated with that task only.
>
> 3) When setting prctl(PR_SET_PDEATHSIG, signo) from a thread of a process and
> then querying it with prctl(PR_GET_PDEATHSIG, &signo) from another thread it
> will return 0 as no pdeath_signal is not set for that task.
>
> Documentation clearly states that PR_SET_PDEATHSIG and PR_GET_PDEATHSIG should
> act on processes, in particular case #1 is very counter intuitive because the
> child process should not care whether the parent is multi-threaded or not.
>
> This patch moves pdeath_signal from task_struct to signal_struct in order to
> make it effectively per-process even on multi-threaded processes.
>
> Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
> ---
>  fs/exec.c                  |    2 +-
>  include/linux/sched.h      |    7 ++++++-
>  kernel/cred.c              |    2 +-
>  kernel/exit.c              |    8 +++++---
>  kernel/fork.c              |    1 -
>  kernel/sys.c               |    5 +++--
>  security/apparmor/domain.c |    2 +-
>  security/selinux/hooks.c   |    2 +-
>  security/smack/smack_lsm.c |    2 +-
>  9 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index a79786a..67ce5ee 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1151,7 +1151,7 @@ void setup_new_exec(struct linux_binprm * bprm)
>  	/* install the new credentials */
>  	if (!uid_eq(bprm->cred->uid, current_euid()) ||
>  	    !gid_eq(bprm->cred->gid, current_egid())) {
> -		current->pdeath_signal = 0;
> +		current->signal->pdeath_signal = 0;
>  	} else {
>  		would_dump(bprm, bprm->file);
>  		if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index f34437e..de22765 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -554,6 +554,12 @@ struct signal_struct {
>  	unsigned int		flags; /* see SIGNAL_* flags below */
>
>  	/*
> +	 * Support for PR_SET_PDEATHSIG.
> +	 * If non-zero, it contains the signal sent when the parent dies.
> +	 */
> +	int pdeath_signal;
> +
> +	/*
>  	 * PR_SET_CHILD_SUBREAPER marks a process, like a service
>  	 * manager, to re-parent orphan (double-forking) child processes
>  	 * to this process instead of 'init'. The service manager is
> @@ -1285,7 +1291,6 @@ struct task_struct {
>  /* task state */
>  	int exit_state;
>  	int exit_code, exit_signal;
> -	int pdeath_signal;  /*  The signal sent when the parent dies  */
>  	unsigned int jobctl;	/* JOBCTL_*, siglock protected */
>  	/* ??? */
>  	unsigned int personality;
> diff --git a/kernel/cred.c b/kernel/cred.c
> index de728ac..128f46e 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -496,7 +496,7 @@ int commit_creds(struct cred *new)
>  	    !cap_issubset(new->cap_permitted, old->cap_permitted)) {
>  		if (task->mm)
>  			set_dumpable(task->mm, suid_dumpable);
> -		task->pdeath_signal = 0;
> +		task->signal->pdeath_signal = 0;
>  		smp_wmb();
>  	}
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 34867cc..bcb471e 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -770,6 +770,11 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p,
>  	if (same_thread_group(p->real_parent, father))
>  		return;
>
> +	/* Check if prctl(PR_SET_PDEATHSIG, ...) was set for this process. */
> +	if (p->signal->pdeath_signal)
> +		group_send_sig_info(p->signal->pdeath_signal,
> +				    SEND_SIG_NOINFO, p);
> +
>  	/* We don't want people slaying init.  */
>  	p->exit_signal = SIGCHLD;
>
> @@ -806,9 +811,6 @@ static void forget_original_parent(struct task_struct *father)
>  				BUG_ON(t->ptrace);
>  				t->parent = t->real_parent;
>  			}
> -			if (t->pdeath_signal)
> -				group_send_sig_info(t->pdeath_signal,
> -						    SEND_SIG_NOINFO, t);
>  		} while_each_thread(p, t);
>  		reparent_leader(father, p, &dead_children);
>  	}
> diff --git a/kernel/fork.c b/kernel/fork.c
> index ab5211b..4c9f9b8 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1402,7 +1402,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	else
>  		p->exit_signal = (clone_flags & CSIGNAL);
>
> -	p->pdeath_signal = 0;
>  	p->exit_state = 0;
>
>  	p->nr_dirtied = 0;
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 9ff89cb..fd9ee49 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2007,11 +2007,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  				error = -EINVAL;
>  				break;
>  			}
> -			me->pdeath_signal = arg2;
> +			me->signal->pdeath_signal = arg2;
>  			error = 0;
>  			break;
>  		case PR_GET_PDEATHSIG:
> -			error = put_user(me->pdeath_signal, (int __user *)arg2);
> +			error = put_user(me->signal->pdeath_signal,
> +					 (int __user *)arg2);
>  			break;
>  		case PR_GET_DUMPABLE:
>  			error = get_dumpable(me->mm);
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index b81ea10..0eac0d7 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -564,7 +564,7 @@ void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
>  	    (unconfined(new_cxt->profile)))
>  		return;
>
> -	current->pdeath_signal = 0;
> +	current->signal->pdeath_signal = 0;
>
>  	/* reset soft limits and set hard limits for the new profile */
>  	__aa_transition_rlimits(profile, new_cxt->profile);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 372ec65..e839600 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2195,7 +2195,7 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
>  	flush_unauthorized_files(bprm->cred, current->files);
>
>  	/* Always clear parent death signal on SID transitions. */
> -	current->pdeath_signal = 0;
> +	current->signal->pdeath_signal = 0;
>
>  	/* Check whether the new SID can inherit resource limits from the old
>  	 * SID.  If not, reset all soft limits to the lower of the current
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index ee0bb57..3c85790 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -496,7 +496,7 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
>  	struct task_smack *bsp = bprm->cred->security;
>
>  	if (bsp->smk_task != bsp->smk_forked)
> -		current->pdeath_signal = 0;
> +		current->signal->pdeath_signal = 0;
>  }
>
>  /**
> --
> 1.7.7.6
>

Re: [PATCHv2 1/1] prctl: move pdeath_signal from task_struct to signal_struct

[Albert Cahalan]

On Tue, Jun 12, 2012 at 12:19 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/11, Filipe Brandenburger wrote:

> However. This code is very, very old. I simply do not know if we can
> change the current behaviour or not. In particular, Albert Cahalan
> didn't like this change when it was last discussed because it can
> break his application.
...
>> Documentation clearly states that PR_SET_PDEATHSIG and PR_GET_PDEATHSIG should
>> act on processes, in particular case #1 is very counter intuitive because the
>> child process should not care whether the parent is multi-threaded or not.

You are assuming that the child and parent are normal separate
applications, developed by separate people, with one just being
run by the other. That is actually the odd case AFAIK, because
why then would there be reason to care about parent death?
The more normal case IMHO is that the child and parent are
deeply aware of each other. Most likely they are the same program.

In my case I have an in-process emulator, kind of like valgrind,
that uses real threads for accuracy and performance. It makes
extra threads for monitoring and control. These extra threads
keep an eye on the app being examined; obviously none of this
would work if the death signal were not per-thread.

Fix the documentation to match reality, then add a per-process one
if there is any use for it. I don't think I've ever seen use for a
per-process death signal, so perhaps it shouldn't be added even
though it does make sense.

Re: [PATCHv2 1/1] prctl: move pdeath_signal from task_struct to signal_struct

[Filipe Brandenburger]

Hi,

On Wed, Jun 13, 2012 at 11:46 AM, Albert Cahalan <acahalan@gmail.com> wrote:
>> On 06/11, Filipe Brandenburger wrote:
>>> Documentation clearly states that PR_SET_PDEATHSIG and PR_GET_PDEATHSIG should
>>> act on processes, in particular case #1 is very counter intuitive because the
>>> child process should not care whether the parent is multi-threaded or not.
>
> You are assuming that the child and parent are normal separate
> applications, developed by separate people, with one just being
> run by the other. That is actually the odd case AFAIK, because
> why then would there be reason to care about parent death?
> The more normal case IMHO is that the child and parent are
> deeply aware of each other. Most likely they are the same program.
>
> In my case I have an in-process emulator, kind of like valgrind,
> that uses real threads for accuracy and performance. It makes
> extra threads for monitoring and control. These extra threads
> keep an eye on the app being examined; obviously none of this
> would work if the death signal were not per-thread.
>
> Fix the documentation to match reality, then add a per-process one
> if there is any use for it. I don't think I've ever seen use for a
> per-process death signal, so perhaps it shouldn't be added even
> though it does make sense.

The issue I have with the current implementation is that it's non
obvious, it has some quite awkward corner cases (see
https://bugzilla.kernel.org/show_bug.cgi?id=43300) and it ends up
exposing implementation details of the Linux kernel that should not
really be that visible to userland...

But I don't really have a strong case for needing this change, I only
got involved on it through the Bugzilla report, it struck me as odd...

I'm copying David Wilcox who opened the Bugzilla issue and Adam
Peterson who commented on the issue, they both seem to have use cases
for the code to match the documentation, they might be able to give
better arguments of why they would like to have this behavior changed.

Thanks,
Filipe