Hi Ingo,
Please find the revised set of patches that implement Hardware
Breakpoint (or watchpoint) registers and an arch-specific implementation
for x86/x86_64.
Changelog
---------
The previous version of these patches were submitted through
http://article.gmane.org/gmane.linux.kernel/794870. The patches now
contain a new ftrace plugin for kernel symbol tracing using hardware
breakpoint registers. The patches are now re-based to -tip tree commit
e40aa382597b30c4d702951348500e812b4cb3d0.
A small usage note on the plugin along with the description of the
breakpoint interfaces is included below.
Kindly accept these patches to become a part of -tip tree.
Description
-------------
The Hardware Breakpoint registers can be used for tracing changes to a
variable or data location (even I/O ports in x86/x86_64) and will be
extremely helpful in debugging problems such as memory corruption. While
these registers have been used by user-space debuggers for long (through
'ptrace' syscalls), Kgdb exploited them for kernel-space addresses.
The proposed framework, introduces interfaces to use them directly on
both user- and kernel-space addresses apart from arbitrating requests
from various such users for the limited number of registers.
The interfaces are:
int register_kernel_hw_breakpoint(struct hw_breakpoint *);
void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp);
int register_user_hw_breakpoint(struct task_struct *tsk,
struct hw_breakpoint *bp);
void unregister_user_hw_breakpoint(struct task_struct *tsk,
struct hw_breakpoint *bp);
The 'struct hw_breakpoint' will be the anchor data-structure containing
all the necessary information such as name or address, type, length,
priority and pointers to handler functions (some of which are
arch-specific). More information about the role of each field, the
handler
functions and their return values can be found in the descriptive
comments
preceding these functions and in "include/asm-generic/hw_breakpoint.h".
While (un)register_user_hw_breakpoint() isn't exported yet, its
worker-routine __register_user-hw_breakpoint() is used by ptrace syscall
for all breakpoint register requirements. For the kernel-space, a simple
use case to trace 'write' operations on a kernel variable can be found
in samples/hw_breakpoint/data_breakpoint.c.
In the current patchset, support is provided only for read and
read-write breakpoints on data locations, which can be later expand to
include instruction and I/O breakpoints for x86/x86_64.
There is pending integration with 'KGDB' without which mutual exclusion
between them (KGDB and HW breakpoint use through above interfaces) needs
to be observed.
Ftrace plugin
-------------
Usage
------
mount -t debugfs debugfs /debug
cd /debug/tracing/
echo 0 > tracing_enabled
cat available_tracers
echo ksym_tracer > current_tracer
echo "pid_max:rw-" > ksym_trace_filter
echo "jiffies:-w-" > ksym_trace_filter
echo 1 > tracing_enabled
cat trace
Sample Output (snipped)
-------------
[root@mymachine tracing]# cat trace
# tracer: ksym_tracer
#
# TASK-PID CPU# Symbol Type Function
# | | | | |
atieventsd 3053 1 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5394 1 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5394 0 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5394 0 pid_max RW alloc_pid+0x6c/0x2fd
bash 4898 1 pid_max RW alloc_pid+0x6c/0x2fd
atieventsd 3053 1 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5401 1 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5413 0 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5415 0 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5415 0 pid_max RW alloc_pid+0x6c/0x2fd
authatieventsd. 5413 0 pid_max RW alloc_pid+0x6c/0x2fd
hald-runner 2766 0 pid_max RW alloc_pid+0x6c/0x2fd
hal-system-kill 5425 1 pid_max RW alloc_pid+0x6c/0x2fd
hal-system-kill 5425 1 pid_max RW alloc_pid+0x6c/0x2fd
swapper 0 0 jiffies W do_timer+0x16/0xb0
swapper 0 0 jiffies W do_timer+0x16/0xb0
gnome-terminal 4521 0 jiffies W do_timer+0x16/0xb0
Xorg 3235 0 jiffies W do_timer+0x16/0xb0
Xorg 3235 0 jiffies W do_timer+0x16/0xb0
[root@mymachine tracing]#
Thanks,
K.Prasad
* [email protected] <[email protected]> wrote:
> Hi Ingo,
> Please find the revised set of patches that implement
> Hardware Breakpoint (or watchpoint) registers and an
> arch-specific implementation for x86/x86_64.
General structure looks good, with a good deal of details
that need to be addressed.
Firstly, as far as i can see this should work on 32-bit too,
correct?
Secondly, what about other architectures - will they build just
fine without any arch level glue code? kernel/hw_breakpoint.o
get build unconditionally - without any benefit to non-x86 code.
Perhaps an ARCH_HAS_HW_BREAKPOINTS Kconfig method would be
useful to add.
There's also a number of (small) style issues.
kernel/hw_breakpoint.c and other new .c files dont comply to the
customary comment style of:
/*
* Comment .....
* ...... goes here:
*/
also, the #include files section style should match that of
arch/x86/mm/fault.c - it's a conflict-avoidance style.
also, things like this:
static struct notifier_block hw_breakpoint_exceptions_nb = {
.notifier_call = hw_breakpoint_exceptions_notify,
.priority = 0x7fffffff /* we need to be notified first */
};
should be:
static struct notifier_block hw_breakpoint_exceptions_nb = {
.notifier_call = hw_breakpoint_exceptions_notify,
/* We need to be notified first: */
.priority = 0x7fffffff,
};
Ingo
There's also a few checkpatch warnings that need to be
addressed:
ERROR: do not use assignment in if condition
#1084: FILE: arch/x86/kernel/ptrace.c:581:
+ else if ((thbi = alloc_thread_hw_breakpoint(tsk)) ==
NULL)
WARNING: suspect code indent for conditional statements (16, 16)
#2836: FILE: kernel/trace/trace_ksym.c:324:
+ if (strncmp(entry->ksym_hbkpt->info.name,
KSYM_SELFTEST_ENTRY,
[...]
+ kfree(entry->ksym_hbkpt->info.name);
Ingo
On Tue, 10 Mar 2009, Ingo Molnar wrote:
>
> There's also a few checkpatch warnings that need to be
> addressed:
>
> ERROR: do not use assignment in if condition
> #1084: FILE: arch/x86/kernel/ptrace.c:581:
> + else if ((thbi = alloc_thread_hw_breakpoint(tsk)) ==
> NULL)
Changing this to remove the assignment from within the "if" condition
would make the code less readable, not more. It's part of a long
series of tests; in schematic form:
if (n == 4 || n == 5)
...
else if (n == 6) {
...
}
else if (!tsk->thread.hw_breakpoint_info && val == 0)
...
else if ((thbi = alloc_thread_hw_breakpoint(tsk)) == NULL)
...
else if (n < HB_NUM) {
...
}
else
...
If you can suggest a way to change the code without making it worse,
I'm sure Prasad will be happy to adopt it.
Alan Stern
* Alan Stern <[email protected]> wrote:
> On Tue, 10 Mar 2009, Ingo Molnar wrote:
>
> >
> > There's also a few checkpatch warnings that need to be
> > addressed:
> >
> > ERROR: do not use assignment in if condition
> > #1084: FILE: arch/x86/kernel/ptrace.c:581:
> > + else if ((thbi = alloc_thread_hw_breakpoint(tsk)) ==
> > NULL)
>
> Changing this to remove the assignment from within the "if"
> condition would make the code less readable, not more. [...]
It's not just about basic readability. We dont do assignmets
like that because it's very easy to miss them (and their side
effects) and conflating them with '=='.
It's part of a long
> series of tests; in schematic form:
>
> if (n == 4 || n == 5)
> ...
> else if (n == 6) {
> ...
> }
> else if (!tsk->thread.hw_breakpoint_info && val == 0)
> ...
> else if ((thbi = alloc_thread_hw_breakpoint(tsk)) == NULL)
> ...
> else if (n < HB_NUM) {
> ...
> }
> else
> ...
>
> If you can suggest a way to change the code without making it
> worse, I'm sure Prasad will be happy to adopt it.
The obvious solution which we use in many other places in
arch/x86 is to split out that butt-ugly if else if else if else
maze into a helper inline function that does:
if (n == 4 || n == 5) {
do stuff;
return;
}
if (n == 6) {
do stuff;
return;
}
if (!tsk->thread.hw_breakpoint_info && val == 0) {
do stuff;
return;
}
thbi = alloc_thread_hw_breakpoint(tsk);
if (!tbhi)
...
Ingo
On Tue, Mar 10, 2009 at 02:46:55PM +0100, Ingo Molnar wrote:
>
> * [email protected] <[email protected]> wrote:
>
> > Hi Ingo,
>
> > Please find the revised set of patches that implement
> > Hardware Breakpoint (or watchpoint) registers and an
> > arch-specific implementation for x86/x86_64.
>
> General structure looks good, with a good deal of details
> that need to be addressed.
>
Thanks to Alan Stern for answering most of the questions....I am
pitching in to fill the gaps and do any re-write based on the
comments.
> Firstly, as far as i can see this should work on 32-bit too,
> correct?
>
Yes. It's been tested on 32-bit x86 all throughout.
> Secondly, what about other architectures - will they build just
> fine without any arch level glue code? kernel/hw_breakpoint.o
> get build unconditionally - without any benefit to non-x86 code.
> Perhaps an ARCH_HAS_HW_BREAKPOINTS Kconfig method would be
> useful to add.
The hardware breakpoint interfaces haven't been put under any CONFIG_
till now, but I think we should bring them under a new config, say
CONFIG_HW_BREAKPOINT. It would help create a dependancy for
CONFIG_KSYM_TRACER too.
>
> There's also a number of (small) style issues.
> kernel/hw_breakpoint.c and other new .c files dont comply to the
> customary comment style of:
>
> /*
> * Comment .....
> * ...... goes here:
> */
>
> also, the #include files section style should match that of
> arch/x86/mm/fault.c - it's a conflict-avoidance style.
>
> also, things like this:
>
> static struct notifier_block hw_breakpoint_exceptions_nb = {
> .notifier_call = hw_breakpoint_exceptions_notify,
> .priority = 0x7fffffff /* we need to be notified first */
> };
>
> should be:
>
> static struct notifier_block hw_breakpoint_exceptions_nb = {
> .notifier_call = hw_breakpoint_exceptions_notify,
> /* We need to be notified first: */
> .priority = 0x7fffffff,
> };
>
> Ingo
Sure, will look at the comment styling before I re-send the patchset.
Thanks,
K.Prasad
On Wed, 11 Mar 2009, K.Prasad wrote:
> The hardware breakpoint interfaces haven't been put under any CONFIG_
> till now, but I think we should bring them under a new config, say
> CONFIG_HW_BREAKPOINT. It would help create a dependancy for
> CONFIG_KSYM_TRACER too.
With these patches, ptrace is dependent on hw-breakpoint. You can't
disable CONFIG_HW_BREAKPOINT without breaking ptrace.
Alan Stern
On Wed, Mar 11, 2009 at 12:34:43PM -0400, Alan Stern wrote:
> On Wed, 11 Mar 2009, K.Prasad wrote:
>
> > The hardware breakpoint interfaces haven't been put under any CONFIG_
> > till now, but I think we should bring them under a new config, say
> > CONFIG_HW_BREAKPOINT. It would help create a dependancy for
> > CONFIG_KSYM_TRACER too.
>
> With these patches, ptrace is dependent on hw-breakpoint. You can't
> disable CONFIG_HW_BREAKPOINT without breaking ptrace.
>
> Alan Stern
>
Agreed. We might have to retain the old code for ptrace and put the new
implementation under #ifdef CONFIG_HW_BREAKPOINT to get them working.
What do you think?
Thanks,
K.Prasad
* K.Prasad <[email protected]> wrote:
> On Wed, Mar 11, 2009 at 12:34:43PM -0400, Alan Stern wrote:
> > On Wed, 11 Mar 2009, K.Prasad wrote:
> >
> > > The hardware breakpoint interfaces haven't been put under any CONFIG_
> > > till now, but I think we should bring them under a new config, say
> > > CONFIG_HW_BREAKPOINT. It would help create a dependancy for
> > > CONFIG_KSYM_TRACER too.
> >
> > With these patches, ptrace is dependent on hw-breakpoint.
> > You can't disable CONFIG_HW_BREAKPOINT without breaking
> > ptrace.
> >
> > Alan Stern
> >
>
> Agreed. We might have to retain the old code for ptrace and
> put the new implementation under #ifdef CONFIG_HW_BREAKPOINT
> to get them working. What do you think?
With the simple reservation mechanism i suggested i have no
problem with having HW_BREAKPOINT enabled [selected]
unconditionally on x86.
Your ptrace changes are an improvement in terms of code quality
so as long as the facility is simple and obvious, it's a step
forward.
#ifdefs are ugly and hard to maintain - especially in such a
rarely used and still critical API as ptrace.
Ingo