Messages in this thread | | | From | "Eric W. Biederman" <> | Date | Mon, 06 Jun 2022 09:33:17 -0500 | Subject | Re: [PATCH v1] signal.c: repeatedly set the TIF_SIGPENDING flag |
| |
Hao Wu <guoyuanchao1202@gmail.com> 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@gmail.com> > --- > 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
| |