Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 13 May 2020 22:24:56 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
        paulmck <paulmck@kernel.org>, Andy Lutomirski <luto@kernel.org>,
        Alexandre Chartre <alexandre.chartre@oracle.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Petr Mladek <pmladek@suse.com>, rostedt <rostedt@goodmis.org>,
        "Joel Fernandes, Google" <joel@joelfernandes.org>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Juergen Gross <jgross@suse.com>,
        Brian Gerst <brgerst@gmail.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Will Deacon <will@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
Message-ID: <552488029.20647.1589423096441.JavaMail.zimbra@efficios.com>
In-Reply-To: <20200505135314.808628211@linutronix.de>
References: <20200505134926.578885807@linutronix.de> <20200505135314.808628211@linutronix.de>
Subject: Re: [patch V4 part 4 15/24] x86/db: Split out dr6/7 handling
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Thread-Topic: x86/db: Split out dr6/7 handling
Thread-Index: Lshy92u6l/WGHmCqbJJrQlEvg9Pv+w==
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

----- On May 5, 2020, at 9:49 AM, Thomas Gleixner tglx@linutronix.de wrote:

> From: Peter Zijlstra <peterz@infradead.org>
> 
> DR6/7 should be handled before nmi_enter() is invoked and restore after
> nmi_exit() to minimize the exposure.
> 
> Split it out into helper inlines and bring it into the correct order.
> 
[...]
> 
> +static __always_inline void debug_enter(unsigned long *dr6, unsigned long *dr7)
> +{
> +	/*
> +	 * Disable breakpoints during exception handling; recursive exceptions
> +	 * are exceedingly 'fun'.
> +	 *
> +	 * Since this function is NOKPROBE, and that also applies to
> +	 * HW_BREAKPOINT_X, we can't hit a breakpoint before this (XXX except a
> +	 * HW_BREAKPOINT_W on our stack)
> +	 *
> +	 * Entry text is excluded for HW_BP_X and cpu_entry_area, which
> +	 * includes the entry stack is excluded for everything.
> +	 */
> +	get_debugreg(*dr7, 6);
> +	set_debugreg(0, 7);
> +
> +	/*
> +	 * The Intel SDM says:
> +	 *
> +	 *   Certain debug exceptions may clear bits 0-3. The remaining
> +	 *   contents of the DR6 register are never cleared by the
> +	 *   processor. To avoid confusion in identifying debug
> +	 *   exceptions, debug handlers should clear the register before
> +	 *   returning to the interrupted task.
> +	 *
> +	 * Keep it simple: clear DR6 immediately.
> +	 */
> +	get_debugreg(*dr6, 6);
> +	set_debugreg(0, 6);
> +	/* Filter out all the reserved bits which are preset to 1 */
> +	*dr6 &= ~DR6_RESERVED;
> +}
> +
> +static __always_inline void debug_exit(unsigned long dr7)
> +{
> +	set_debugreg(dr7, 7);
> +}

Out of curiosity, what prevents the compiler from moving instructions
outside of the code regions surrounded by entry/exit ? This is an always
inline, which invokes set_debugreg which is inline for CONFIG_PARAVIRT_XXL=n,
which in turn uses an asm() (not volatile), without any memory clobber.

Also, considering that "inline" is not sufficient to ensure the compiler
does not emit a traceable function, I suspect you'll also want to mark
"native_get_debugreg" and "native_set_debugreg" always inline as well.

Thanks,

Mathieu

> +
> /*
>  * Our handling of the processor debug registers is non-trivial.
>  * We do not clear them on entry and exit from the kernel. Therefore
> @@ -718,28 +756,13 @@ static bool is_sysenter_singlestep(struc
> dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
> {
> 	struct task_struct *tsk = current;
> +	unsigned long dr6, dr7;
> 	int user_icebp = 0;
> -	unsigned long dr6;
> 	int si_code;
> 
> -	nmi_enter();
> -
> -	get_debugreg(dr6, 6);
> -	/*
> -	 * The Intel SDM says:
> -	 *
> -	 *   Certain debug exceptions may clear bits 0-3. The remaining
> -	 *   contents of the DR6 register are never cleared by the
> -	 *   processor. To avoid confusion in identifying debug
> -	 *   exceptions, debug handlers should clear the register before
> -	 *   returning to the interrupted task.
> -	 *
> -	 * Keep it simple: clear DR6 immediately.
> -	 */
> -	set_debugreg(0, 6);
> +	debug_enter(&dr6, &dr7);
> 
> -	/* Filter out all the reserved bits which are preset to 1 */
> -	dr6 &= ~DR6_RESERVED;
> +	nmi_enter();
> 
> 	/*
> 	 * The SDM says "The processor clears the BTF flag when it
> @@ -777,7 +800,7 @@ dotraplinkage void do_debug(struct pt_re
> #endif
> 
> 	if (notify_die(DIE_DEBUG, "debug", regs, (long)&dr6, error_code,
> -							SIGTRAP) == NOTIFY_STOP)
> +		       SIGTRAP) == NOTIFY_STOP)
> 		goto exit;
> 
> 	/*
> @@ -816,6 +839,7 @@ dotraplinkage void do_debug(struct pt_re
> 
> exit:
> 	nmi_exit();
> +	debug_exit(dr7);
> }
>  NOKPROBE_SYMBOL(do_debug);

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com