Re: [patch V4 part 1 05/36] x86/entry: Flip _TIF_SIGPENDING and _TIF_NOTIFY_RESUME handling

[Steven Rostedt]

On Wed, 13 May 2020 16:56:41 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:

> ----- On May 5, 2020, at 9:16 AM, Thomas Gleixner tglx@xxxxxxxxxxxxx wrote:
> 
> > Make sure task_work runs before any kind of userspace -- very much
> > including signals -- is invoked.  
> 
> What is missing from this patch description is: _why_ is this deemed
> useful ?
> 
> Also, color me confused: is "do_signal()" actually running any user-space,
> or just setting up the user-space stack for eventual return to signal handler ?
> 
> Also, it might be OK, but we're changing the order of two things which
> have effects on each other: restartable sequences abort fixup for preemption
> and do_signal(), which also have effects on rseq abort.
> 
> Because those two will cause the abort to trigger, I suspect changing
> the order might be OK, but we really need to think this through.
> 
> Thanks,
> 
> Mathieu
> 
> > 
> > Suggested-by: Andy Lutomirski <luto@xxxxxxxxxx>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > ---
> > arch/x86/entry/common.c |    8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > --- a/arch/x86/entry/common.c
> > +++ b/arch/x86/entry/common.c
> > @@ -156,16 +156,16 @@ static void exit_to_usermode_loop(struct
> > 		if (cached_flags & _TIF_PATCH_PENDING)
> > 			klp_update_patch_state(current);
> > 
> > -		/* deal with pending signal delivery */
> > -		if (cached_flags & _TIF_SIGPENDING)
> > -			do_signal(regs);
> > -
> > 		if (cached_flags & _TIF_NOTIFY_RESUME) {
> > 			clear_thread_flag(TIF_NOTIFY_RESUME);
> > 			tracehook_notify_resume(regs);
> > 			rseq_handle_notify_resume(NULL, regs);
> > 		}
> > 
> > +		/* deal with pending signal delivery */
> > +		if (cached_flags & _TIF_SIGPENDING)
> > +			do_signal(regs);

Looking deeper into this, it appears that do_signal() can freeze or kill the
task.

That is, it wont go back to user space here, but simply schedule out (being
traced) or even exit (killed).

Before the resume hooks would never be called in such cases, and now they
are.

-- Steve


> > +
> > 		if (cached_flags & _TIF_USER_RETURN_NOTIFY)
> >  			fire_user_return_notifiers();  
>