Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 20 Feb 2020 15:30:40 -0600
From:   Corey Minyard <minyard@acm.org>
To:     Will Deacon <will@kernel.org>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Corey Minyard <cminyard@mvista.com>
Subject: Re: [PATCH v2] arm64:kgdb: Fix kernel single-stepping
Message-ID: <20200220213040.GA2919@minyard.net>
Reply-To: minyard@acm.org
References: <20200219152403.3495-1-minyard@acm.org>
 <20200220142214.GC14459@willie-the-truck>
 <20200220163038.GJ3704@minyard.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200220163038.GJ3704@minyard.net>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Feb 20, 2020 at 10:30:38AM -0600, Corey Minyard wrote:
> On Thu, Feb 20, 2020 at 02:22:14PM +0000, Will Deacon wrote:
> > On Wed, Feb 19, 2020 at 09:24:03AM -0600, minyard@acm.org wrote:

snip...

> > > After fixing this and doing some more testing, I ran into another issue:
> > > 
> > > * Kernel enables the pt_regs single step
> > > * Kernel returns from the exception with ERET.
> > > * An interrupt or page fault happens on the instruction, causing the
> > >   instruction to not be run, but the exception handler runs.
> > 
> > This sounds like you've broken debug; we should take the step exception
> > in the exception handler. That's the way this is supposed to work.
> 
> Ok, here is the disconnect, I think.  If that is the case, then what I'm
> seeing is working like it should.  That doesn't work with gdb, though,
> gdb expects to be able to single-step and get to the next instruction.
> The scenario I mentioned at the top of this email.
> 
> Let me look at this a bit more.  I'll look at this on qemu and maybe a
> pi.
> 

Ok, this is the disconnect.  I was assuming that single step would stop
at the next instruction after returning from an exception.  qemu works
the same way the hardware I have does.  So I'm assuming arm64 doesn't
clear PTRACE.SS on an exception, even though that seems to be what the
manual says.

You can reproduce this by setting up kgdb on the kernel and hooking up
gdb, setting a breakpoint somewhere that has interrupts enabled, then
doing a "continue".  It will hit the same breakpoint again and again
because the PC doesn't get advanced by the single step and the timer
interrupt is always going to be pending.  I can do a more detailed set
of instructions with qemu, if you like.

I looked at kprobes a bit.  I don't think kprobes will have a problem
with this particular issue, it disables interrupts while single
stepping and doesn't allow a probe on any instruction that would modify
the interrupt settings.  I didn't look at page faults, but I assume that
it also won't allow a probe where there can be a page fault.

If a single-step is enabled and an exception occurs before the
instruction is executed, the single step is happening in the exception
handler when debug is re-enabled.  This what you are saying is how it is
supposed to work.

That's not what gdb is expecting, and that's not how x86 works, at
least.  I looked at ARM and MIPS and they don't even do single steps in
the kernel debugger.  PPC seems to work like x86 from code examination
and since our testers haven't reported this bug on that architecture.

I can do a patch that works sort of like kprobes, disabling interrupts
and simulating a single-step if the instruction modifies the daif bits.
Then you couldn't single step across an instruction that did a page
fault, but that's probably not a huge restriction.

I could modify the patch I have now to ifdef it out unless kgdb is
enabled.

I can do a patch that just pulls single step support out of the kgdb
interface for ARM64, since it doesn't work as gdb expects, anyway.  And
that's what ARM does.

It doesn't really matter to me.  I'm just trying to fix a bug that was
reported to me, and trying to get it upstream as a good citizen.  I don't
use kgdb.

-corey