Messages in this thread | | | Date | Wed, 9 Mar 2022 21:57:22 -0800 | From | Kees Cook <> | Subject | Re: [PATCH 08/13] task_work: Call tracehook_notify_signal from get_signal on all architectures |
| |
On Wed, Mar 09, 2022 at 10:24:49AM -0600, Eric W. Biederman wrote: > Always handle TIF_NOTIFY_SIGNAL in get_signal. With commit 35d0b389f3b2 > ("task_work: unconditionally run task_work from get_signal()") always > calling task_wofffffffrk_run all of the work of tracehook_notify_signal is
typo: cat on keyboard
> already happening except clearing TIF_NOTIFY_SIGNAL. > > Factor clear_notify_signal out of tracehook_notify_signal and use it in > get_signal so that get_signal only needs one call of trask_work_run.
typo: trask -> task
> > To keep the semantics in sync update xfer_to_guest_mode_work (which > does not call get_signal) to call tracehook_notify_signal if either > _TIF_SIGPENDING or _TIF_NOTIFY_SIGNAL.
I see three logical changes in this patch, I think?
- creation and use of clear_notify_signal() - removal of handle_signal_work() and removal of arch_do_signal_or_restart() has_signal arg - something with get_signal() I don't understand yet: - why is clear_notify_signal() added? - why is tracehook_notify_signal() removed?
> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > arch/s390/kernel/signal.c | 4 ++-- > arch/x86/kernel/signal.c | 4 ++-- > include/linux/entry-common.h | 2 +- > include/linux/tracehook.h | 9 +++++++-- > kernel/entry/common.c | 12 ++---------- > kernel/entry/kvm.c | 2 +- > kernel/signal.c | 14 +++----------- > 7 files changed, 18 insertions(+), 29 deletions(-) > > diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c > index 307f5d99514d..ea9e5e8182cd 100644 > --- a/arch/s390/kernel/signal.c > +++ b/arch/s390/kernel/signal.c > @@ -453,7 +453,7 @@ static void handle_signal(struct ksignal *ksig, sigset_t *oldset, > * stack-frames in one go after that. > */ > > -void arch_do_signal_or_restart(struct pt_regs *regs, bool has_signal) > +void arch_do_signal_or_restart(struct pt_regs *regs) > { > struct ksignal ksig; > sigset_t *oldset = sigmask_to_save(); > @@ -466,7 +466,7 @@ void arch_do_signal_or_restart(struct pt_regs *regs, bool has_signal) > current->thread.system_call = > test_pt_regs_flag(regs, PIF_SYSCALL) ? regs->int_code : 0; > > - if (has_signal && get_signal(&ksig)) {
Right, the only caller of arch_do_signal_or_restart(), handle_signal_work(), only happens after its caller has already checked _TIF_SIGPENDING.
> + if (get_signal(&ksig)) { > /* Whee! Actually deliver the signal. */ > if (current->thread.system_call) { > regs->int_code = current->thread.system_call; > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c > index ec71e06ae364..de3d5b5724d8 100644 > --- a/arch/x86/kernel/signal.c > +++ b/arch/x86/kernel/signal.c > @@ -861,11 +861,11 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs) > * want to handle. Thus you cannot kill init even with a SIGKILL even by > * mistake. > */ > -void arch_do_signal_or_restart(struct pt_regs *regs, bool has_signal) > +void arch_do_signal_or_restart(struct pt_regs *regs) > { > struct ksignal ksig; > > - if (has_signal && get_signal(&ksig)) { > + if (get_signal(&ksig)) { > /* Whee! Actually deliver the signal. */ > handle_signal(&ksig, regs); > return; > diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h > index 9efbdda61f7a..3537fd25f14e 100644 > --- a/include/linux/entry-common.h > +++ b/include/linux/entry-common.h > @@ -257,7 +257,7 @@ static __always_inline void arch_exit_to_user_mode(void) { } > * > * Invoked from exit_to_user_mode_loop(). > */ > -void arch_do_signal_or_restart(struct pt_regs *regs, bool has_signal); > +void arch_do_signal_or_restart(struct pt_regs *regs); > > /** > * exit_to_user_mode - Fixup state when exiting to user mode > diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h > index fa834a22e86e..b44a7820c468 100644 > --- a/include/linux/tracehook.h > +++ b/include/linux/tracehook.h > @@ -106,6 +106,12 @@ static inline void tracehook_notify_resume(struct pt_regs *regs) > rseq_handle_notify_resume(NULL, regs); > } > > +static inline void clear_notify_signal(void) > +{ > + clear_thread_flag(TIF_NOTIFY_SIGNAL); > + smp_mb__after_atomic(); > +} > + > /* > * called by exit_to_user_mode_loop() if ti_work & _TIF_NOTIFY_SIGNAL. This > * is currently used by TWA_SIGNAL based task_work, which requires breaking > @@ -113,8 +119,7 @@ static inline void tracehook_notify_resume(struct pt_regs *regs) > */ > static inline void tracehook_notify_signal(void) > { > - clear_thread_flag(TIF_NOTIFY_SIGNAL); > - smp_mb__after_atomic(); > + clear_notify_signal(); > if (task_work_pending(current)) > task_work_run(); > } > diff --git a/kernel/entry/common.c b/kernel/entry/common.c > index f0b1daa1e8da..79eaf9b4b10d 100644 > --- a/kernel/entry/common.c > +++ b/kernel/entry/common.c > @@ -139,15 +139,7 @@ void noinstr exit_to_user_mode(void) > } > > /* Workaround to allow gradual conversion of architecture code */ > -void __weak arch_do_signal_or_restart(struct pt_regs *regs, bool has_signal) { } > - > -static void handle_signal_work(struct pt_regs *regs, unsigned long ti_work) > -{ > - if (ti_work & _TIF_NOTIFY_SIGNAL) > - tracehook_notify_signal(); > - > - arch_do_signal_or_restart(regs, ti_work & _TIF_SIGPENDING); > -} > +void __weak arch_do_signal_or_restart(struct pt_regs *regs) { } > > static unsigned long exit_to_user_mode_loop(struct pt_regs *regs, > unsigned long ti_work) > @@ -170,7 +162,7 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs, > klp_update_patch_state(current); > > if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) > - handle_signal_work(regs, ti_work); > + arch_do_signal_or_restart(regs); > > if (ti_work & _TIF_NOTIFY_RESUME) > tracehook_notify_resume(regs); > diff --git a/kernel/entry/kvm.c b/kernel/entry/kvm.c > index 96d476e06c77..cabf36a489e4 100644 > --- a/kernel/entry/kvm.c > +++ b/kernel/entry/kvm.c > @@ -8,7 +8,7 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work) > do { > int ret; > > - if (ti_work & _TIF_NOTIFY_SIGNAL) > + if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) > tracehook_notify_signal(); > > if (ti_work & _TIF_SIGPENDING) { > diff --git a/kernel/signal.c b/kernel/signal.c > index 3b4cf25fb9b3..8632b88982c9 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -2626,20 +2626,12 @@ bool get_signal(struct ksignal *ksig) > struct signal_struct *signal = current->signal; > int signr; > > + clear_notify_signal();
Why is this added?
> if (unlikely(task_work_pending(current))) > task_work_run(); > > - /* > - * For non-generic architectures, check for TIF_NOTIFY_SIGNAL so > - * that the arch handlers don't all have to do it. If we get here > - * without TIF_SIGPENDING, just exit after running signal work. > - */ > - if (!IS_ENABLED(CONFIG_GENERIC_ENTRY)) { > - if (test_thread_flag(TIF_NOTIFY_SIGNAL)) > - tracehook_notify_signal();
I don't see why this gets removed?
> - if (!task_sigpending(current)) > - return false; > - } > + if (!task_sigpending(current)) > + return false; > > if (unlikely(uprobe_deny_signal())) > return false; > -- > 2.29.2 >
-- Kees Cook
| |