Date:   Thu, 16 Feb 2023 23:35:19 +0900
From:   Masami Hiramatsu (Google) <mhiramat@kernel.org>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Josh Poimboeuf <jpoimboe@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>, 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>
Subject: Re: [PATCH 2/2] x86/entry: Fix unwinding from kprobe on PUSH/POP
 instruction
Message-Id: <20230216233519.eacdf4166d4ec20f3046a1e2@kernel.org>
In-Reply-To: <Y+4T0EV8SBN09KxA@hirez.programming.kicks-ass.net>
References: <cover.1676068346.git.jpoimboe@kernel.org>
        <baafcd3cc1abb14cb757fe081fa696012a5265ee.1676068346.git.jpoimboe@kernel.org>
        <20230213234357.1fe194b2767d9bc431202d4c@kernel.org>
        <Y+tx6DZyoQ362lUM@hirez.programming.kicks-ass.net>
        <20230214170552.glhdytvunczyxxao@treble>
        <Y+yzMmL7gUprDru3@hirez.programming.kicks-ass.net>
        <20230215231637.laryjsua5p4wcd57@treble>
        <Y+4JhmEGDR16EVpi@hirez.programming.kicks-ass.net>
        <Y+4T0EV8SBN09KxA@hirez.programming.kicks-ass.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 16 Feb 2023 12:30:24 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Feb 16, 2023 at 11:46:30AM +0100, Peter Zijlstra wrote:
> > On Wed, Feb 15, 2023 at 03:16:37PM -0800, Josh Poimboeuf wrote:
> > > On Wed, Feb 15, 2023 at 11:25:54AM +0100, Peter Zijlstra wrote:
> > 
> > > > Well, I was specifically thinking about #DB, why don't we need to
> > > > decrement when we put a hardware breakpoint on a stack modifying op?
> > > 
> > > I assume you mean the INT1 instruction.  Yeah, maybe we should care
> > > about that.
> > 
> > Nah, I was thinking #DB from DR7, but ...
> > 
> > > I'm struggling to come up with any decent ideas about how to implement
> > > that.  Presumably the #DB handler would have to communicate to the
> > > unwinder somehow whether the given frame is a trap.
> > 
> > ... I had forgotten that #DB is not unconditionally trap :/ The worst
> > part seems to be that code breakpoints are faults while data breakpoints
> > are traps.
> > 
> > And you so don't want to go decode the DR registers in the unwinder,
> > quality mess this :/
> > 
> > Put a breakpoint on the stack and you've got PUSH doing a trap, put a
> > breakpoint on the PUSH instruction and you get a fault, and lo and
> > behold, you get a different unwind :-(
> 
> It could be I'm just confusing things... when #DB traps it is actually
> because the instruction is complete, so looking up the ORC based on the
> next instruction is correct, while when #DB faults, it is because the
> instruction has not yet completed and again ORC lookup on IP just works.
> 
> So while determining if #DB is trap or fault is a giant pain in the
> arse, it does not actually matter for the unwinder in this case.
> 
> And with the INT3 thing the problem is that we've replaced an
> instruction that was supposed to do a stack op.
> 

If the kprobe checks whether the original instruction do a stack op and
if so, setting a flag on current_kprobe will help unwinder finds that case?

Of course all INT3 user may need to do this but it should be limited.

Thank you, 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>