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

From: Eric W. Biederman
Date: Mon Jun 06 2022 - 10:34:16 EST


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