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;
MIME-Version: 1.0
References: <20200716151943.2167652-1-daniel.thompson@linaro.org>
 <20200716151943.2167652-4-daniel.thompson@linaro.org> <CAD=FV=UBMa4pt7Xg2sKXHdjo6tVaQWBQFtT_aoDJ+Xt1HrPD+Q@mail.gmail.com>
 <20200720081323.5f3xojhu6v23jgqi@holly.lan>
In-Reply-To: <20200720081323.5f3xojhu6v23jgqi@holly.lan>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 21 Jul 2020 14:22:08 -0700
Message-ID: <CAD=FV=VipxFspgHJZ0uKURyfE4e3GB-w3gyJZdRn5CF9aLT8fg@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] kgdb: Add NOKPROBE labels on the trap handler functions
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Jason Wessel <jason.wessel@windriver.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Sumit Garg <sumit.garg@linaro.org>,
        Petr Mladek <pmladek@suse.com>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Will Deacon <will@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        kgdb-bugreport@lists.sourceforge.net,
        LKML <linux-kernel@vger.kernel.org>,
        Patch Tracking <patches@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Mon, Jul 20, 2020 at 1:13 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Fri, Jul 17, 2020 at 03:39:58PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > >
> > > Currently kgdb honours the kprobe blocklist but doesn't place its own
> > > trap handling code on the list. Add labels to discourage attempting to
> > > use kgdb to debug itself.
> > >
> > > These changes do not make it impossible to provoke recursive trapping
> > > since they do not cover all the calls that can be made on kgdb's entry
> > > logic. However going much further whilst we are sharing the kprobe
> > > blocklist risks reducing the capabilities of kprobe and this would be a
> > > bad trade off (especially so given kgdb's users are currently conditioned
> > > to avoid recursive traps).
> > >
> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > ---
> > >  kernel/debug/debug_core.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> >
> > I could just be missing something, but...
> >
> > I understand not adding "NOKPROBE_SYMBOL" to generic kernel functions
> > that kgdb happens to call, but I'm not quite sure I understand why all
> > of the kdb / kgdb code itself shouldn't be in the blocklist.  I
> > certainly don't object to the functions you added to the blocklist, I
> > guess I'm must trying to understand why it's a bad idea to add more or
> > how you came up with the list of functions that you did.
>
> Relatively early in the trap handler execution (just after we bring the
> other CPUs to a halt) all breakpoints are replaced with the original
> opcodes. Therefore I only marked up functions that run between the trap
> firing and the breakpoints being removed (and also between the
> breakpoints being reinstated and trap exit).

Ah, OK!  Could that be added to the commit message?

Also, shouldn't you mark kgdb_arch_set_breakpoint()?  What about
dbg_activate_sw_breakpoints()?  I haven't gone and extensively
searched, but those two jump out to me as ones that were missed.

I suppose that means that if someone tried to set a breakpoint on a
kgdb function that wasn't one of the ones that you listed then the
system would happily report that the breakpoint has been set (no error
given) but that the breakpoint would just have no effect?  It wouldn't
crash (which is good), it just wouldn't detect that the breakpoint was
useless.  However, if these were in the NOKPROBE_SYMBOL then you'd get
a nice error message.  Is there no way we could use a linker script to
just mark everything using a linker script or somesuch?

-Doug