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

[Jeff Merkey]

On 1/11/16, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> 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?
>

Hi Andy,

I agree with #2, we should clear the breakpoint.  As for #1, if
there's an execute breakpoint it MUST be cleared by setting the RF flags
or it will just fire off again when it sees the iretd from the int1
exception handler.  I
do use the breakpoint API, this showed up while debugging and
testing the API with "lazy debug register switching" and using kgdb.

So do you want me to expand the patch and clear the breakpoint?

:-)

Jeff