Date:   Fri, 17 Feb 2023 13:44:20 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Josh Poimboeuf <jpoimboe@kernel.org>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Chen Zhongjin <chenzhongjin@huawei.com>,
        "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Masami Hiramatsu <mhiramat@kernel.org>
Subject: Re: [PATCH 2/2] x86/entry: Fix unwinding from kprobe on PUSH/POP
 instruction
Message-ID: <Y+92pBgaH8VpYbsL@hirez.programming.kicks-ass.net>
References: <cover.1676068346.git.jpoimboe@kernel.org>
 <baafcd3cc1abb14cb757fe081fa696012a5265ee.1676068346.git.jpoimboe@kernel.org>
 <Y+4agrPvSUYhdaFT@hirez.programming.kicks-ass.net>
 <20230216160619.rleqhnrufcilizy2@treble>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230216160619.rleqhnrufcilizy2@treble>
Precedence: bulk

On Thu, Feb 16, 2023 at 08:06:19AM -0800, Josh Poimboeuf wrote:
> On Thu, Feb 16, 2023 at 12:58:58PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 10, 2023 at 02:42:02PM -0800, Josh Poimboeuf wrote:
> > 
> > > The problem is that #BP saves the pointer to the instruction immediately
> > > *after* the INT3, rather than to the INT3 itself.  The instruction
> > > replaced by the INT3 hasn't actually run, but ORC assumes otherwise and
> > > expects the wrong stack layout.
> > > 
> > > Fix it by annotating the #BP exception as a non-signal stack frame,
> > > which tells the ORC unwinder to decrement the instruction pointer before
> > > looking up the corresponding ORC entry.
> > > 
> > > Reported-by: Chen Zhongjin <chenzhongjin@huawei.com>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> > > ---
> > >  arch/x86/entry/entry_64.S | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> > > index 15739a2c0983..8d21881adf86 100644
> > > --- a/arch/x86/entry/entry_64.S
> > > +++ b/arch/x86/entry/entry_64.S
> > > @@ -385,7 +385,14 @@ SYM_CODE_END(xen_error_entry)
> > >   */
> > >  .macro idtentry vector asmsym cfunc has_error_code:req
> > >  SYM_CODE_START(\asmsym)
> > > -	UNWIND_HINT_IRET_REGS offset=\has_error_code*8
> > > +
> > > +	.if \vector == X86_TRAP_BP
> > > +		/* #BP advances %rip to the next instruction */
> > > +		UNWIND_HINT_IRET_REGS offset=\has_error_code*8 signal=0
> > 
> > So the fact that INT3 is trap like is not the problem, the problem is
> > that we use INT3 to overwrite stack modifying instruction and we should
> > not assume those instructions have completed when in the #BP handler.
> > 
> > Now, the reason all this actually works is because INT3 itself does not
> > modify the stack so rewinding on non-overwrite INT3 instructions is
> > invariant wrt stack state.
> 
> Right, that's what my patch description attempting to say.
> 
> That's also why I was asking about INT1, which is a trap.  Do we care
> about INT1?

We do not care about INT1, #DB is an IST and an allround pain in the
backside, INT3 is where it's at.