Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

(WARNING: I've browsed the ARMv7 breakpoints implementation
but I may have an erratic/incomplete understanding, then parts
of this review might make little sense)


2010/3/10 Will Deacon <will.deacon@arm.com>:
> The hw-breakpoint framework in the kernel requires architecture-specific
> support in order to install, remove, validate and manage hardware
> breakpoints.
>
> This patch adds preliminary support for this framework to the ARM
> architecture, but restricts the number of watchpoints to a single resource
> to get around the fact that the Data Fault Address Register is unpredictable
> when a watchpoint debug exception is taken.


What do you mean here by unpredictable? It would be a pity to limit the
resources to one register.



> +}
> +
> +/* Determine number of WRP registers available. */
> +static int get_num_wrps(void)
> +{
> +       /*
> +        * FIXME: When a watchpoint fires, the only way to work out which
> +        * watchpoint it was is by disassembling the faulting instruction
> +        * and working out the address of the memory access.



Doh! That must explain the problem with DFAR...
That's really not convenient :-(



> +        *
> +        * Furthermore, we can only do this if the watchpoint was precise
> +        * since imprecise watchpoints prevent us from calculating register
> +        * based addresses.
> +        *
> +        * For the time being, we only report 1 watchpoint register so we
> +        * always know which watchpoint fired. In the future we can either
> +        * add a disassembler and address generation emulator, or we can
> +        * insert a check to see if the DFAR is set on watchpoint exception
> +        * entry [the ARM ARM states that the DFAR is UNPREDICTABLE, but
> +        * experience shows that it is set on some implementations].


Ok...



> +/*
> + * Install a perf counter breakpoint.
> + */
> +int arch_install_hw_breakpoint(struct perf_event *bp)
> +{
> +       struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> +       struct perf_event **slot, **slots;
> +       int i, max_slots, ctrl_base, val_base, ret = 0;
> +
> +       /* Ensure that we are in monitor mode and halting mode is disabled. */
> +       ret = enable_monitor_mode();
> +       if (ret)
> +               goto out;
> +
> +       if (info->type == ARM_BREAKPOINT_EXECUTE) {
> +               /* Breakpoint */
> +               ctrl_base = ARM_BASE_BCR;
> +               val_base = ARM_BASE_BVR;
> +               slots = __get_cpu_var(bp_on_reg);
> +               max_slots = core_num_brps;
> +       } else {
> +               /* Watchpoint */
> +               ctrl_base = ARM_BASE_WCR;
> +               val_base = ARM_BASE_WVR;
> +               slots = __get_cpu_var(wp_on_reg);
> +               max_slots = core_num_wrps;
> +       }
> +
> +       for (i = 0; i < max_slots; ++i) {
> +               slot = &slots[i];
> +
> +               if (!*slot) {
> +                       *slot = bp;
> +                       break;
> +               }
> +       }
> +
> +       if (WARN_ONCE(i == max_slots, "Can't find any breakpoint slot")) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       /* Setup the address register. */
> +       write_wb_reg(val_base + i, info->address);
> +
> +       /* Setup the control register. */
> +       write_wb_reg(ctrl_base + i, (info->type << 3) |
> +                       (info->privilege << 1) | (info->len << 5) | 1);



Ok, alternatively, why not having the control register in the arch
breakpoint structure:

u32   __reserved : 19,
        len             :  7,
        type           :  2,
        privilege      :  2,
        enabled      :  1;

It will avoid you to play with bitwise operations that may scale into dirty
and error prone as you may support more features from the control register
(link, mask, etc...).

So in the future if you want to support more things, you can just split out
the __reserved field into the features it has, depending on the ARM versions.

Ah and it will make ptrace support easier: the user writes into the val/ctrl
fields directly as if they were the true registers, then you can just validate
the fields you want without bitwise ops.



> +/*
> + * Validate the arch-specific HW Breakpoint register settings.
> + */
> +int arch_validate_hwbkpt_settings(struct perf_event *bp,
> +                                 struct task_struct *tsk)
> +{
> +       struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> +       int ret = 0;
> +       u32 alignment_mask = 0x3;
> +
> +       /* Build the arch_hw_breakpoint. */
> +       ret = arch_build_bp_info(bp);
> +       if (ret)
> +               goto out;
> +
> +       /* Check address alignment. */
> +       if (info->len == ARM_BREAKPOINT_LEN_8)
> +               alignment_mask = 0x7;
> +       if (info->address & alignment_mask) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       /* Check that the virtual address is in the proper range. */
> +       if (tsk) {
> +               if (!(info->privilege & ARM_BREAKPOINT_USER)) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +       } else {
> +               if (!(info->privilege & ARM_BREAKPOINT_SUPER)) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +       }


You seem to make a wrong assumption here.
This is not because the breakpoint is task-bound that we don't
want it to trigger on the kernel. We may want to trigger breakpoints
on tasklist_lock accesses from a given task.

The only case for which we don't want it to trigger on the kernel
is for ptrace breakpoints.

I guess I should remove this tsk parameter as it makes the things
only confusing. We should simply create ptrace breakpoints with
bp->attr.exclude_kernel set to 1.

Because when bp->attr.exclude_kernel is set to 1, then it truly makes sense
to think about user and supervisor privileges, just to avoid to filter the
event in the software level. Note we do this filtering on software level,
but it going to be less costly if done from hardware.

Even if you don't support ptrace right now, we can define a kernel exluded
breakpoint from perf syscall.

(Note for myself: set exclude_kernel to ptrace breakpoints in x86)


> +/*
> + * Release the user breakpoints used by ptrace.
> + */
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> +{
> +       int i;
> +       struct thread_struct *t = &tsk->thread;
> +
> +       for (i = 0; i < HBP_NUM; i++) {
> +               unregister_hw_breakpoint(t->debug.hbp[i]);
> +               t->debug.hbp[i] = NULL;
> +       }
> +}


Do you actually support ptrace breakpoints?



> +
> +void hw_breakpoint_pmu_unthrottle(struct perf_event *bp)
> +{
> +       /* TODO */
> +}


We don't need this callback anymore, it has been removed because
of ptrace corner cases...


Thanks!

Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Will Deacon]

Hi Frederic,

Many thanks for taking a look at the these patches, I appreciate the
feedback.

On Tue, 2010-04-13 at 14:32 +0100, Frederic Weisbecker wrote:
> (WARNING: I've browsed the ARMv7 breakpoints implementation
> but I may have an erratic/incomplete understanding, then parts
> of this review might make little sense)
> 
It looks like you've understood it correctly. Actually, I have
a disclaimer of my own; I'm using a new mail client in the hope
that LKML won't drop my messages. Anyway...
> 
> 2010/3/10 Will Deacon <will.deacon@arm.com>:
> > The hw-breakpoint framework in the kernel requires architecture-specific
> > support in order to install, remove, validate and manage hardware
> > breakpoints.
> >
> > This patch adds preliminary support for this framework to the ARM
> > architecture, but restricts the number of watchpoints to a single resource
> > to get around the fact that the Data Fault Address Register is unpredictable
> > when a watchpoint debug exception is taken.
> 
> What do you mean here by unpredictable? It would be a pity to limit the
> resources to one register.
> 
By unpredictable I mean that the value in the DFAR is not defined to
correspond to the causative address in any way. This isn't the case
for a standard data abort, but it is in the case of a watchpoint
exception.
> 
> > +}
> > +
> > +/* Determine number of WRP registers available. */
> > +static int get_num_wrps(void)
> > +{
> > +       /*
> > +        * FIXME: When a watchpoint fires, the only way to work out which
> > +        * watchpoint it was is by disassembling the faulting instruction
> > +        * and working out the address of the memory access.
> 
> Doh! That must explain the problem with DFAR...
> That's really not convenient :-(
> 
I know, it makes life a lot harder for us. The most annoying thing is that
I'm yet to find a real implementation that *doesn't* set the DFAR to what
we want - we just can't rely on that!
> 
<snip>

> > +       /* Setup the address register. */
> > +       write_wb_reg(val_base + i, info->address);
> > +
> > +       /* Setup the control register. */
> > +       write_wb_reg(ctrl_base + i, (info->type << 3) |
> > +                       (info->privilege << 1) | (info->len << 5) | 1);

> Ok, alternatively, why not having the control register in the arch
> breakpoint structure:
> 
> u32   __reserved : 19,
>         len             :  7,
>         type           :  2,
>         privilege      :  2,
>         enabled      :  1;
> 
> It will avoid you to play with bitwise operations that may scale into dirty
> and error prone as you may support more features from the control register
> (link, mask, etc...).
> 
Nice, I like that a lot!

> Ah and it will make ptrace support easier: the user writes into the val/ctrl
> fields directly as if they were the true registers, then you can just validate
> the fields you want without bitwise ops.
> 
I'm unsure about this. As mainline stands, ARM has no ptrace support for the
hardware breakpoint registers. This means that we could expose the
hardware resources in an idealised fashion via ptrace so that if the
interface varies between CPUs, userspace doesn't need to care. I had a
crack at this with another patch in the series here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html

> > +       /* Check that the virtual address is in the proper range. */
> > +       if (tsk) {
> > +               if (!(info->privilege & ARM_BREAKPOINT_USER)) {
> > +                       ret = -EFAULT;
> > +                       goto out;
> > +               }
> > +       } else {
> > +               if (!(info->privilege & ARM_BREAKPOINT_SUPER)) {
> > +                       ret = -EFAULT;
> > +                       goto out;
> > +               }
> > +       }
> 
> 
> You seem to make a wrong assumption here.
> This is not because the breakpoint is task-bound that we don't
> want it to trigger on the kernel. We may want to trigger breakpoints
> on tasklist_lock accesses from a given task.

> The only case for which we don't want it to trigger on the kernel
> is for ptrace breakpoints.
> 
Surely we can't have breakpoints triggering on *any* part of the
breakpoint handling code path? Otherwise we'll just get stuck. That's
why I disallow breakpoints in the exception section.

> I guess I should remove this tsk parameter as it makes the things
> only confusing. We should simply create ptrace breakpoints with
> bp->attr.exclude_kernel set to 1.

Sounds good to me. That also means that it's one less thing for the arch
to worry about!
> 
> > +/*
> > + * Release the user breakpoints used by ptrace.
> > + */
> > +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +       int i;
> > +       struct thread_struct *t = &tsk->thread;
> > +
> > +       for (i = 0; i < HBP_NUM; i++) {
> > +               unregister_hw_breakpoint(t->debug.hbp[i]);
> > +               t->debug.hbp[i] = NULL;
> > +       }
> > +}
> 
> 
> Do you actually support ptrace breakpoints?
> 
Yep. See the link mentioned above.
> 
> > +
> > +void hw_breakpoint_pmu_unthrottle(struct perf_event *bp)
> > +{
> > +       /* TODO */
> > +}
> 
> 
> We don't need this callback anymore, it has been removed because
> of ptrace corner cases...
> 
Ok, I'll remove it.

I'll take a look at the new constraints and allocation patches you
posted this morning and then adjust these patches accordingly.

Thanks,

Will

Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

On Tue, Apr 13, 2010 at 04:52:45PM +0100, Will Deacon wrote:
> On Tue, 2010-04-13 at 14:32 +0100, Frederic Weisbecker wrote:
> > (WARNING: I've browsed the ARMv7 breakpoints implementation
> > but I may have an erratic/incomplete understanding, then parts
> > of this review might make little sense)
> > 
> It looks like you've understood it correctly. Actually, I have
> a disclaimer of my own; I'm using a new mail client in the hope
> that LKML won't drop my messages. Anyway...


Looks like that worked as well :)



> > 2010/3/10 Will Deacon <will.deacon@arm.com>:
> > > The hw-breakpoint framework in the kernel requires architecture-specific
> > > support in order to install, remove, validate and manage hardware
> > > breakpoints.
> > >
> > > This patch adds preliminary support for this framework to the ARM
> > > architecture, but restricts the number of watchpoints to a single resource
> > > to get around the fact that the Data Fault Address Register is unpredictable
> > > when a watchpoint debug exception is taken.
> > 
> > What do you mean here by unpredictable? It would be a pity to limit the
> > resources to one register.
> > 
> By unpredictable I mean that the value in the DFAR is not defined to
> correspond to the causative address in any way. This isn't the case
> for a standard data abort, but it is in the case of a watchpoint
> exception.



Ok.



> > 
> > > +}
> > > +
> > > +/* Determine number of WRP registers available. */
> > > +static int get_num_wrps(void)
> > > +{
> > > +       /*
> > > +        * FIXME: When a watchpoint fires, the only way to work out which
> > > +        * watchpoint it was is by disassembling the faulting instruction
> > > +        * and working out the address of the memory access.
> > 
> > Doh! That must explain the problem with DFAR...
> > That's really not convenient :-(
> > 
> I know, it makes life a lot harder for us. The most annoying thing is that
> I'm yet to find a real implementation that *doesn't* set the DFAR to what
> we want - we just can't rely on that!


You mean that sometimes DFAR doesn't have the true ip origin of the
exception?
May be you can check the trapped regs to find the instruction pointer
that caused the exception?


 
> > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > fields directly as if they were the true registers, then you can just validate
> > the fields you want without bitwise ops.
> > 
> I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> hardware breakpoint registers. This means that we could expose the
> hardware resources in an idealised fashion via ptrace so that if the
> interface varies between CPUs, userspace doesn't need to care. I had a
> crack at this with another patch in the series here:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html



Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
I'm going to look at this thread too.



 
> > You seem to make a wrong assumption here.
> > This is not because the breakpoint is task-bound that we don't
> > want it to trigger on the kernel. We may want to trigger breakpoints
> > on tasklist_lock accesses from a given task.
> 
> > The only case for which we don't want it to trigger on the kernel
> > is for ptrace breakpoints.
> > 
> Surely we can't have breakpoints triggering on *any* part of the
> breakpoint handling code path? Otherwise we'll just get stuck. That's
> why I disallow breakpoints in the exception section.



Ah indeed you really need to protect against recursion.

But I'm unclear about the difference between Supervisor and System.
May be System means the common kernel ring? And you enter into
Supervisor when an exception triggers?

Do these exceptions also concern page faults and not only debug
exceptions?

 
> > I guess I should remove this tsk parameter as it makes the things
> > only confusing. We should simply create ptrace breakpoints with
> > bp->attr.exclude_kernel set to 1.
> 
> Sounds good to me. That also means that it's one less thing for the arch
> to worry about!


Yeah!



> I'll take a look at the new constraints and allocation patches you
> posted this morning and then adjust these patches accordingly.


Thanks.

Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Will Deacon]

Hello Frederic,

On Tue, 2010-04-13 at 21:40 +0100, Frederic Weisbecker wrote:
> > > > +/* Determine number of WRP registers available. */
> > > > +static int get_num_wrps(void)
> > > > +{
> > > > +       /*
> > > > +        * FIXME: When a watchpoint fires, the only way to work out which
> > > > +        * watchpoint it was is by disassembling the faulting instruction
> > > > +        * and working out the address of the memory access.
> > >
> > > Doh! That must explain the problem with DFAR...
> > > That's really not convenient :-(
> > >
> > I know, it makes life a lot harder for us. The most annoying thing is that
> > I'm yet to find a real implementation that *doesn't* set the DFAR to what
> > we want - we just can't rely on that!
> 
> You mean that sometimes DFAR doesn't have the true ip origin of the
> exception?
> May be you can check the trapped regs to find the instruction pointer
> that caused the exception?

There are two related issues here:

1.) A watchpoint can be synchronous or asynchronous, you can determine which when
    you take the exception. If the watchpoint is synchronous, then yes, we can look
    at the trapped regs to find the causative instruction. Then we would need to
    disassemble the instruction and emulate the address generation part [which may
    also need the trapped regs] in order to calculate the faulting data address.
    If the watchpoint exception is asynchronous, we can read the WFAR register to
    find the address of the causative instruction. Unfortunately, we could still
    be screwed in this case because we won't be able to emulate the address
    calculation for a register-relative load/store. Thankfully, v7 cores only
    generate synchronous watchpoints [apart from some very early revisions which
    you won't see in the wild] but v6 cores are the opposite; they only generate
    asynchronous watchpoint exceptions.

2.) For a standard data abort, the DFAR is updated to contain the data address which
    caused the fault. Ideally this would also be the case for watchpoint exceptions
    because then we wouldn't have to disassemble anything [and as I mentioned
    previously, I'm yet to find a real implementation that doesn't set the DFAR on
    taking a watchpoint, but it's not something that the architecture guarantees].
    
> > > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > > fields directly as if they were the true registers, then you can just validate
> > > the fields you want without bitwise ops.
> > >
> > I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> > hardware breakpoint registers. This means that we could expose the
> > hardware resources in an idealised fashion via ptrace so that if the
> > interface varies between CPUs, userspace doesn't need to care. I had a
> > crack at this with another patch in the series here:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html
> 
> 
> 
> Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
> I'm going to look at this thread too.

It's not so much about necessity, but I think it would be nice if userspace
didn't have to care so much about the breakpoint/watchpoint debug hardware on the
target. Since we don't have any backwards compatibility issues [because ptrace in
mainline has never supported hardware breakpoints on ARM] we can define a stable
API regardless of the hardware.

> > > You seem to make a wrong assumption here.
> > > This is not because the breakpoint is task-bound that we don't
> > > want it to trigger on the kernel. We may want to trigger breakpoints
> > > on tasklist_lock accesses from a given task.
> >
> > > The only case for which we don't want it to trigger on the kernel
> > > is for ptrace breakpoints.
> > >
> > Surely we can't have breakpoints triggering on *any* part of the
> > breakpoint handling code path? Otherwise we'll just get stuck. That's
> > why I disallow breakpoints in the exception section.
> 
> 
> 
> Ah indeed you really need to protect against recursion.

I'm unsure about how to provide full protection though. You could still
set a breakpoint on the breakpoint handling code that exists outside of the
exception text. Do other archs suffer from this problem? I'm tempted
just to disallow Kernel breakpoints for now, but that would be a shame.

> But I'm unclear about the difference between Supervisor and System.
> May be System means the common kernel ring? And you enter into
> Supervisor when an exception triggers?

Sorry, I think my ARM_BREAKPOINT_SUPER #define is confusing. I'll rename it
to ARM_BREAKPOINT_PRIV because the hardware only distinguishes between
privileged and unprivileged accesses, regardless of the mode.

> Do these exceptions also concern page faults and not only debug
> exceptions?

I think so.

One thing that has hit me is the behaviour of the framework when a breakpoint
is hit. If the breakpoint is synchronous, after the exception has been handled
[and perf_bp_event has been called], execution will restart at the faulting
instruction. It is down to the client program [traditionally a debugger] to
remove the breakpoint, single-step and re-insert the breakpoint. Obviously the
perf tool doesn't do this, so if you try to perf stat a breakpoint event you
will livelock [potentially another reason to disable Kernel breakpoints?]

Cheers,

Will

Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

On Wed, Apr 14, 2010 at 11:27:01AM +0100, Will Deacon wrote:
> Hello Frederic,
> 
> On Tue, 2010-04-13 at 21:40 +0100, Frederic Weisbecker wrote:
> > > > > +/* Determine number of WRP registers available. */
> > > > > +static int get_num_wrps(void)
> > > > > +{
> > > > > +       /*
> > > > > +        * FIXME: When a watchpoint fires, the only way to work out which
> > > > > +        * watchpoint it was is by disassembling the faulting instruction
> > > > > +        * and working out the address of the memory access.
> > > >
> > > > Doh! That must explain the problem with DFAR...
> > > > That's really not convenient :-(
> > > >
> > > I know, it makes life a lot harder for us. The most annoying thing is that
> > > I'm yet to find a real implementation that *doesn't* set the DFAR to what
> > > we want - we just can't rely on that!
> > 
> > You mean that sometimes DFAR doesn't have the true ip origin of the
> > exception?
> > May be you can check the trapped regs to find the instruction pointer
> > that caused the exception?
> 
> There are two related issues here:
> 
> 1.) A watchpoint can be synchronous or asynchronous, you can determine which when
>     you take the exception. If the watchpoint is synchronous, then yes, we can look
>     at the trapped regs to find the causative instruction. Then we would need to
>     disassemble the instruction and emulate the address generation part [which may
>     also need the trapped regs] in order to calculate the faulting data address.
>     If the watchpoint exception is asynchronous, we can read the WFAR register to
>     find the address of the causative instruction. Unfortunately, we could still
>     be screwed in this case because we won't be able to emulate the address
>     calculation for a register-relative load/store. Thankfully, v7 cores only
>     generate synchronous watchpoints [apart from some very early revisions which
>     you won't see in the wild] but v6 cores are the opposite; they only generate
>     asynchronous watchpoint exceptions.
> 
> 2.) For a standard data abort, the DFAR is updated to contain the data address which
>     caused the fault. Ideally this would also be the case for watchpoint exceptions
>     because then we wouldn't have to disassemble anything [and as I mentioned
>     previously, I'm yet to find a real implementation that doesn't set the DFAR on
>     taking a watchpoint, but it's not something that the architecture guarantees].



It's a cruel architecture...


     
> > > > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > > > fields directly as if they were the true registers, then you can just validate
> > > > the fields you want without bitwise ops.
> > > >
> > > I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> > > hardware breakpoint registers. This means that we could expose the
> > > hardware resources in an idealised fashion via ptrace so that if the
> > > interface varies between CPUs, userspace doesn't need to care. I had a
> > > crack at this with another patch in the series here:
> > >
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html
> > 
> > 
> > 
> > Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
> > I'm going to look at this thread too.
> 
> It's not so much about necessity, but I think it would be nice if userspace
> didn't have to care so much about the breakpoint/watchpoint debug hardware on the
> target. Since we don't have any backwards compatibility issues [because ptrace in
> mainline has never supported hardware breakpoints on ARM] we can define a stable
> API regardless of the hardware.


Yeah, looking at how this is implemented across versions, it seems
that they all share the same registers, but early versions have
unfeatured reserved bytes in control registers that are filled
with new features over time in subsequent versions.

It looks like the things are then forward compatible but not
backward.

Isn't that enough to provide a ptrace interface? I mean you can't do
much magic here. Having something that works everywhere would consist
in implementing only the minimal set of features only.



> > > > You seem to make a wrong assumption here.
> > > > This is not because the breakpoint is task-bound that we don't
> > > > want it to trigger on the kernel. We may want to trigger breakpoints
> > > > on tasklist_lock accesses from a given task.
> > >
> > > > The only case for which we don't want it to trigger on the kernel
> > > > is for ptrace breakpoints.
> > > >
> > > Surely we can't have breakpoints triggering on *any* part of the
> > > breakpoint handling code path? Otherwise we'll just get stuck. That's
> > > why I disallow breakpoints in the exception section.
> > 
> > 
> > 
> > Ah indeed you really need to protect against recursion.
> 
> I'm unsure about how to provide full protection though. You could still
> set a breakpoint on the breakpoint handling code that exists outside of the
> exception text. Do other archs suffer from this problem? I'm tempted
> just to disallow Kernel breakpoints for now, but that would be a shame.


In x86 we just write the control register to disable the breakpoints
in the exception handler. (I suspect we don't do it early enough though).

You can do the same in ARM.

Currently, there is no true security flow because if a breakpoint
is task bound, x86 refuse breakpoints in kernel addresses.
But as I noticed before, this is a wrong behaviour as we also
want to be able to trace kernel var accesses while in a given task
context, except for ptrace.

I should just add a CAP_SYS_ADMIN() && !ptrace check for kernel
addresses.



> > But I'm unclear about the difference between Supervisor and System.
> > May be System means the common kernel ring? And you enter into
> > Supervisor when an exception triggers?
> 
> Sorry, I think my ARM_BREAKPOINT_SUPER #define is confusing. I'll rename it
> to ARM_BREAKPOINT_PRIV because the hardware only distinguishes between
> privileged and unprivileged accesses, regardless of the mode.


Ok.



> > Do these exceptions also concern page faults and not only debug
> > exceptions?
> 
> I think so.
> 
> One thing that has hit me is the behaviour of the framework when a breakpoint
> is hit. If the breakpoint is synchronous, after the exception has been handled
> [and perf_bp_event has been called], execution will restart at the faulting
> instruction. It is down to the client program [traditionally a debugger] to
> remove the breakpoint, single-step and re-insert the breakpoint. Obviously the
> perf tool doesn't do this, so if you try to perf stat a breakpoint event you
> will livelock [potentially another reason to disable Kernel breakpoints?]


If I understand you correctly,

sync = breakpoint triggers before the instruction is executed,
       trap return to the faulting instruction
async = breakpoint triggers after the instruction is executed

Then yeah, the current breakpoint framework only supports async
ones.

If an arch has no choice but using sync breakpoints, it needs
to handle that. It seems PowerPc has a similar problem, may be
have a look at Prasad's patches.

If there is something that can be handled from the generic
layer to help solving this problem, I'll try something.

Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Will Deacon]

Hi Frederic,

Thanks for addressing some of these problems.

> > There are two related issues here:
> >
> > 1.) A watchpoint can be synchronous or asynchronous, you can determine which when
> >     you take the exception. If the watchpoint is synchronous, then yes, we can look
> >     at the trapped regs to find the causative instruction. Then we would need to
> >     disassemble the instruction and emulate the address generation part [which may
> >     also need the trapped regs] in order to calculate the faulting data address.
> >     If the watchpoint exception is asynchronous, we can read the WFAR register to
> >     find the address of the causative instruction. Unfortunately, we could still
> >     be screwed in this case because we won't be able to emulate the address
> >     calculation for a register-relative load/store. Thankfully, v7 cores only
> >     generate synchronous watchpoints [apart from some very early revisions which
> >     you won't see in the wild] but v6 cores are the opposite; they only generate
> >     asynchronous watchpoint exceptions.
> >
> > 2.) For a standard data abort, the DFAR is updated to contain the data address which
> >     caused the fault. Ideally this would also be the case for watchpoint exceptions
> >     because then we wouldn't have to disassemble anything [and as I mentioned
> >     previously, I'm yet to find a real implementation that doesn't set the DFAR on
> >     taking a watchpoint, but it's not something that the architecture guarantees].
> 
> 
> 
> It's a cruel architecture...
> 

It grows on you :)

> > > > > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > > > > fields directly as if they were the true registers, then you can just validate
> > > > > the fields you want without bitwise ops.
> > > > >
> > > > I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> > > > hardware breakpoint registers. This means that we could expose the
> > > > hardware resources in an idealised fashion via ptrace so that if the
> > > > interface varies between CPUs, userspace doesn't need to care. I had a
> > > > crack at this with another patch in the series here:
> > > >
> > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html
> > >
> > >
> > >
> > > Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
> > > I'm going to look at this thread too.
> >
> > It's not so much about necessity, but I think it would be nice if userspace
> > didn't have to care so much about the breakpoint/watchpoint debug hardware on the
> > target. Since we don't have any backwards compatibility issues [because ptrace in
> > mainline has never supported hardware breakpoints on ARM] we can define a stable
> > API regardless of the hardware.
> 
> 
> Yeah, looking at how this is implemented across versions, it seems
> that they all share the same registers, but early versions have
> unfeatured reserved bytes in control registers that are filled
> with new features over time in subsequent versions.
> 
> It looks like the things are then forward compatible but not
> backward.
> 
> Isn't that enough to provide a ptrace interface? I mean you can't do
> much magic here. Having something that works everywhere would consist
> in implementing only the minimal set of features only.

I suppose the ptrace interface will involve bit-packing at some point anyway,
so mirroring the hardware configuration might not be as bad as I initially thought.
The hardware has some slightly more esoteric features [breakpoint linking, context
matching etc] that won't be supported in software for now so I'll ignore those bits.

> > I'm unsure about how to provide full protection though. You could still
> > set a breakpoint on the breakpoint handling code that exists outside of the
> > exception text. Do other archs suffer from this problem? I'm tempted
> > just to disallow Kernel breakpoints for now, but that would be a shame.
> 
> 
> In x86 we just write the control register to disable the breakpoints
> in the exception handler. (I suspect we don't do it early enough though).
> 
> You can do the same in ARM.

Ok.
 
> > One thing that has hit me is the behaviour of the framework when a breakpoint
> > is hit. If the breakpoint is synchronous, after the exception has been handled
> > [and perf_bp_event has been called], execution will restart at the faulting
> > instruction. It is down to the client program [traditionally a debugger] to
> > remove the breakpoint, single-step and re-insert the breakpoint. Obviously the
> > perf tool doesn't do this, so if you try to perf stat a breakpoint event you
> > will livelock [potentially another reason to disable Kernel breakpoints?]
> 
> 
> If I understand you correctly,
> 
> sync = breakpoint triggers before the instruction is executed,
>        trap return to the faulting instruction
> async = breakpoint triggers after the instruction is executed

That's right.
 
> Then yeah, the current breakpoint framework only supports async
> ones.

Oh no!

> If an arch has no choice but using sync breakpoints, it needs
> to handle that. It seems PowerPc has a similar problem, may be
> have a look at Prasad's patches.

Could you give me a pointer to these patches please [I can't see them on LKML]?
I *think* PowerPC is lucky enough to have hardware single step, so handling
synchronous breakpoints isn't *too* difficult. In ARM we have to work out the
address of the following instruction, which is quite tricky because it could be a
branch instruction [from a selection of instruction sets] and also be predicated.
GDB can handle this, but the Kernel doesn't have the capability to do so [there's
some code in arch/arm/ptrace.c but it's not feature complete].

> If there is something that can be handled from the generic
> layer to help solving this problem, I'll try something.

Unfortunately this is very much an arch problem. As a half-way house, I might
implement only the ptrace part of hw-breakpoint for ARM. That way we can
force the client to handle stepping over the breakpoint and we also won't have
to worry about recursive breakpoints in the Kernel.

Cheers,

Will

Re: [RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 03:17:59PM +0100, Will Deacon wrote:
> Hi Frederic,
> > It's a cruel architecture...
> > 
> 
> It grows on you :)


:-)))


 
> > Yeah, looking at how this is implemented across versions, it seems
> > that they all share the same registers, but early versions have
> > unfeatured reserved bytes in control registers that are filled
> > with new features over time in subsequent versions.
> > 
> > It looks like the things are then forward compatible but not
> > backward.
> > 
> > Isn't that enough to provide a ptrace interface? I mean you can't do
> > much magic here. Having something that works everywhere would consist
> > in implementing only the minimal set of features only.
> 
> I suppose the ptrace interface will involve bit-packing at some point anyway,
> so mirroring the hardware configuration might not be as bad as I initially thought.
> The hardware has some slightly more esoteric features [breakpoint linking, context
> matching etc] that won't be supported in software for now so I'll ignore those bits.



In fact, I guess watchpoint to breakpoint linking is something that userspace
can use. But the context ID linking....well I haven't understood this part
in ARM documentation :)


 
> > > I'm unsure about how to provide full protection though. You could still
> > > set a breakpoint on the breakpoint handling code that exists outside of the
> > > exception text. Do other archs suffer from this problem? I'm tempted
> > > just to disallow Kernel breakpoints for now, but that would be a shame.
> > 
> > 
> > In x86 we just write the control register to disable the breakpoints
> > in the exception handler. (I suspect we don't do it early enough though).
> > 
> > You can do the same in ARM.
> 
> Ok.



Speaking about this made me wonder about the current state in x86.
For now it's safe because task bound breakpoint only allow userspace
addresses.
But this is not right because we want task bound to be able to trigger
on kernel. Once we'll allow this, we need a bit of policy.

Because yeah we can protect against recursion, but always partially.
The only solution to really protect would be disabling the breakpoint
as the very first operation in the trap handler.

But that's ugly and may be costly as well.

So what I'm going to do is to simply check attr.exclude_kernel on
breakpoint creation and look if the address is in the kernel space.

So that if you want to crash your machine why lurking at the very
tight window of trap handler possible recursion, you need to be
root :)

Because when a perf event registers and doesn't have exclude_kernel,
you need to be admin.

Fortunately we don't have this security hole with the current rules,
but I'll try to make this check more generic (will require archs that
support breakpoints to implement arch_check_va_in_kernelspace()).


> > If an arch has no choice but using sync breakpoints, it needs
> > to handle that. It seems PowerPc has a similar problem, may be
> > have a look at Prasad's patches.
> 
> Could you give me a pointer to these patches please [I can't see them on LKML]?
> I *think* PowerPC is lucky enough to have hardware single step, so handling
> synchronous breakpoints isn't *too* difficult. In ARM we have to work out the
> address of the following instruction, which is quite tricky because it could be a
> branch instruction [from a selection of instruction sets] and also be predicated.
> GDB can handle this, but the Kernel doesn't have the capability to do so [there's
> some code in arch/arm/ptrace.c but it's not feature complete].


I don't remember exactly how this is handled. I remember a story about
a one-shot breakpoint, ie: that triggers only once.

Anyway, I have yet to review the latest version:

http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081714.html
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081715.html
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081716.html



> 
> > If there is something that can be handled from the generic
> > layer to help solving this problem, I'll try something.
> 
> Unfortunately this is very much an arch problem. As a half-way house, I might
> implement only the ptrace part of hw-breakpoint for ARM. That way we can
> force the client to handle stepping over the breakpoint and we also won't have
> to worry about recursive breakpoints in the Kernel.


:-s

Yeah, we'll see what we can do.