Re: [GIT PULL v4.5] Fix INT1 recursion with unregistered breakpoints

[Andy Lutomirski]

On Mon, Jan 11, 2016 at 4:35 PM, Jeff V. Merkey <linux.mdb@xxxxxxxxx> wrote:
> The following changes since commit ee78027142ab39d4f3c0e1af71ed343e0ff2dafd:
>
>   Merge pull request #12 from torvalds/master (2015-12-19 11:12:20 -0700)
>
> are available in the git repository at:
>
>
>   https://github.com/jeffmerkey/linux.git fixes
>
> for you to fetch changes up to b5f894bf53e7c401cc5a88b8a8b13059a176a538:
>
>   Fix INT1 Recursion with unregistered breakpoints (2015-12-19 20:33:59 -0700)
>
> ----------------------------------------------------------------
> Jeff Merkey (1):
>       Fix INT1 Recursion with unregistered breakpoints
>
>  arch/x86/include/uapi/asm/debugreg.h |  1 +
>  arch/x86/kernel/hw_breakpoint.c      | 25 +++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
> index 3c0874d..78fc83c 100644
> --- a/arch/x86/include/uapi/asm/debugreg.h
> +++ b/arch/x86/include/uapi/asm/debugreg.h
> @@ -38,6 +38,7 @@
>  #define DR_RW_EXECUTE (0x0)   /* Settings for the access types to trap on */
>  #define DR_RW_WRITE (0x1)
>  #define DR_RW_READ (0x3)
> +#define DR_RW_MASK (0x3) /* mask for breakpoint type field */
>
>  #define DR_LEN_1 (0x0) /* Settings for data length to trap on */
>  #define DR_LEN_2 (0x4)
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 50a3fad..d199834 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -444,7 +444,7 @@ EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
>  static int hw_breakpoint_handler(struct die_args *args)
>  {
>         int i, cpu, rc = NOTIFY_STOP;
> -       struct perf_event *bp;
> +       struct perf_event *bp = NULL;
>         unsigned long dr7, dr6;
>         unsigned long *dr6_p;
>
> @@ -477,6 +477,14 @@ static int hw_breakpoint_handler(struct die_args *args)
>                         continue;
>
>                 /*
> +                * Check if we got an execute breakpoint, if so
> +                * set the resume flag to avoid int1 recursion.
> +                */
> +               if (((dr7 >> ((i * DR_CONTROL_SIZE) + DR_CONTROL_SHIFT))
> +                       & DR_RW_MASK) == DR_RW_EXECUTE)
> +                       args->regs->flags |= X86_EFLAGS_RF;
>

I'm still not at all sure I like this, for two reasons:

1. This duplicates the RF-setting logic with all of the legitimate
users.  I can see this blowing up in the case where a breakpoint API
user wants to clear a breakpoint and *not* set RF in case there's
another (possibly just-registered) breakpoint?

2. Shouldn't this clear the spurious breakpoint rather than leaving it set?

In any event, why not just change whatever you're doing to make this
blow up to use the actual breakpoint API?