linux-kernel - Re: [PATCH 01/31] Add hard/soft lockup debugger entry points

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56AA7D14.5090407@ezchip.com>
Date:	Thu, 28 Jan 2016 15:41:56 -0500
From:	Chris Metcalf <cmetcalf@...hip.com>
To:	Jeffrey Merkey <jeffmerkey@...il.com>,
	<linux-kernel@...r.kernel.org>
CC:	<akpm@...ux-foundation.org>, <dzickus@...hat.com>,
	<uobergfe@...hat.com>, <atomlin@...hat.com>, <mhocko@...e.cz>,
	<fweisbec@...il.com>, <tj@...nel.org>,
	<hidehiro.kawai.ez@...achi.com>, <tglx@...utronix.de>
Subject: Re: [PATCH 01/31] Add hard/soft lockup debugger entry points

On 01/28/2016 02:46 PM, Jeffrey Merkey wrote:
> This patch series adds an export which can be set by system debuggers to
> direct the hard lockup and soft lockup detector to trigger a breakpoint
> exception and enter a debugger if one is active.  It is assumed that if
> someone sets this variable, then an breakpoint handler of some sort will
> be actively loaded or registered via the notify die handler chain.
>
> This addition is extremely useful for debugging hard and soft lockups
> real time and quickly from a console debugger.

I'm concerned that you are duplicating the breakpoint instructions
for all the platforms.  Could you make kgdb.h include kdebug.h and
just move the arch_kgdb_breakpoint() implementations in kgdb.h to
arch_breakpoint() in kdebug.h?  Then each platform can just put an
appropriate define in kgdb.h, e.g. "#define arch_kgdb_breakpoint
arch_breakpoint", unless (like mips) they have a more complicated
requirement.

I'm concerned that in some cases (e.g. arm64) there is a perfectly good
breakpoint defined in kgdb.h but you are providing a no-op in kdebug.h.
You should probably do another check across all the architectures for
this case.

You should probably add your no-op implementation of arch_breakpoint()
to asm-generic/kdebug.h, and then add "generic-y" lines to the Kbuild
files for the architectures that you are creating new empty files for.
I'm a little ambivalent about the "silent no-op" implementation, but I'm
not really sure there's a better option.

For mips, I'm pretty sure you don't want to create a global "breakinst"
symbol every time you insert a breakpoint into code.  I think this is an
example of where you need to have a different implementation of
arch_breakpoint() and arch_kgdb_breakpoint(), since mips makes its
breakpoint magical by knowing what the address used for that specific
instruction is, and you can't do that for arch_breakpoint().

As a general rule, you probably want to provide header guards in new
headers that you create, but if you just use asm-generic instead, it
actually won't matter for this case.

I should add that I didn't do a thorough review of the patch series,
just a quick skim of a few of the architectures.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com