Re: [RFC][PATCH] Notify init when processes are reparented to it
From: Oleg Nesterov
Date: Sun Dec 28 2008 - 04:22:30 EST
On 12/27, Scott James Remnant wrote:
>
> Allow the init daemon to be notified by signal when processes are
> reparented to it. The signal has the same form as SIGCHLD except that
> the si_status field contains the original parent proecss id.
I think the changelog should explain why this change is useful.
I can't judge the usefulness, just a couple of nits about the
code.
> +++ b/include/linux/sched.h
> @@ -1133,6 +1133,7 @@ struct task_struct {
> int exit_state;
> int exit_code, exit_signal;
> int pdeath_signal; /* The signal sent when the parent dies */
> + int adopt_signal; /* The signal sent when a process is reparented */
Should be cleared in copy_process() ?
And what if init is multithreaded? This should be per-process, not
per-thread.
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -813,6 +813,9 @@ static void reparent_thread(struct task_struct *p, struct task_struct *father)
> /* We already hold the tasklist_lock here. */
> group_send_sig_info(p->pdeath_signal, SEND_SIG_NOINFO, p);
>
> + if (p->real_parent->adopt_signal)
> + do_notify_parent_adopted(p, father);
> +
Please note that we notify the new parent even if we re-parent to our
sub-thread, not to the new process. This means that a multithreaded
init can have the false notifications.
And, given that ->adopt_signal is per-thread, a multithreaded init
should be very careful with prctl(PR_SET_ADOPTSIG) if it does not
want to miss the notification.
> +void do_notify_parent_adopted(struct task_struct *tsk, struct task_struct *father)
> +{
> + struct siginfo info;
> + unsigned long flags;
> + struct task_struct *reaper;
> + struct sighand_struct *sighand;
> + int ret;
> +
> + reaper = tsk->real_parent;
> +
> + memset (&info, 0, sizeof info);
Unneeded? This function seems to fill all interesting fields.
> + info.si_signo = reaper->adopt_signal;
> + /*
> + * set code to the same range as SIGCHLD so the right bits of
> + * siginfo_t get copied, to userspace this will appear as si_code=0
> + */
> + info.si_code = __SI_CHLD;
> + /*
> + * see comment in do_notify_parent() about the following 4 lines
> + */
> + rcu_read_lock();
> + info.si_pid = task_pid_nr_ns(tsk, reaper->nsproxy->pid_ns);
> + info.si_status = task_pid_nr_ns(father, reaper->nsproxy->pid_ns);
perhaps it is better to use task_active_pid_ns(reaper).
> + rcu_read_unlock();
> +
> + info.si_uid = tsk->uid;
> +
> + info.si_utime = cputime_to_clock_t(tsk->utime);
> + info.si_stime = cputime_to_clock_t(tsk->stime);
> +
> + sighand = reaper->sighand;
> + spin_lock_irqsave(&sighand->siglock, flags);
> + __group_send_sig_info(reaper->adopt_signal, &info, reaper);
This looks unsafe.
At this point reaper can already change its ->adopt_signal. For example
if it was cleared, send_signal() can crash while doing (say) sigismember().
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/