linux-kernel - Re: [ANNOUNCE] mdb: Merkey's Linux Kernel Debugger 2.6.27-rc4 released

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: <48AD757A.8000608@s5r6.in-berlin.de>
Date:	Thu, 21 Aug 2008 16:02:34 +0200
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	jmerkey@...fmountaingroup.com
CC:	paulmck@...ux.vnet.ibm.com, Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Nick Piggin <nickpiggin@...oo.com.au>,
	David Howells <dhowells@...hat.com>
Subject: Re: [ANNOUNCE] mdb: Merkey's Linux Kernel Debugger      2.6.27-rc4
 released

jmerkey@...fmountaingroup.com wrote:
> It's simple to reproduce.  Take away the volatile declaration for the
> rlock_t structure in mdb-ia32.c (rlock_t debug_lock) in all code
> references and watch the thing lock up in SMP with multiple processors in
> the debugger each stuck with their own local copy of debug_lock.

I'm having a quick look at mdb-2.6.27-rc4-ia32-08-20-08.patch at the
moment.  Speaking of debug_lock()...:

You use spin_trylock_irqsave((spinlock_t *)&rlock->lock, rlock->flags)
in there.  Minor nit:  The pointer type cast is unnecessary.

Major problem:  rlock->flags is wrong in this call.  Use an on-stack
flags variable for the initial spin_trylock_irqsave.  Ditto in the
following call of spin_trylock_irqsave.

Next major problem with debug_lock() and debug_unlock():  The reference
counting doesn't work.  You need an atomic_t counter.  Have a look at
the struct kref accessors for example, or even make use of the kref API.
Or if it isn't feasible to fix with atomic_t, add a second spinlock to
rlock_t to ensure integrity of .count (and of the .processor if necessary).

Furthermore, I have doubts about the loop which is entered by CPU B
while CPU A holds the rlock.  You are fully aware that atomic_read(a) &&
!atomic_read(b) in its entirety is not atomic, I hope.

All this aside, I don't see *anything* in debug_lock and _unlock which
would necessitate volatile.  Well, volatile might have papered over some
of these bugs.

PS:
Try to cut down on #if/#endif clutter.  It should be possible to reduce
them at least in .c files; .h are a different matter.  For example,
#if MDB_DEBUG_DEBUGGER
	DBGPrint("something");
#endif
can be trivially reduced to
	dbg_mdb_printk("something");
where dbg_mdb_printk() is defined as an inline function which does
nothing when MDB_DEBUG_DEBUGGER is false.

PS2:
Why are there this many debug printks anyway?
-- 
Stefan Richter
-=====-==--- =--- =-=-=
http://arcgraph.de/sr/
--
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/