Re: Bug 16061 - single stepping in a signal handler can cause the single step flag to get "stuck"

[Oleg Nesterov <oleg@redhat.com>]

sorry for noise, forgot to mention...

On 06/02, Oleg Nesterov wrote:
>
> However, what I am thinking about is the more "clever" change (it
> passed ptrace-tests).
>
> Do you think it can be correct? I am asking because I never understood
> the TIF_SINGLESTEP/TIF_FORCED_TF interaction. But otoh, shouldn't
> TIF_FORCED_TF + X86_EFLAGS_TF always imply TIF_SINGLESTEP? at least
> in handle_signal().
>
> IOW, help! To me, the patch below is also cleanup. But only if you think
> it can fly ;)

and it is not clear to me if we should keep this code

	/*
	 * Clear TF when entering the signal handler, but
	 * notify any tracer that was single-stepping it.
	 * The tracer may want to single-step inside the
	 * handler too.
	 */
	regs->flags &= ~X86_EFLAGS_TF;

in handle_signal(). If TF was set by us, it was cleared by
user_disable_single_step(). Otherwise, why should we clear it if
the tracer did set_flags(X86_EFLAGS_TF) ?

Oleg.

> --- 34-rc1/arch/x86/kernel/signal.c~BZ16061_MAYBE_FIX	2010-06-02 21:11:07.000000000 +0200
> +++ 34-rc1/arch/x86/kernel/signal.c	2010-06-02 21:11:48.000000000 +0200
> @@ -682,6 +682,7 @@ static int
>  handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
>  	      sigset_t *oldset, struct pt_regs *regs)
>  {
> +	bool stepping;
>  	int ret;
>  
>  	/* Are we from a system call? */
> @@ -706,13 +707,10 @@ handle_signal(unsigned long sig, siginfo
>  		}
>  	}
>  
> -	/*
> -	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
> -	 * flag so that register information in the sigcontext is correct.
> -	 */
> -	if (unlikely(regs->flags & X86_EFLAGS_TF) &&
> -	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
> -		regs->flags &= ~X86_EFLAGS_TF;
> +	stepping = test_thread_flag(TIF_SINGLESTEP);
> +	if (stepping)
> +		// do this before setup_sigcontext()
> +		user_disable_single_step(current);
>  
>  	ret = setup_rt_frame(sig, ka, info, oldset, regs);
>  
> @@ -748,8 +746,7 @@ handle_signal(unsigned long sig, siginfo
>  	recalc_sigpending();
>  	spin_unlock_irq(&current->sighand->siglock);
>  
> -	tracehook_signal_handler(sig, info, ka, regs,
> -				 test_thread_flag(TIF_SINGLESTEP));
> +	tracehook_signal_handler(sig, info, ka, regs, stepping);
>  
>  	return 0;
>  }