linux-kernel - Re: [PATCH 01/13] RFC ONLY - kdb: core for kgdb back end

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: <20090509040942.GB8007@elte.hu>
Date:	Sat, 9 May 2009 06:09:42 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Jason Wessel <jason.wessel@...driver.com>
Cc:	linux-kernel@...r.kernel.org, kgdb-bugreport@...ts.sourceforge.net,
	kdb@....sgi.com, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 01/13] RFC ONLY - kdb: core for kgdb back end


* Jason Wessel <jason.wessel@...driver.com> wrote:

> This is an RFC patch.  This work is by no means in its final form, 
> nor is it in a form that would be suitible for upstream merging.  
> This is an early prototype of a kdb frontend talking to a kgdb 
> backend.  It is meant to foster some discussion around the 
> usefulness of merging kdb and kgdb together, as well as experiment 
> with changes to kgdb's core to improve robustness and 
> functionality.
> 
> This patch contains the kdb core and some instrumentation into the 
> core kernel which kdb requires in order to gather information for 
> some of its reporting functions.

Just a first quick 30-seconds impression from skimming through the 
patch:

 - The cleanups are an absolute must before doing any in-depth 
   review. We only want to waste valuable review bandwidth on code 
   that at least _looks_ nice and tidy.

 - Many functions are way too large, with many indentation levels - 
   they need a split-up.

 - Most of the code patterns dont match core kernel standards and 
   practices, so it's not reviewable in detail. It needs a 
   thorough clean-up not just on the surface, but on the algorithmic 
   level as well.

bits like:

> +		// HACK HACK HACK
> +		printk(KERN_CRIT "DOH NEED TO IMPLEMENT THIS!");

need fixed.

Locking needs reviewed and fixed:

> +/* Locking is awkward.  The debug code is called from all contexts, including
> + * non maskable interrupts.  A normal spinlock is not safe in NMI context.  Try
> + * to get the debug allocator lock, if it cannot be obtained after a second
> + * then give up.  If the lock could not be previously obtained on this cpu then
> + * only try once.
> + *
> + * sparse has no annotation for "this function _sometimes_ acquires a lock", so
> + * fudge the acquire/release notation.
> + */

Plus, if _any_ debugger front-end is considered for merging, it 
_must_ work with Kernel Mode Setting properly, out of X. No ifs
and when.

Also, high-level file organization: i'd suggest to move it all under 
the kernel/debug/ hierarchy, and move kernel/kgdb.c to 
kernel/debug/backend/core.c or so [possibly split up a bit, it's 
getting quite large] and the KDB bits under kernel/debug/frontend/. 
We dont want multiple back-ends nor multiple front-ends. We want one 
good back-end and one good (built-in) front-end.

I supported and helped a debugging backend and i dont consider a 
front-end completely impossible either. But it will have to meet a 
_lot_ of stringent standards because a good kernel debugging 
front-end's cross section to the system is even larger than a 
backend's. It's a tough job to get this done.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/