Oleg Nesterov <[email protected]> writes:
> On 07/09, Linus Torvalds wrote:
>>
>> But the patch was written for testing and feedback more than anything
>> else. Comments?
>
> see my reply on bugzilla. Can't we add lkml?
Done.
> Perhaps we can do another change? Not sure it is actually better, but I think
> it is always good to discuss the alternatives.
>
> 1. Once again, we turn "int group" argument into thread/process/pgid enum, and
> change kill_pgrp/tty_signal_session_leader to pass "group = pgid", imo this
> makes sense in any case.
I agree. There are a lot more multi-process signals cases than handled
in Linus's patch. I believe this will clean that up.
> 2. To simplify, lets suppose we add the new PF_INFORK flag. Yes, this is bad,
> we can do better. I think we can simply add "struct hlist_head
> forking_threads"
> into signal_struct, so complete_signal() can just do hlist_for_each_entry()
> rather than for_each_thread() + PF_INFORK check. We don't even need a new
> member in task_struct.
We still need the distinction between multi-process signals and single
process signals (which is the hard part). For good performance of
signal delivery to multi-threaded tasks we still need a new member in
signal_struct. Plus it is a bit more work to update the list or even
walk the list than a sequence counter.
So I think adding a sequence counter to let us know about multiprocess
signals is the local optimum.
If we ever need to remove the restart case entirely from fork and queue
all new multi-process signals for the newly created task. Going through
the list of forking
>
> 3. copy_process() can simply block/unblock all signals (except KILL/STOP), see
> the "patch" below.
All signals are effectively blocked for the duration of the fork for the
calling task. Where we get into trouble and where we need a fix for
correctness is that another thread can dequeue the signal. Blocking
signals of the forking task does not change that.
I think that reveals another bug in our current logic. For blocked
multi-process signals we don't ensure they get delivered to both the
parent and the child if the signal logically comes in after the fork.
Multi-threaded b0rked ness and blocked signal b0rkenness, plus periodic
timers making for take forever for no good reason. This business of
ensuring a signal is logically delvered before or after a fork is
tricky.
Eric
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 9440d61..652ef09 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1591,6 +1591,23 @@ static __latent_entropy struct task_struct
> *copy_process(
> int retval;
> struct task_struct *p;
>
> + spin_lock_irq(¤t->sighand->siglock);
> + recalc_sigpending_tsk(current);
> + if (signal_pending(current)) {
> + retval = -ERESTARTNOINTR;
> + } else {
> + // Yes, yes, this is wrong, just to explain the idea.
> + // We should not block SIGKILL, we need to clear PF_INFORK
> + // and restore ->blocked in error paths, we need helper(s).
> + retval = 0;
> + current->flags |= PF_INFORK;
> + current->saved_sigmask = current->blocked;
> + sigfillset(¤t->blocked);
> + }
> + spin_unlock_irq(¤t->sighand->siglock);
> + if (retval)
> + return retval;
> +
> /*
> * Don't allow sharing the root directory with processes in a different
> * namespace
> @@ -1918,8 +1935,14 @@ static __latent_entropy struct task_struct
> *copy_process(
> * A fatal signal pending means that current will exit, so the new
> * thread can't slip out of an OOM kill (or normal SIGKILL).
> */
> - recalc_sigpending();
> - if (signal_pending(current)) {
> + // check signal_pending() before __set_task_blocked() which does
> + // recalc_sigpending(). Again, this is just to explain the idea,
> + // __set_task_blocked() is not exported, it is suboptimal, we can
> + // do better.
> + bool eintr = signal_pending();
> + current->flags &= ~PF_INFORK;
> + __set_task_blocked(current, ¤t->saved_sigmask);
> + if (eintr) {
> retval = -ERESTARTNOINTR;
> goto bad_fork_cancel_cgroup;
> }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 8d8a940..3433e66 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -900,6 +900,14 @@ static void complete_signal(int sig, struct task_struct
> *p, int group)
> struct signal_struct *signal = p->signal;
> struct task_struct *t;
>
> + // kill_pgrp/etc.
> + if (group == 2) {
> + for_each_thread(p, t) {
> + if (p->flags & PF_INFORK &&
> !sigismember(&p->saved_sigmask, sig))
> + signal_wake_up(t, 0);
> + }
> + }
> +
> /*
> * Now find a thread we can wake up to take the signal off the queue.
> *
