I sent Jan & Jason earlier a quick review of the kgdb code currently in git-x86#mm.

The kgdb code in git-x86#mm is right now is totally broken of course because the CFI
annotations in assembler code are gone now, but they are needed for gdb use. 
And the bogus fault handling code is still in there, but Jason apprently fixed that one.

Here are the other commments I wrote just in case someone is interested.
Overall I would say there is quite a lot of stuff to clean up still.

<snip>

ok, doing a review on the code in git-x86#mm. I have not read
everything in detail, just a quick skip over.

My personal test for clean kernel debugger integration is if the
debugger could be made a loadable (but not unloadable) module with
minor/no effort. How far away is your code from that?

The 32bit in_exception_stack code looks weird. Are you sure you cannot
just use the pt_regs passed to the die notifier?

It might be safer to explicitely disable interrupts early in the notifier.

You should probably use simple_strtoul() instead of inventing an
own hex parser in kgdb.c. And sprintf instead of an own hex writer.
In general more use sprintf would probably shorten a lot of the parser
code.

The num_online_cpu()s check in getthread() looks weird. What is that
supposed to do?

kgdb_softlock_notify: I think the right way to handle this is just
calling touch_softlockup_watchdog() (or perhaps better touch_nmi_watchdog)
Actually it should be only needed once when you exit the debugger
for soft lockup, but regularly for nmi watchdog.

On x86 it is ok, but on other architectures i'm suspect the SMP
synchronization with atomics might benefit from more memory barriers.

gdb_cmd_reboot: always calling emergency_sync looks dangerous.
In fact i suspect if you hold the other CPUs the changes are good
it will lock up. You should at least offer a option to not call that
(or better don't do it by default). Even machine_start is likely
lock up actually if the other CPUs are truly halted (as th...
[ message continues ]

..

Speaking of which.. the kernel implementation of snprintf() seems
to have a bug somewhere, in that it returns an incorrect count in
some situations -- mostly around where the buffer is too small to
hold the data being written.  There's an off-by-one bug there somewhere,
but I have not had time yet to track it down more precisely.

Cheers
--

This loss of CFI data is unfortunate for other potential consumers
too, such as systemtap, which intends to supplement its current stack
backtracing heuristics with CFI data.

- FChE
--

yes, but we removed those patches meanwhile because Roland McGrath has 
agreed to clean up the CFI mess. (Thanks Roland! We were very close to 
nuking them altogether because the unreadability they introduced was 
causing real bugs.)

[ btw., Andi's characterisation of it being "totally broken" is 
  incorrect and alarmist. It only means that the "backtrace" command 
  will not work in kgdb in certain types of contexts (only the current 
  function is displayed, not the parent function(s)), but there's no 
  "breakage", just less information available to the debugger. ]

in any case, while there are still holes in the frame annotations and it 
all needs a good bit of cleanup, that is a largely separate issue from 
whether kgdb is merge-worthy or not.

I'd like to thank Andi for his kgdb review efforts and comments. 
Unfortunately they were not done against kgdb-light which is being 
offered for upstream merge, but against an older version of kgdb. (Sorry 
about the 12 hour delay in my reply - Andi's mail did not have any of 
the kgdb or x86 persons Cc:-ed so it was not immediately noticed on a 
busy Monday, only in the lower-prio lkml scans.)

after a quick glance i'm happy to conclude that all but one of Andi's 
review comments were already addressed in kgdb-light -v7 AFAICS, and the 
only one not addressed (emergency_resync()) is now addressed in -v8.

the -v8 kgdb-light tree (against v2.6.25-rc1) can be pulled from:

   git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git

shortlog, diffstat and patch attached below. Tip of the -v8 tree is 
commit f20d70a6dd5785c9dd4c8215daf21f34365ce1c7.

In any case, if there are any open issues we are very much ready and 
willing to address them now or after any potential upstream merge of 
this codebase. This has meanwhile become one of the best-reviewed pieces 
of kernel code in living memory ;-)

	Ingo

------------------>
Ingo Molnar (3):
      pids: add pid_max prototype
      uaccess: add probe_kernel...
[ message continues ]

...dst and src are twisted. Fix below (should make sw-breakpoints work
again).

Jan


diff --git a/kernel/kgdb.c b/kernel/kgdb.c
index dce89d1..10fe113 100644
--- a/kernel/kgdb.c
+++ b/kernel/kgdb.c
@@ -153,14 +153,14 @@ int __weak kgdb_validate_break_address(unsigned long addr)
 {
 	char tmp_variable[BREAK_INSTR_SIZE];
 
-	return probe_kernel_read((char *)addr, tmp_variable, BREAK_INSTR_SIZE);
+	return probe_kernel_read(tmp_variable, (char *)addr, BREAK_INSTR_SIZE);
 }
 
 int __weak kgdb_arch_set_breakpoint(unsigned long addr, char *saved_instr)
 {
 	int err;
 
-	err = probe_kernel_read((char *)addr, saved_instr, BREAK_INSTR_SIZE);
+	err = probe_kernel_read(saved_instr, (char *)addr, BREAK_INSTR_SIZE);
 	if (err)
 		return err;
 

--

thanks, applied :-) Merged, uploaded the updated tree to the usual 
place. New tip of head is 59b64c80d706ae3504ccc76a9c00cdfea4a10701.

	Ingo
--

> after a quick glance i'm happy to conclude that all but one of Andi's 

At least the weird in_interrupt_stack() code seems to be still
in there.

I also still see a lot of open coded hex manipulation.

The synchronization code looks as bad as it was before.

The getthread magic pids are also still in there. What is that
good for?  

The notifiers are also still run with interrupts enabled,
so could in theory be preempted (only in some very exotic cases though)

The cpu hotplug races are still there I think

I have not rechecked everything else.

-Andi

--

i've uploaded kgdb-light -v9, it can be pulled from:

   git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git

find the shortlog, diffstat, full patch below.

There are a number of changes in -v9 - a patch from Jason to clean up 
kernel-messages-over-the-kgdb-console behavior and a bugfix from Jan.


fixed. (That code is gone now - the pt_regs passed in to the notifier 
points to the right kind of register context already, so the whole 
shadow task business on 64-bit was an unnecessary complication - as you 
suspected it. With it went another bunch of kernel/kgdb.c complication 

i reworked and cleaned up all the kgdb locking code completely. No more 
NR_CPUS spinlocks, there's now proper use of barriers, etc. No more 
silly "timeouts" for locking either ... There's now a very well-defined 
and clean locking state-machine for the primary CPU and the secondary 



previously Mark indicated some sort of sprintf return value breakage he 
observed, and kgdb would rely on sprintf return values so i'm inclined 

i fixed them too.

thanks for your suggestions, anything else you can think of?

	Ingo

------------------>
Ingo Molnar (3):
      pids: add pid_max prototype
      uaccess: add probe_kernel_write()
      x86: kgdb support

Jason Wessel (3):
      kgdb: core
      consoles: polling support, kgdboc
      kgdb: document parameters

 Documentation/kernel-parameters.txt |    5 +
 arch/x86/Kconfig                    |    1 +
 arch/x86/kernel/Makefile            |    1 +
 arch/x86/kernel/kgdb.c              |  555 ++++++++++++
 drivers/char/tty_io.c               |   47 +
 drivers/serial/8250.c               |   58 ++
 drivers/serial/Kconfig              |    3 +
 drivers/serial/Makefile             |    1 +
 drivers/serial/kgdboc.c             |  163 ++++
 drivers/serial/serial_core.c        |   70 ++-
 include/asm-x86/kgdb.h              |   81 ++
 include/linux/kgdb.h                |  271 ++++++
 include/linux/pid.h                 |    ...
[ message continues ]

First nobody answered the "kgdb clean enough for a module"

A timeout for waiting for other CPUs is actually not a bad idea for a 
debugger. After all you still want to debug even if some other CPUs


Pretty much all the proc output and sysfs show functions rely on these return 

The 64bit gdb actually supports segment registers:

static int amd64_linux_gregset64_reg_offset[] =
{
  RAX * 8, RBX * 8,             /* %rax, %rbx */
  RCX * 8, RDX * 8,             /* %rcx, %rdx */
  RSI * 8, RDI * 8,             /* %rsi, %rdi */
  RBP * 8, RSP * 8,             /* %rbp, %rsp */
  R8 * 8, R9 * 8,               /* %r8 ... */
  R10 * 8, R11 * 8,
  R12 * 8, R13 * 8,
  R14 * 8, R15 * 8,             /* ... %r15 */
  RIP * 8, EFLAGS * 8,          /* %rip, %eflags */
  CS * 8, SS * 8,               /* %cs, %ss */
  DS * 8, ES * 8,               /* %ds, %es */
  FS * 8, GS * 8,               /* %fs, %gs */
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1, -1,
  ORIG_RAX * 8

You silently overwrite any user ptrace hw breakpoints right? To do it cleanly 

All the kerneldoc comments are useless if you don't add the file

I don't think that case should happen during roundup for once. If it happens 
something is wrong.


That seems weird. I think other parts try to support user mode debugging
too. In theory there is no reason it shouldn't be able to do this


Ok I have not checked, but I hope you have strong protections against
reentry of the debugger just in case an exception here falls down to

That wrapper should not be needed; everybody can use instruction_pointer()




Hmm, so idle thread numbers depend on a sysctl? That seems weird.



It's still likely to deadlock on MP I think

[...] Haven't read further.

-Andi

--

this is kgdb-light, version -v10 (against Linus-latest), and can be 
pulled from:

   git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git

shortlog, diffstat and full patch can be found further below.

in -v10 i have depleted Andi Kleen's (known ;) pool of last-minute 

correct (just like SysRq-B can hang) - in -v10 i've fixed this by 

it's a sensible check, it validates a remote-provided "tid" against the 
range we accept. (It's a bit superfluous as find_task_by_pid() is safe 
enough.)


yes, i thought about that too yesterday, but was unsure how the rest of 
the code would code with that. It's no big issue and i have no strong 
feelings either way.


GDB wants to track threads, but on the Linux side each idle task has PID 

GDB simply needs an ID it can work with - there's nothing weird about 

as Roland explained it, the simplicity and of this code intentional.


That's wrong: in-source documentation is never useless. If it happens to 
meet DocBook style that's an added bonus. If the DocBook template is 

no, not all architectures have it. This is a weak alias that is 


The currently submitted code does not try to support user mode 
debugging. It might be a nice add-on later, if done cleanly and 

yes, this means if the user has configured kcrash as well then that will 

i disagree that a kernel debugger should necessarily be a kernel module. 
It might be nice at a future stage, but right now it would just 

i went for correctness and simplicity first. If a system is hung, the 
debugging CPU might hang too at any time. A timeout on the other hand 
introduces the possibility of a 'dead' CPU just coming back to life 
after the 'timeout', corrupting debugger data. So for now the rule is 
very simple.


you are wrong, as procfs does not rely on it for a protocol stack. It 
relies on it mostly for user-space "this many bytes were processed" 
return values and those values are often wrong and user-space just 
handles it gratiously.

Anyway,...
[ message continues ]

Hi,


  I am just trying to play with this, I am not able to make it work.
Assume I am able with git, I use it every day on my software, then I
am correctly tracking and building your branch on git.kernel.org.

My config has both KGDB and KGDB_SERIAL_CONSOLE. kernel command line
has kgdbwait kgdboc=ttyS0,115200. linux console is not on serial but if
do not enable KGDB_SERIAL_CONSOLE I am then unable to hit SysRq+g. If
instead I use linux serial console, gdb messes with getty and things
quickly get worse.

1. at boot, kernel waits for a gdb connection
2. gdb connection is established from another box
3. gdb breaks in
4. hit continue at gdb prompt
5. target box returns to life. everything looks ok (it is 2.6.25-rc1!)
6. SysRq+h on the target correctly shows the help on the console
7. SysRq+g on the target, it prints "SysRq : GDB" and hangs forever
8. gdb on the host does not break in

is really that simple, am i overlooking anything? thank you.

regards,
Domenico

#
# Automatically generated make config: don't edit
# Linux kernel version: 2.6.25-rc1
# Mon Feb 11 11:34:22 2008
#
CONFIG_PARISC=y
CONFIG_MMU=y
CONFIG_STACK_GROWSUP=y
CONFIG_RWSEM_GENERIC_SPINLOCK=y
# CONFIG_ARCH_HAS_ILOG2_U32 is not set
# CONFIG_ARCH_HAS_ILOG2_U64 is not set
CONFIG_GENERIC_FIND_NEXT_BIT=y
CONFIG_GENERIC_BUG=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_GENERIC_TIME=y
CONFIG_TIME_LOW_RES=y
CONFIG_GENERIC_HARDIRQS=y
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_IRQ_PER_CPU=y
CONFIG_ARCH_SUPPORTS_AOUT=y
CONFIG_DEFCONFIG_LIST="/lib/modules/$UNAME_RELEASE/.config"

#
# General setup
#
CONFIG_EXPERIMENTAL=y
CONFIG_LOCK_KERNEL=y
CONFIG_INIT_ENV_ARG_LIMIT=32
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
CONFIG_POSIX_MQUEUE=y
# CONFIG_BSD_PROCESS_ACCT is not set
# CONFIG_TASKSTATS is not set
# CONFIG_AUDIT is not set
CONFIG_IKCONFIG=y
# CONFIG_IKCONFIG_PROC is not set
CONFIG_LOG_BUF_SHIFT=15
# CONFIG_CGROUPS is ...
[ message continues ]

I doubt the .config you posted was the one you used, because there were
no KGDB options specified at all.

If you want to be using your vga device as the console that is ok. You
can still activate the sysrq with running the command: echo g >
/proc/sysrq-trigger

You might consider doing "set debug remote 1" in the gdb session if you
want to see what is going on. After the continue, the next time you
break in with the debugger it should send $T.........etc which is a stop
packet.

Jason.
--

indeed it was of a parisc.. :)

here is the right one:

#
# Automatically generated make config: don't edit
# Linux kernel version: 2.6.25-rc1
# Tue Feb 12 13:11:26 2008
#
# CONFIG_64BIT is not set
CONFIG_X86_32=y
# CONFIG_X86_64 is not set
CONFIG_X86=y
# CONFIG_GENERIC_LOCKBREAK is not set
CONFIG_GENERIC_TIME=y
CONFIG_GENERIC_CMOS_UPDATE=y
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_STACKTRACE_SUPPORT=y
CONFIG_HAVE_LATENCYTOP_SUPPORT=y
CONFIG_SEMAPHORE_SLEEPERS=y
CONFIG_FAST_CMPXCHG_LOCAL=y
CONFIG_MMU=y
CONFIG_ZONE_DMA=y
CONFIG_QUICKLIST=y
CONFIG_GENERIC_ISA_DMA=y
CONFIG_GENERIC_IOMAP=y
CONFIG_GENERIC_BUG=y
CONFIG_GENERIC_HWEIGHT=y
# CONFIG_GENERIC_GPIO is not set
CONFIG_ARCH_MAY_HAVE_PC_FDC=y
CONFIG_DMI=y
# CONFIG_RWSEM_GENERIC_SPINLOCK is not set
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
# CONFIG_ARCH_HAS_ILOG2_U32 is not set
# CONFIG_ARCH_HAS_ILOG2_U64 is not set
CONFIG_ARCH_HAS_CPU_IDLE_WAIT=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
# CONFIG_GENERIC_TIME_VSYSCALL is not set
CONFIG_ARCH_HAS_CPU_RELAX=y
# CONFIG_HAVE_SETUP_PER_CPU_AREA is not set
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
CONFIG_ARCH_SUSPEND_POSSIBLE=y
# CONFIG_ZONE_DMA32 is not set
CONFIG_ARCH_POPULATES_NODE_MAP=y
# CONFIG_AUDIT_ARCH is not set
CONFIG_ARCH_SUPPORTS_AOUT=y
CONFIG_GENERIC_HARDIRQS=y
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_X86_BIOS_REBOOT=y
CONFIG_KTIME_SCALAR=y
CONFIG_DEFCONFIG_LIST="/lib/modules/$UNAME_RELEASE/.config"

#
# General setup
#
CONFIG_EXPERIMENTAL=y
CONFIG_BROKEN_ON_SMP=y
CONFIG_LOCK_KERNEL=y
CONFIG_INIT_ENV_ARG_LIMIT=32
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_POSIX_MQUEUE is not set
# CONFIG_BSD_PROCESS_ACCT is not set
# CONFIG_TASKSTATS is not set
# CONFIG_AUDIT is not set
CONFIG_IKCONFIG=y
# CONFIG_IKCONFIG_PROC is not set
CONFIG_LOG_BUF_SHIFT=14
# CONFIG_CGROUPS is not set
# CONFIG_FAIR_GROUP_SCHED is not se...
[ message continues ]

Can't be very many because oprofile needs it and it works on most archs now.
Anyways, the right thing is to just add it to the architectures

Hmm, I'm pretty sure I saw some code that was only useful for the
user case. Perhaps you should take all that out then if it doesn't
work anyways?


Seems wrong intended behaviour to me. If kgdb is active it should have priority

The question is less about actually having it as a module, but 
just if the interfaces are clean enough to allow it as a module.

If all code is correct, it likely won't need a debugger.

Ok it just makes kgdb useless for a wide range of kernel problems.

If the return value of read() is wrong not even cat(1) will work
correctly, but lose bytes.

You're saying cat is not supposed to work on /proc? That's certainly

Sure a lot of ugly code works in practice. If it's clean and maintaintable 
code is another question of course.

-Andi

--

i'm glad that there are no other valid-looking (to me) objections left 
against kgdb-light -v10, other than "it would be nice to have more kgdb 

(It seems we do have a fundamental disagreement about how kgdb should 
act: in my opinion it should never break a correct system, while in your 
opinion apparently it's OK to compromise on that to make debugging 
easier. (find below more detailed arguments about this.) If you do not 
change your position about that issue we'll have to agree to disagree 
about that point.)

So unless i forgot about something (please yell if so), it seems to me 
kgdb is now pretty ready for an upstream merge.

here are my replies to the most recent points you brought up [in order 
of how i perceive the topic's importance]:


i gave you very specific technological reasons for why we dont want to 
do spinning for now: we dont _ever_ want to break a correctly working 
system with kgdb.

A valid counter-argument is _not_ to argue "but it would be nice to have 
if the system is broken in X, Y and Z ways" (like you did), but to point 
it out why the behavior we chose is wrong on a correctly working system.

Yes, a buggy system might misbehave in various ways but my primary 
interest is in keeping correctly working systems correct.

And note that kgdb is not just a "debugger", it's a system inspection 
tool. An intelligent, human-controlled printk. A kernel internals 
learning tool. An extension to the kernel console concept. Yes, people 
frequently use it for debugging too, but the other uses are actually 

but your contention is simply wrong. Most of our debugging 
infrastructure is non-modular for a good reason. Modularization 
increases complexity and that's exactly the wrong direction for 


it's not reimplemented - kgdb_arch_pc() does not directly map to 

that's already all taken out in -v10. (if you find any leftovers then 

that's the approach we are taking: be as unintrusive as possible. This 
means that the notifier here is registered at th...
[ message continues ]

This is not a technical argument, but I am not a big fan of hard hanging
the system if you cannot sync all the CPUs. The original intent was to
at least provide a sync error message to the end user after some
reasonable time. Then allow someone to collect any data you can get and
you basically have to reboot. The reboot was never forced, but assumed
the end users of this knew what they were doing in the first place.

Certainly in a completely working system where you use kgdb only for
inspection this is not an issue, unless you use a breakpoint or single
step one of the smp_call functions. As we all know there are lots of

We might be best served to add a comment to explain the purpose of
kgdb_arch_pc() and put it in the optional implementation function
headers in include/linux/kgdb.h

On some archs certain exceptions do not report the address that the
exception occurred at when you call instruction_pointer(). This optional
function allows for an arch to perform a "fixup" to get the address the
exception actually occurred at.

Kgdb requires the actual exception address so a sanity check can be
performed to make sure kgdb did not hit an exception while in a chunk of
code kgdb requires for its functionality. If you hit one of these
conditions kgdb makes its best attempt to try to "patch the wound"
inflicted by shooting yourself but at least you get notified vs a silent
hang :-)

Jason.
--

That was for the old longjmp checks, but that code is long gone isn't it

In what cases is that still needed?

-Andi
--

No this is a kgdb integrity check.

Basically when you reach this chunk of code it is before the hand off
to the source debugger.  We cannot continue because there was a
breakpoint in a part of the system kgdb was using while doing its
normal work.  The reality is that KGDB is not self contained.  It
relies on some external I/O methods, atomic page fault handling and
some other pieces.  If you take an exception there, the kgdb integrity
check absolutely needs to fail.

It was the source of some great pain for folks who tried to use kgdb
for more than an inspection tool.  There are some parts of the system


This is a totally stupid test case but it should illustrate the problem.

- connect to kgdb
- place a break point at kgdb_disable_hw_debug()
- continue
- Now try to break in again

You will see the kgdb integrity check fail because you put a
breakpoint in a place that kgdb actually needs to perform its work.
In this case kgdb will clean up the exception and restore operation
and you will be ok.  The kgdboc has a much smaller swath of code that
you cannot debug vs something like kgdboe where there are lot more
places you cannot debug due to the amount of code to services the
ethernet device, irq sync etc...

This check is absolutely required to help prevent silent death via
dumb breakpoints or stepping around in random places (which is ill
advised anyway).

Jason.
--

Don't you just need a simple recursion counter for this? 

I cannot think of a reliable simple way to check for this using the instruction
pointer. The only way would be to use explicit annotations for all
possible code similar to what kprobes does (__kprobes), but that would be 
hugely intrusive all over the tree.

With the recursion counter the only problem would be someone 
setting a break point on the early notifier code itself that contains
a recursion check, but that would be only a few lines of code
so the risk of someone setting a break point exactly there would

I believe you, but I don't believe that your implementation
of this handles all cases.

-Andi
--

It is more than a simple recursion check (which is already in the code)
because there are some conditions we can recover from.  I'd rather not
crash the system out if it can be recovered.

kgdb_reenter_check() has the "simple" recursion check with the
exception_level variable.

It also has several special checks against the breakpoint situation I
described before as well as trying to remove all the breakpoints.  
Ultimately if all those checks fails, it results in panic, because at
the end of the day that is all you can do if you re-enter the debugger
from the debugger (IE: you are toast).

I don't see any reason to change the code there unless it can be further
improved some way.  We want to fail loudly and verbosely when this kind
of thing happens so we can determine if it was the end user that caused
the problem or if there is a real defect.

Jason.
--

Ok I'm trying to understand the code as you describe it. As far 
as I can see (in kgdb-light-v10) it is:

+       addr = kgdb_arch_pc(ks->ex_vector, ks->linux_regs);
+       kgdb_deactivate_sw_breakpoints();
+
+       /*
+        * If the break point removed ok at the place exception
+        * occurred, try to recover and print a warning to the end
+        * user because the user planted a breakpoint in a place that
+        * KGDB needs in order to function.
+        */
+       if (kgdb_remove_sw_break(addr) == 0) {

and

+static int kgdb_remove_sw_break(unsigned long addr)
+{
+       int i;
+
+       for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
+               if ((kgdb_break[i].state == BP_SET) &&
+                               (kgdb_break[i].bpt_addr == addr)) {
+                       kgdb_break[i].state = BP_REMOVED;
+                       return 0;
+               }
+       }
+       return -ENOENT;

correct? 

I don't think that code does what you describe at all. Are you
sure we're talking about the same thing? 

There is certainly no real protection against break points in
debugger code in there as far as I can see (except for the reentry
counter)

-Andi
--

Perhaps we are talking about different things.  I will agree that there
is no protection against putting a breakpoint in some code that is part
of the debugger.  That was certainly not what I was talking about.

You might be able to perform some protections at the breakpoint set time
or request to single step, but that is much more complex than is worth
it for the time being.  The recursion check guards against basic
stupidity or accidental stepping out of a frame you didn't mean to. 
Also there are a lot of non-obvious code paths that can get executed via
kgdb such as the fault handlers.  The simple recursion check covers
enough cases that you don't want to live without it.

If you want to improve it please provide some patches or further
elaborate on what needs to be fixed.

Jason
--

I don't know -- I have not reread everything. Please don't consider
my comments as approval of the code base. I still think it does quite
a lot of dubious and ugly things overall and should get far more clean up

Stopping all CPUs for indefinite time very much seems like 
"breaking a correctly working system" to me.  In a correctly working system 

The only way I know of to do that is gdb vmlinux /proc/kcore 

For that gdb vmlinux /proc/kcore already works fine. Or fireproxy.

The main complexity in module handling is handling (or rather preventing)
module unload.  I explicitely excluded that in my earlier mail. 
Module loading on the other hand tends to be relatively easy.

I did a modular kernel debugger on my own some time ago and once
the interfaces were clean it was very simple. I think the reverse
is true too -- if having it as a module is easy then the interfaces
are clean too. That is why I asked for it. It's a good basic

If that is true then it is definitely misnamed and likely 

Yeah, it is consistently wrong agreed.

-Andi

--

well, this is a small detail, but still you are wrong, and on a 
correctly working system this will not occur. (if yes, tell me how)

KGDB does a very straightforward "all CPUs enter controlled state" 
transition when the session begins, and at the end an "all CPUs 
continue" transition.

I'm not sure what you mean exactly under "stopping all CPUs for 
indefinite amount of time" (your statement is sufficiently vague to be 
hard to counter via specifics) - that does not happen, unless the system 
is so buggy that a CPU is not able to process an NMI anymore [which is 
rather rare] - in that case the whole system is likely locked up anyway. 
In that case the simplest and safest behavior is what kgdb-light does 
currently: it will only proceed if all CPUs have responded. Note that 
you are wrong to suggest that "KGDB locks up", the system _has already 
locked up_.

yes, we could "time out" and force a KGDB session even if some CPUs do 
not respond. But it's obviously not a completely safe system state, 
because other CPUs might be changing things under the feet of the 
debugger. So the safest first-level approach is to not enter the 
debugger in this case.

	Ingo
--

Quite frankly, I don't see why the kernel kgdb layer should have *any* 
code like this at all.

The one who is actually debugging is the one who should decide which CPU's 
get stopped, and which don't.

I realize that the gdb remote protocol is probably a piece of crap and 
cannot handle that, but hey, that's not my problem, and more importantly, 
I don't think it's even a *remotely* valid reason for making bad decisions 
in the kernel. gdb was still open source last time I saw, and I think it's 
reasonable to just say:

 - the kgdb commands should always act on the *current* CPU only
 - add one command that says "switch over to CPU #n" which just releases 
   the current CPU and sends an IPI to that CPU #n (no timeouts, no 
   synchronous waiting, no nothing - it's like a "continue", but with a 
   "try to get the other CPU to stop"

Yes, other CPU's will obviously often end up stopping due to waiting for 
some spinlock or other if we stop one, but that's a separate issue, and 
quite often it might be sufficient - and what we want.

And yes, you'd likely have to add some support to gdb to make this 
_usable_, but now all that usability crap, all those timeouts for "stop 
all CPU's" are now in user space on the _debugger_ side. That can be as 
fancy as it wants to be.

And maybe this isn't realistic. I'm not saying "we _must_ do it this way", 
I just want to say that the kernel kgdb layer should be as thin ass 
humanly possible, and maybe the right thing to do is to simply totally 
punt on the whole "stop other cpu's" issue and make it a debugger-side 
question.

In other words, is it perhaps possible to just *get*rid*of* that 
"kgdb_active" and "nmicallback" and the whole multi-CPU roundup? Just use 
a kgdb spinlock around the stuff that actually sends and receives 
individual packets, and expect the debugger side to sort them out (yeah, I 
suspect this involves having to add the CPU ID to each packet).

			Linus
--

The problem I see here is that the kernel tends to get badly confused
if one CPU just stops responding. At some point someone does an global
IPI and that then hangs.  You would need to hotunplug the CPU which
is theoretically possible, but quite intrusive. Or maybe the "isolate CPUs 
in cpusets" frame work someone posted recently on l-k could be used.  Still 
would probably have all kinds of tricky issues and races.

-Andi

--

Yes.  A stopped CPU is very visible and hence can change the behaviour of

I don't think you'd want to be poking around in kernel internals while some
of the CPUs are continuing to run.  It sounds rather creepy.  You want
everything to stop.  Including time-related things.

Bear in mind that one of the things you do with kgdb is to modify kernel
memory - I'd do things like

int foo;

	...
	if (foo == 1)
		special_stuff();
	...

to trigger a particular behaviour at a particular time.  If you're making
multiple changes, you want them "atomic" wrt all CPUs.  (Of course, if you
happeed to breakpoint one CPU while it was partway through reading multiple
locations, you lose.  But that's a teeny window).



OT: another thing you can do with kgdb is error-path testing:

	foo = kmalloc(...)
BP->	if (!foo)
		recover();

put a breakpoint on the !foo test and set foo to zero by hand.
--

Hi -


Just for completeness, keep in mind that one can already 

If "foo" is a global within a particular compliation unit, any old
function in that CU can be probed to set/get the global.  (Setting
incoming function parameters works too.)

# cat delayed-set.stp
   # set a systemtap script variable based on a /proc control file
   probe procfs("activate").write { setit = $value }
   global setit
   # check the systemtap global in order to set the kernel global
   probe kernel.function("foo_checking_fn") { 
      if (setit=="1") { setit = ""; $foo = 1 }
   }
# stap -g -m ds delayed-set.stp &

# cat setit2.stp
  probe kernel.statement("*@dir/function.c:222") { /*if (desired)*/ $foo = 0 }
# stap -g setit2.stp &


- FChE
--

You don't even need systemtap: it has been always possible with
gdb -write vmlinux /proc/kcore

The only value add of the kgdb stub is really that you can single
step too and do post mortem (although the later works fine using
kdump/crash these days too)

-Andi
--

Note I wrote the one above only as a reply to Linus' proposal, not
because I was advocating "live debugging" (or rather I think if you want
that just use gdb vmlinux /proc/kcore) 

I agree with you in principle, but what do you do if one of the CPUs doesn't 
answer? 

Ingo seems to think it's ok for the debugger then to just hang too,
I think it should eventually time out and debug anyways.

Also there are some limits on how much the system should be frozen
down. For example if you're strict about this requirement you
would need to require full DMA quiescence (like kexec does). But that's
obvious not a good idea for the debugger. So it's always shades
of gray, not black/white.

What I find strange with the current patch is that it goes to extreme
measures to stop the CPUs (will hang forever), but not do the whole
thing (DMA quiescence too) 

-Andi

--

Andi, please refrain from misrepresenting my position, i can speak for 
myself.

As i stated it before, i'm not against adding some configuration on 
roundup behavior as long as it's clearly add-on optional system policy. 
In fact i already implemented that in the kgdb-light tree.

But if your goal is to create smoke where there is none, then feel free 
to continue to beat down on this poor strawman, ok? ;-) Meanwhile we'll 
continue to clean up kgdb to make it ready for an eventual (and IMO well 
deserved) .26 merge. All the useful bits of your comments have made kgdb 
cleaner - thanks for that. But please lets keep this constructive. 
Thanks,

	Ingo
--

You're thinking about this totally *wrong*.

You definitely do not want to hot-unplug or isolate anything at all. 
That's explicitly against the whole point of kgdb not changing what it is 
trying to measure.

Just let the other CPU's hang naturally if they need to wait for IPI's 
etc. What's the downside? That's what you were trying to do in the first 
place by havign the kgdb callback!

So you can't have it both ways. Either serializing other cpu's with kgdb 
is good (the whole "kgdb_nmicallback" thing or whatever it was called), in 
which case it's also perfectly ok to just let them stop when waiting for 
IPI's.

My point was *not* that kgdb should take control of one CPU, and the other 
CPU's should continue to work as if nothing happened. That is insane and 
impossible (since you may be stopping a CPU while it holds central 
spinlocks etc). No, my point was that I think kgdb should be as light and 
non-intrusive as possible, and that any "higher level behaviour" (like the 
decision of whether to try to synchronize other CPU's or not) should be 
left to the debugger.

But only if that makes kgdb patches less intrusive!

In other words, I'm not at all trying to push any particular solution 
here, except for the "keep it simple, and anything even remotely debatable 
or intrusive to the system should be excised". And I wanted to point out 
that maybe all these timeout etc decisions can be pushed to the debugger.

So I think we can either:

 - have no timeouts or other fancy crap _at_all_, with very simple locking 
   (ie looks what v10 mostly seems to do)

 - or you do the fancy dance entirely in the remote debugger.

I don't care. The only thing I care about is that kgdb support never 
_ever_ shows up in any interesting code, and that it remains totally 
invisible to essentially all of the kernel except the place that would 
otherwise print out an oops.

And I absolutely don't want it to be fancy, I want it to be so simple that 
even _I_ can look at it and say "I...
[ message continues ]

I agree that it wouldn't be a good idea -- i was just pointing out

There tend to be timeouts (e.g. softlock/nmi watchdog at least). I think
some of the IPIs eventually time out too.  In general losing a lot 
of time can lead to weird side effects.

-Andi
--

I do agree that kgdb and watchdogs aren't like to work well together. And 
I don't think you can sanely expect to not have a "disable lockup 
detection" when you start poking around with kgdb.

We also just have to expect that time will also stop while sometbody is 
messing around with kgdb.

So I don't dispute that any kernel debugger will *always* be intrusive in 
those ways. That's pretty inevitable. I just think the code itself can try 
to avoid hooking into various places all over the map.

			Linus
--

i actually think that the notion of "stopping all system state" is 
rather intuitive from a debugging POV: when you have a bug trigger 
somewhere then getting an NMI to all CPUs and stopping them dead in 
their tracks preserves us the system in its most useful state.

also, when using kgdb to "look at system state" it's best to have as 
little "unrelated" activity as possible.

the timeout argument brought up by Andi was IMO really just pulled here 
by its hair, it never happened to me in practice and i was surprised it 
even came up. (i never before saw "KGDB roundup" fail - and i tried it 
on an 8-way system too) I think the memory-access routine cleanups were 
far more interesting from a failure scenario POV. Maybe Andrew could 
shed some light on his experience and preference here - he's been using 
KGDB for many years.

	Ingo
--

on a second thought - i actually think it's rather possible and 
straightforward to do what you suggest. Stopping of all CPUs is still 
useful, but should be an optional property. I'll play with this a bit 
and see how GDB reacts.

	Ingo
--

Stopping with interrupts off. Nothing scheduled anymore. 

An easy definition for the condition is anything that requires 
touch_{nmi,softlockup}_watchdog [which kgdb definitely does, 

While that is a slight risk that problem is already there anyways.
Lots of agents in the system could do that. Do you plan to stop
all DMA too for example if you're so worried about this? 
Or how about SMM code changing something? 

Anyways the slight risk of the other CPUs eventually recovering
would seem a acceptable trade off versus not being able to use
the debugger to debug the system with hanging CPUs. 

A possible compromise between my and your position on this would 
be also having an option for this, with default to off
(although I would expect that would be a inconvenient default for
many people)

-Andi (who retires from this thread now, I already spent too much on this)

--

Quite frankly, if kgdb starts doing somethign "fancy", there is no way 
I'll merge it.

This includes things like having "breakpoint reservations" (discussed 
earlier) and just generally trying to add lots of infrastructure to make 
kgdb "fit in" to the kernel.

The fact is, not having kgdb enabled should have _zero_ impact on the 
kernel, but the implication is also that when you *do* enable kgdb, that 
shouldn't change anything visible either: the rest of the kernel should 
simply not _possibly_ be able to even tell or care (which is why 
breakpoint reservations are out - they are against the whole point of 
debugging a unmodified kernel).

So kgdb in my opinion will *have* to step on some other kernel 
infrastructure. I also think that things like timeouts are not something 
the client should do - if a kgdb lock never gets through, then it should 
be the debugging end that should just react to ^C and tough titties: it's 
not like there's a whole lot to be done.

In other words: I think the kgdb patches have become a lot more palatable 
over the last week, but I think so exactly because they have gotten 
smaller and less invasive. Anything that evokes any discussion AT ALL 
should just be removed. It really should be that simple.

[ The exception being that I think hw breakpoint support should be added 
  back in - never mind that it won't work if the "native kernel" also uses 
  them. Tough.

  If the debugger screws up the hw breakpoint state, that's a debugger 
  error. I'd hope that the remote debugger *defaults* to just re-writing 
  the instruction stream for that reason, but if you want to do a data 
  breakpoint, you simply *have* to use the hw breakpoints for kgdb.

  And you just need to live with the fact that it simply won't be possible 
  to do if the client wants to use hw breakpoints too. If the client 
  starts using hw breakpoints - tough titties, it takes them. ]

So keep the damn thing really simple, and don't try to handle every 
possible thing. W...
[ message continues ]

HW breakpoints (particularly data breakpoints) are extremely useful
and I am proposing to put them back into kgdb-light. The HW
breakpoint patch worked exactly as described. If the user space
writes into the HW breakpoint registers via ptrace, those take
priority over the kernel HW breakpoints.

We have to assume that the person debugging the kernel ultimately has
full control of the system, and will do what ever is needed to
debug/inspect the kernel how he or she chooses.

Of course at some future time if there is a generic API to use for the
kernel level HW breakpoints, kgdb can adapt like anything else.

Jason.
--

yes, that's what i've been doing. When anything became questionable even 
just a little bit, it was the axe or i changed it to something really 
obvious and correct.

by 2.6.26 kgdb-light will become so neutral to the rest of the kernel 

yeah. I believe we need to achieve a "known zero impact, 100% trustable" 

yes - if something locks up, it will be the NMI watchdog starting the 
debugger eventually - not the other way around. The NMI watchdog is 
already system policy which can be turned on/off and is well known and 
expresses the user's wishes with what should happen to the system.

The debugger should really be a fundamentally very passive "console" 
type of thing, with as little direct policy as possible. That minimizes 
the chance that it accidentally messes up something and also makes it 
more likely that it will actually work reliably and dependably.

	Ingo
--

I think that part is actually mostly ok now (old kgdb stubs were
much worse in this regard) 

I still think the ultimative proof for this would be working

While agreed in theory there are some exceptions: any time an oops
happens or a crash dump would happen automatically kgdb should definitely
do something very visible. Unfortunately that's not always the case

The problem here is that the timeout we were talking about 
is not integrated into the kgdb event loop, it is rather the loop
that protects the event loop. If you hang there all input

If you want areally simple kgdb the best option would be re-porting
the really simple (tm) 2.4 era x86-64 kgdb stub I did long ago.
It had a small fraction of the code size of the current code, but
was also missing a lot of features of course.

-Andi
--

To pick up this idea again I did the experimental patch below. It
applies against Jason's latest kgdb-light patch queue:

http://git.kernel.org/?p=linux/kernel/git/jwessel/linux-2.6-kgdb.git;a=shortlog;h=for_...

The patch nicely demonstrates what deeper dependencies on kernel
services currently exist in kgdb-light. The following symbols were
unresolvable:

 o genapic - for send_IPI_allbutself, ie. CPU roundup
 o machine_emergency_restart - for implementing "R0" gdb packet
 o idle_task - kgdb tells the per-cpu idle tasks apart
 o clocksource_touch_watchdog - obvious

For simplicity reasons I just gpl-exported all of them. The result is
a fully modular kgdb that may help to reduce concerns regarding its
intrusiveness.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

---
 arch/x86/kernel/Makefile               |    4 +++-
 arch/x86/kernel/genapic_64.c           |    1 +
 arch/x86/kernel/{kgdb.c => kgdb-x86.c} |    0 
 arch/x86/kernel/reboot.c               |    1 +
 arch/x86/mach-generic/probe.c          |    1 +
 kernel/Makefile                        |    1 -
 kernel/sched.c                         |    1 +
 kernel/time/clocksource.c              |    1 +
 lib/Kconfig.kgdb                       |    2 +-
 {kernel => lib}/kgdb.c                 |    8 ++++++--
 10 files changed, 15 insertions(+), 5 deletions(-)
 rename arch/x86/kernel/{kgdb.c => kgdb-x86.c} (100%)
 rename {kernel => lib}/kgdb.c (100%)

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 4cd39cd..2e733b1 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -58,7 +58,6 @@ obj-$(CONFIG_MODULES)		+= module_$(BITS).o
 obj-$(CONFIG_ACPI_SRAT) 	+= srat_32.o
 obj-$(CONFIG_EFI) 		+= efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_DOUBLEFAULT) 	+= doublefault_32.o
-obj-$(CONFIG_KGDB)		+= kgdb.o
 obj-$(CONFIG_VM86)		+= vm86_32.o
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 
@@ -79,6 +78,9 @@ endif
 obj-$(CONFIG_SCx200)		+= scx200.o
 scx200-y...
[ message continues ]

Very nice! If it's that simple then the kgdb integration is really


I would rather export some generic wrapper for that than the full genapic

Hmm, might be a bit dangerous to call this directly -- there are various
quirks with e.g. not rebooting on CPU #0 and not resetting APIC 
state. But ok [this is not directly related to the fact that it's
exported now, just mentioning this in general]

-Andi

--

kgdb was never expected to be any better than using a sysrq-b.   Unless
you put some really ugly code in to deal with lots of edge cases there
are certainly times it is just going to force you to press the power
button.  If there is a better function for "reset now and cross fingers"
kgdb can be changed easily enough to do that.

Also given that you can have more than one type of "R" packet it would
be easy enough to add other reboot types, such as an R1 which might call
to the bios or another means to reset.  For now the intent is pretty
clear and there is not likely a need to expand the scope.

Jason.
--

see? There's the difference between us. The initial merge of KGDB does 
not want to include policies for nuances that pose an "acceptable slight 
risk".

Had we done it, you could have (rightfully in that case, i have to say) 
complained about that "slight but real danger" of a debugger activating 

yes, i said this the first time you mentioned this issue that while it's 
all no big deal, it could perhaps be a sensible (but default-off) 
layered-on option for later. But the initial merge is for obvious stuff 
and it does not want to do such things.

	Ingo
--

Ok fine. Please implement DMA quiescence on kgdb entry then; otherwise
the code won't conform to your standards at all.

-Andi
--

well, it does not take a mindreader to conclude that nothing we do could 
possibly result in you "concluding" that kgdb would be ready for a merge 
;-)

for me it suffices that you've run out of technical arguments and are 

	Ingo
--

Let's put it like this: the amount of problems I found by reading
only about 20% of the kgdb patch justifies the statement above. Also
contrary to your repeated claims many of those are not fixed yet
as far as I can see.

Also every time I dig into something new problems raise their
head -- e.g. see earlier exchange with Jason about the likely
bogus recursion check.

Overall that's not a sign of a clean merge ready code base.

-Andi
--

There was work underway on that before (hw_breakpoint).  I'm not entirely
sure you want to use fancy stuff like that in kgdb.  It's nice for kgdb to
be as self-contained as possible, so you can debug everything else with it.
If you're going to rely on lots of higher-level infrastructure, it could be
using kprobes for its software breakpoint handling, too.

At any rate, I think it would be good if the hw breakpoint support in kgdb
were chopped out into a separate patch.  First make kgdb work with no code
touching debug registers at all.  Then a second patch can add the hw
breakpoint support.  The latter one can be mulled over or replaced with a
hw_breakpoint merge or just put off for a while because it's a big can of
worms to multiplex the use of the debug registers.


Thanks,
Roland
--

agreed, and i've done that now. Thanks,

	Ingo
--

1) The content is valid no matter the formatting
2) It is a well known format
3) And it is widely used

Using a wellknown format does not require it to be postprocessed.
It is by far better than "one-format-for-each-developer".

	Sam
--

They seem to have reverted these patches now, so that problem 
is gone.

Yes there are a couple of other consumers of CFI too, like crash
or the out of tree (but not dead) in kernel unwinder.

-Andi
--