Re: [PATCH v5 3/6] signal: Add calculate_sigpending()

Re: [PATCH v5 3/6] signal: Add calculate_sigpending()

[Eric W. Biederman]

<wen.yang99@zte.com.cn> writes:

> EricW.Biederman <ebiederm@xmission.com> wrote:
>> Add a function calculate_sigpending to test to see if any signals are
>> pending for a new task immediately following fork. Signals have to
>> happen either before or after fork. Today our practice is to push
>> all of the signals to before the fork, but that has the downside that
>> frequent or periodic signals can make fork take much much longer than
>> normal or prevent fork from completing entirely.
>> 
>
>> + calculate_sigpending();
>> }
>> /*
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index dddbea558455..1e06f1eba363 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -172,6 +172,17 @@ void recalc_sigpending(void)
>> }
>> +void calculate_sigpending(void)
>> +{
>> + /* Have any signals or users of TIF_SIGPENDING been delayed
>> + * until after fork?
>> + */
>> + spin_lock_irq(&current->sighand->siglock);
>> + set_tsk_thread_flag(current, TIF_SIGPENDING);
>> + recalc_sigpending();
>> + spin_unlock_irq(&current->sighand->siglock);
>> +}
>> +
>
> The new function calculate_sigpending is similar to  recalc_sigpending,
> but recalc_sigpending has no spin_lock_irq(&current->sighand->siglock) in it.
> This gives recalc_sigpending more flexibility, 
> we may use spin_lock_irq or spin_lock_irqsave before recalc_sigpending .
> eg: 
>
> static int autofs4_write(struct autofs_sb_info *sbi,
>              struct file *file, const void *addr, int bytes)
> {
>   ...
>         spin_lock_irqsave(&current->sighand->siglock, flags);
>         sigdelset(&current->pending.signal, SIGPIPE);
>         recalc_sigpending();
>         spin_unlock_irqrestore(&current->sighand->siglock, flags);
>   ... 
> }
>
> or:
> void kernel_sigaction(int sig, __sighandler_t action)
> {
>     spin_lock_irq(&current->sighand->siglock);
> ...
>         recalc_sigpending();
> ...   
>     spin_unlock_irq(&current->sighand->siglock);
> }
>
>
> But calculate_sigpending is currently hardwired to call spin_lock_irq.

calculate_sigpending really only exists to keep the code comprehensible.

It is only ever expected to be called in exactly one place so the lack
of flexibility should not be a problem.  Further the use of irqsave
is discouraged unless it is necessary.

The irqsave in autofs_write actually looks like a misfeature.  We take
a mutex a few lines earlier, so we know that irqs are enabled.  Saving
and restoring them is uncessary work.  Further unless I am missing
something that code path should be calling kernel_dequeue_signal, to
ensure that any siginfo associated with that SIGPIPE gets dequeued.

Eric

[PATCH v5 0/6] Not restarting for due to signals.

[Eric W. Biederman]

This builds on patches 1-15 of my previous patch posting.  As those are
non-controversial I am not posting them again.

I took longer than I had hoped to get this set together because a kernel
testing robot noticed some random corruption with the way I had been
adding to the list.  I finally tracked it down to failing to remove the
sigset from the list during fork_idle.  So I have made that logic
simpler and use hlist_del_init which will only remove an item from a
list if it was placed on the list in the first place.

I took Oleg's suggesting and moved calculate_sigpending into
schedule_tail where recalc_sigpending an be used directly.  Then in
calculate_sigpending I just unconditionally set TIF_SIGPENDING and allow
recalc_sigpending to clear TIF_SIGPENDING if we don't need it.

I also now handle the stop/continue signal magic where we only let one
of stop signals and SIGCONT be pending at a time.  Looking at it from
first principles dropping one of SIGTSTP SIGTTIN SIGTTOU or SIGCONT
before calling it's handler feels wrong.  I checked and it is our
historical behavior, so I won't even thinking of introducing different
behavior at this point.

Eric W. Biederman (6):
      fork: Move and describe why the code examines PIDNS_ADDING
      fork: Unconditionally exit if a fatal signal is pending
      signal: Add calculate_sigpending()
      fork: Skip setting TIF_SIGPENDING in ptrace_init_task
      fork: Have new threads join on-going signal group stops
      signal: Don't restart fork when signals come in.

 include/linux/ptrace.h       |  2 --
 include/linux/sched/signal.h | 11 ++++++++++
 init/init_task.c             |  1 +
 kernel/fork.c                | 49 ++++++++++++++++++++++++++++++--------------
 kernel/sched/core.c          |  2 ++
 kernel/signal.c              | 43 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 91 insertions(+), 17 deletions(-)

Eric

[PATCH v5 3/6] signal: Add calculate_sigpending()

[Eric W. Biederman]

Add a function calculate_sigpending to test to see if any signals are
pending for a new task immediately following fork.  Signals have to
happen either before or after fork.  Today our practice is to push
all of the signals to before the fork, but that has the downside that
frequent or periodic signals can make fork take much much longer than
normal or prevent fork from completing entirely.

So we need move signals that we can after the fork to prevent that.

This updates the code to set TIF_SIGPENDING on a new task if there
are signals or other activities that have moved so that they appear
to happen after the fork.

As the code today restarts if it sees any such activity this won't
immediately have an effect, as there will be no reason for it
to set TIF_SIGPENDING immediately after the fork.

Adding calculate_sigpending means the code in fork can safely be
changed to not always restart if a signal is pending.

The new calculate_sigpending function sets sigpending if there
are pending bits in jobctl, pending signals, the freezer needs
to freeze the new task or the live kernel patching framework
need the new thread to take the slow path to userspace.

I have verified that setting TIF_SIGPENDING does make a new process
take the slow path to userspace before it executes it's first userspace
instruction.

I have looked at the callers of signal_wake_up and the code paths
setting TIF_SIGPENDING and I don't see anything else that needs to be
handled.  The code probably doesn't need to set TIF_SIGPENDING for the
kernel live patching as it uses a separate thread flag as well.  But
at this point it seems safer reuse the recalc_sigpending logic and get
the kernel live patching folks to sort out their story later.

V2: I have moved the test into schedule_tail where siglock can
    be grabbed and recalc_sigpending can be reused directly.
    Further as the last action of setting up a new task this
    guarantees that TIF_SIGPENDING will be properly set in the
    new process.

    The helper calculate_sigpending takes the siglock and
    uncontitionally sets TIF_SIGPENDING and let's recalc_sigpending
    clear TIF_SIGPENDING if it is unnecessary.  This allows reusing
    the existing code and keeps maintenance of the conditions simple.

    Oleg Nesterov <oleg@redhat.com>  suggested the movement
    and pointed out the need to take siglock if this code
    was going to be called while the new task is discoverable.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/sched/signal.h |  1 +
 kernel/sched/core.c          |  2 ++
 kernel/signal.c              | 11 +++++++++++
 3 files changed, 14 insertions(+)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 94558ffa82ab..b55fd293c1e5 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -372,6 +372,7 @@ static inline int signal_pending_state(long state, struct task_struct *p)
  */
 extern void recalc_sigpending_and_wake(struct task_struct *t);
 extern void recalc_sigpending(void);
+extern void calculate_sigpending(void);
 
 extern void signal_wake_up_state(struct task_struct *t, unsigned int state);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 78d8facba456..3e4ed4b7aa2d 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2813,6 +2813,8 @@ asmlinkage __visible void schedule_tail(struct task_struct *prev)
 
 	if (current->set_child_tid)
 		put_user(task_pid_vnr(current), current->set_child_tid);
+
+	calculate_sigpending();
 }
 
 /*
diff --git a/kernel/signal.c b/kernel/signal.c
index dddbea558455..1e06f1eba363 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -172,6 +172,17 @@ void recalc_sigpending(void)
 
 }
 
+void calculate_sigpending(void)
+{
+	/* Have any signals or users of TIF_SIGPENDING been delayed
+	 * until after fork?
+	 */
+	spin_lock_irq(&current->sighand->siglock);
+	set_tsk_thread_flag(current, TIF_SIGPENDING);
+	recalc_sigpending();
+	spin_unlock_irq(&current->sighand->siglock);
+}
+
 /* Given the mask, find the first available signal that should be serviced. */
 
 #define SYNCHRONOUS_MASK \
-- 
2.17.1