Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 9 Mar 2022 21:57:22 -0800
From:   Kees Cook <keescook@chromium.org>
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Alexey Gladkov <legion@kernel.org>,
        Kyle Huey <me@kylehuey.com>, Oleg Nesterov <oleg@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>, Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH 08/13] task_work: Call tracehook_notify_signal from
 get_signal on all architectures
Message-ID: <202203091455.D4ED8650@keescook>
References: <87o82gdlu9.fsf_-_@email.froward.int.ebiederm.org>
 <20220309162454.123006-8-ebiederm@xmission.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220309162454.123006-8-ebiederm@xmission.com>
Precedence: bulk

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