Re: [PATCH v1] signal.c: repeatedly set the TIF_SIGPENDING flag

[Eric W. Biederman]

Hao Wu <guoyuanchao1202@xxxxxxxxx> writes:

> The recalc_sigpending_and_wake() function calls recalc_sigpending_tsk() and signal_wake_up(),
> both of which set the TIF_SIGPENDING flag, so when recalc_sigpending_tsk() returns true,
> the TIF_SIGPENDING flag will be set twice.
>
> Maybe we can take away the set TIF_SIGPENDING logic from recalc_sigpending_tsk(),
> so that recalc_sigpending_tsk() just determines whether the TIF_SIGPENDING flag needs to be set,
> and the actual set TIF_SIGPENDING flag logic is executed outside.
>
> kernel/signal.c:175: recalc_sigpending_and_wake()
> kernel/signal.c:154: recalc_sigpending_tsk()

I just saw this.

I agree that it is unfortunate that recalc_sigpending_and_wake sets
TIF_SIGPENDING twice.

Looking at the code only force_sig_info_to_task calls
recalc_sigpending_and_wake.  I have some work underway that removes the
need to call recalc_sigpending in force_sig_info_to_task so I believe
that will resolve the matter.

Regardless of changes I have planned force_sig_info_to_task appears to
be the proper place to resolve this issue.  It looks like
complete_signal called from __send_signal_locked called from
send_signal_locked called from force_sig_info_to_task calls
signal_wake_up if that is needed at all.  So I believe the issue you are
seeing could be solved be simply replacing recalc_sigpending_and_wake
with recalc_sigpending_tsk.

Some more analysis might reveal and even nicer clean up.  If
sending the signal is always going to call signal_wake_up calling
recalc_sigpending_tsk might not even be necessary.

Good spotting of a rough spot in the code.  I don't think your proposed
change is the way we should go about fixing it.

Eric
>
> Signed-off-by: Hao Wu <guoyuanchao1202@xxxxxxxxx>
> ---
>  kernel/signal.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9b04631acde8..6c5a00cd7e9a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -157,7 +157,6 @@ static bool recalc_sigpending_tsk(struct task_struct *t)
>  	    PENDING(&t->pending, &t->blocked) ||
>  	    PENDING(&t->signal->shared_pending, &t->blocked) ||
>  	    cgroup_task_frozen(t)) {
> -		set_tsk_thread_flag(t, TIF_SIGPENDING);
>  		return true;
>  	}
> @@ -181,8 +180,11 @@ void recalc_sigpending_and_wake(struct task_struct *t)
>  void recalc_sigpending(void)
>  {
> -	if (!recalc_sigpending_tsk(current) && !freezing(current))
> +	if (recalc_sigpending_tsk(current)) {
> +		set_tsk_thread_flag(t, TIF_SIGPENDING);
> +	} else if (!freezing(current)) {
>  		clear_thread_flag(TIF_SIGPENDING);
> +	}
>  }
>  EXPORT_SYMBOL(recalc_sigpending);
> @@ -2325,7 +2327,9 @@ static void ptrace_stop(int exit_code, int why, int clear_code, kernel_siginfo_t
>  	 * So check for any that we should take before resuming user mode.
>  	 * This sets TIF_SIGPENDING, but never clears it.
>  	 */
> -	recalc_sigpending_tsk(current);
> +	if (recalc_sigpending_tsk(current)) {
> +		set_tsk_thread_flag(t, TIF_SIGPENDING);
> +	}
>  }
>  static void ptrace_do_notify(int signr, int exit_code, int why)
> 2.32.0