Fix INT1 recursion with unregistered breakpoints

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(-)

arch/x86/include/uapi/asm/debugreg.h +1 -0

index 3c0874d..78fc83c 100644
@@ -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)

arch/x86/kernel/hw_breakpoint.c +1 -1

index 50a3fad..d199834 100644
@@ -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;
 

Hi Thomas,

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

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

Quoted 8 lines · Jeff Merkey

Hi Thomas,

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

Quoted 2 lines

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

It seems to me that you're papering over some issue instead of fixing the root cause. If you're using the API, then either you're doing it wrong or the API is broken. Can you figure out which and fix it?

--Andy

Quoted 8 lines · Jeff Merkey

Hi Thomas,

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

Quoted 2 lines

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

It seems to me that you're papering over some issue instead of fixing the root cause. If you're using the API, then either you're doing it wrong or the API is broken. Can you figure out which and fix it?

--Andy

Quoted 9 lines · Andy Lutomirski

Quoted 8 lines · Jeff Merkey

Hi Thomas,

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

Quoted 9 lines

Quoted 2 lines

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

It seems to me that you're papering over some issue instead of fixing the root cause. If you're using the API, then either you're doing it wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Linux should not crash because someone triggered a breakpoint or one got triggered due to a program leaving some bits lying in a read only register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix in my patch, but it does matter to the overall robustness of Linux.

Quoted 23 lines · Jeff Merkey

Quoted 10 lines · Andy Lutomirski

Quoted 8 lines · Jeff Merkey

Hi Thomas,

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

Quoted 9 lines

Quoted 2 lines

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

It seems to me that you're papering over some issue instead of fixing the root cause. If you're using the API, then either you're doing it wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Quoted 7 lines

Linux should not crash because someone triggered a breakpoint or one got triggered due to a program leaving some bits lying in a read only register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

The API appears to work, though. Are you *sure* you're using it correctly? Are you telling the code in kernel/hw_breakpoint.c about your breakpoint?

Quoted 3 lines

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix in my patch, but it does matter to the overall robustness of Linux.

Robust against what, exactly? What's the bug?

I will grant that the comments about lazy dr7 switching are mystifying, and cleaning them up might be nice. But there's no adequate explanation of what the failure mode is, how to trigger it, or why your patch is a reasonable fix. As it stands, you're duplicating code.

--Andy

Quoted 42 lines · Andy Lutomirski

Quoted 23 lines · Jeff Merkey

Quoted 10 lines · Andy Lutomirski

Quoted 8 lines · Jeff Merkey

Hi Thomas,

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

Quoted 9 lines

Quoted 2 lines

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

It seems to me that you're papering over some issue instead of fixing the root cause. If you're using the API, then either you're doing it wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Quoted 7 lines

Linux should not crash because someone triggered a breakpoint or one got triggered due to a program leaving some bits lying in a read only register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

The API appears to work, though. Are you *sure* you're using it correctly? Are you telling the code in kernel/hw_breakpoint.c about your breakpoint?

Quoted 3 lines

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix in my patch, but it does matter to the overall robustness of Linux.

Robust against what, exactly? What's the bug?

Quoted 7 lines

I will grant that the comments about lazy dr7 switching are mystifying, and cleaning them up might be nice. But there's no adequate explanation of what the failure mode is, how to trigger it, or why your patch is a reasonable fix. As it stands, you're duplicating code.

--Andy

Andy,

Couple of things:

Would you like a copy of the test harness that creates this bug to test for yourself? I previously posted it on the list. If you don't have it, I'll provide it.

Since the dr6 bits get shifted around, it doesn't matter if the breakpoint was registered or not in the API because the broken handler will call NULL bp structures and crash whether its registered or not.

You keep asking the same questions and the answers are the the writeup for the patch, including how it is triggered, what triggers it, how I have triggered it, etc. and you are simply ignoring what's written
there (or you have not read it). It makes me wonder if you really
know and understand x86 debugger stuff.

Quoted 52 lines · Jeff Merkey

Quoted 43 lines · Andy Lutomirski

Quoted 23 lines · Jeff Merkey

Quoted 10 lines · Andy Lutomirski

Quoted 8 lines · Jeff Merkey

Hi Thomas,

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

Quoted 9 lines

Quoted 2 lines

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

It seems to me that you're papering over some issue instead of fixing the root cause. If you're using the API, then either you're doing it wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Quoted 7 lines

Linux should not crash because someone triggered a breakpoint or one got triggered due to a program leaving some bits lying in a read only register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

The API appears to work, though. Are you *sure* you're using it correctly? Are you telling the code in kernel/hw_breakpoint.c about your breakpoint?

Quoted 3 lines

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix in my patch, but it does matter to the overall robustness of Linux.

Robust against what, exactly? What's the bug?

Quoted 7 lines

I will grant that the comments about lazy dr7 switching are mystifying, and cleaning them up might be nice. But there's no adequate explanation of what the failure mode is, how to trigger it, or why your patch is a reasonable fix. As it stands, you're duplicating code.

--Andy

Quoted 7 lines

Andy,

Couple of things:

Would you like a copy of the test harness that creates this bug to test for yourself? I previously posted it on the list. If you don't have it, I'll provide it.

If you can send a short, buildable thing that triggers it, I'll read it.

Quoted 5 lines

Since the dr6 bits get shifted around, it doesn't matter if the breakpoint was registered or not in the API because the broken handler will call NULL bp structures and crash whether its registered or not.

And what exactly does this have to do with anything? Your patch is all about spurious breakpoints triggered by dr7 and should have nothing much to do with the value in dr6. Unless dr6 is missing a bit due to some issue, but you never suggested any problem like that.

Quoted 6 lines

You keep asking the same questions and the answers are the the writeup for the patch, including how it is triggered, what triggers it, how I have triggered it, etc. and you are simply ignoring what's written
there (or you have not read it). It makes me wonder if you really
know and understand x86 debugger stuff.

No, your writeup is long, hard to read, and doesn't address how any actual problem exists.

--Andy

Quoted 69 lines · Andy Lutomirski

Quoted 52 lines · Jeff Merkey

Quoted 43 lines · Andy Lutomirski

Quoted 23 lines · Jeff Merkey

Quoted 10 lines · Andy Lutomirski

Quoted 8 lines · Jeff Merkey

Hi Thomas,

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

Quoted 9 lines

Quoted 2 lines

So do you want me to expand the patch and clear the breakpoint? Just give the word and I'll get busy and GIT -R- DONE.

It seems to me that you're papering over some issue instead of fixing the root cause. If you're using the API, then either you're doing it wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Quoted 7 lines

Linux should not crash because someone triggered a breakpoint or one got triggered due to a program leaving some bits lying in a read only register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

The API appears to work, though. Are you *sure* you're using it correctly? Are you telling the code in kernel/hw_breakpoint.c about your breakpoint?

Quoted 3 lines

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix in my patch, but it does matter to the overall robustness of Linux.

Robust against what, exactly? What's the bug?

Quoted 7 lines

I will grant that the comments about lazy dr7 switching are mystifying, and cleaning them up might be nice. But there's no adequate explanation of what the failure mode is, how to trigger it, or why your patch is a reasonable fix. As it stands, you're duplicating code.

--Andy

Quoted 7 lines

Andy,

Couple of things:

Would you like a copy of the test harness that creates this bug to test for yourself? I previously posted it on the list. If you don't have it, I'll provide it.

If you can send a short, buildable thing that triggers it, I'll read it.

Quoted 5 lines

Since the dr6 bits get shifted around, it doesn't matter if the breakpoint was registered or not in the API because the broken handler will call NULL bp structures and crash whether its registered or not.

Quoted 5 lines

And what exactly does this have to do with anything? Your patch is all about spurious breakpoints triggered by dr7 and should have nothing much to do with the value in dr6. Unless dr6 is missing a bit due to some issue, but you never suggested any problem like that.

It's about setting the resume flag when an execute breakpoint occurs, no matter what caused the breakpoint. If is not set, the system will hang with that processor
hung on the same execution address. You cannot have an int1 exception path that does not set the resume flag which is the case here -- there should be no path
where this flag does not get set on an execute breakpoint.

Quoted 84 lines · Jeff Merkey

Quoted 77 lines · Andy Lutomirski

Quoted 59 lines · Jeff Merkey

Quoted 50 lines · Andy Lutomirski

Quoted 29 lines · Jeff Merkey

Quoted 17 lines · Andy Lutomirski

Quoted 11 lines · Jeff Merkey

Hi Thomas,

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

Quoted 3 lines

So do you want me to expand the patch and clear the breakpoint? Just
give the word and I'll get busy and GIT -R- DONE.

Quoted 8 lines

It seems to me that you're papering over some issue instead of fixing
the root cause. If you're using the API, then either you're doing it
wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Quoted 7 lines

Linux should not crash because someone triggered a breakpoint or one got triggered due to a program leaving some bits lying in a read only register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

The API appears to work, though. Are you *sure* you're using it correctly? Are you telling the code in kernel/hw_breakpoint.c about your breakpoint?

Quoted 4 lines

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix
in my patch, but it does matter to the overall robustness of Linux.

Robust against what, exactly? What's the bug?

Quoted 7 lines

I will grant that the comments about lazy dr7 switching are mystifying, and cleaning them up might be nice. But there's no adequate explanation of what the failure mode is, how to trigger it, or why your patch is a reasonable fix. As it stands, you're duplicating code.

--Andy

Quoted 7 lines

Andy,

Couple of things:

Would you like a copy of the test harness that creates this bug to test for yourself? I previously posted it on the list. If you don't have it, I'll provide it.

If you can send a short, buildable thing that triggers it, I'll read it.

Quoted 5 lines

Since the dr6 bits get shifted around, it doesn't matter if the breakpoint was registered or not in the API because the broken handler will call NULL bp structures and crash whether its registered or not.

Quoted 5 lines

And what exactly does this have to do with anything? Your patch is all about spurious breakpoints triggered by dr7 and should have nothing much to do with the value in dr6. Unless dr6 is missing a bit due to some issue, but you never suggested any problem like that.

Quoted 9 lines

It's about setting the resume flag when an execute breakpoint occurs, no matter
what caused the breakpoint. If is not set, the system will hang with that processor
hung on the same execution address. You cannot have an int1 exception path
that does not set the resume flag which is the case here -- there should be no path
where this flag does not get set on an execute breakpoint.

There are many, many ways that one can corrupt kernel state to break things. You could screw up IST state basically anywhere and crash. You could screw up GSBASE. You could poke bad values into pt_regs in a fast syscall and hit the infamous SYSRET failure. You can write a buggy .fault handler that returns success and doesn't actually do anything. And yes, you can set a bit in dr7 without telling the hw_breakpoint code about it and thus infinite loop.

Meanwhile, you keep claiming that kernel has a bug and that the bug can't be triggered without out-of-tree code. In my book, that's not a bug.

If you want to submit a nice clean patch to hw_breakpoint_handler to change the behavior on an unmatched breakpoint, then submit such a patch and justify why (a) the new behavior is better and (b) why it doesn't break any actual in-tree code.

Yes, hw_breakpoint_notify is a piece of shit. In particular, the hilarously indirect way in which it's invoked makes no sense whatsoever. Fixing that (as a separate patch) would be fantastic IMO. But putting extra workarounds into do_debug is not okay, IMO.

Quoted 103 lines · Andy Lutomirski

Quoted 84 lines · Jeff Merkey

Quoted 77 lines · Andy Lutomirski

Quoted 59 lines · Jeff Merkey

Quoted 50 lines · Andy Lutomirski

Quoted 29 lines · Jeff Merkey

Quoted 17 lines · Andy Lutomirski

Quoted 11 lines · Jeff Merkey

Hi Thomas,

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

Quoted 3 lines

So do you want me to expand the patch and clear the breakpoint? Just
give the word and I'll get busy and GIT -R- DONE.

Quoted 8 lines

It seems to me that you're papering over some issue instead of fixing
the root cause. If you're using the API, then either you're doing it
wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Quoted 7 lines

Linux should not crash because someone triggered a breakpoint or one got triggered due to a program leaving some bits lying in a read only register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

The API appears to work, though. Are you *sure* you're using it correctly? Are you telling the code in kernel/hw_breakpoint.c about your breakpoint?

Quoted 4 lines

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix
in my patch, but it does matter to the overall robustness of Linux.

Robust against what, exactly? What's the bug?

Quoted 7 lines

I will grant that the comments about lazy dr7 switching are mystifying, and cleaning them up might be nice. But there's no adequate explanation of what the failure mode is, how to trigger it, or why your patch is a reasonable fix. As it stands, you're duplicating code.

--Andy

Quoted 7 lines

Andy,

Couple of things:

Would you like a copy of the test harness that creates this bug to test for yourself? I previously posted it on the list. If you don't have it, I'll provide it.

If you can send a short, buildable thing that triggers it, I'll read it.

Quoted 5 lines

Since the dr6 bits get shifted around, it doesn't matter if the breakpoint was registered or not in the API because the broken handler will call NULL bp structures and crash whether its registered or not.

Quoted 5 lines

And what exactly does this have to do with anything? Your patch is all about spurious breakpoints triggered by dr7 and should have nothing much to do with the value in dr6. Unless dr6 is missing a bit due to some issue, but you never suggested any problem like that.

Quoted 9 lines

It's about setting the resume flag when an execute breakpoint occurs, no matter
what caused the breakpoint. If is not set, the system will hang with that processor
hung on the same execution address. You cannot have an int1 exception path
that does not set the resume flag which is the case here -- there should be no path
where this flag does not get set on an execute breakpoint.

There are many, many ways that one can corrupt kernel state to break things. You could screw up IST state basically anywhere and crash. You could screw up GSBASE. You could poke bad values into pt_regs in a fast syscall and hit the infamous SYSRET failure. You can write a buggy .fault handler that returns success and doesn't actually do anything. And yes, you can set a bit in dr7 without telling the hw_breakpoint code about it and thus infinite loop.

Quoted 4 lines

Meanwhile, you keep claiming that kernel has a bug and that the bug can't be triggered without out-of-tree code. In my book, that's not a bug.

The handler that fails to set the resume flag is in tree code.

Quoted 6 lines

If you want to submit a nice clean patch to hw_breakpoint_handler to change the behavior on an unmatched breakpoint, then submit such a patch and justify why (a) the new behavior is better and (b) why it doesn't break any actual in-tree code.

At last, a compromise -- accepted. In the meantime, put this patch in to get rid of the crash. I'll code up another series and you can help me by reviewing it and keeping me on my toes.

Quoted 110 lines · Jeff Merkey

Quoted 109 lines · Andy Lutomirski

Quoted 88 lines · Jeff Merkey

Quoted 81 lines · Andy Lutomirski

Quoted 61 lines · Jeff Merkey

Quoted 52 lines · Andy Lutomirski

Quoted 29 lines · Jeff Merkey

Quoted 17 lines · Andy Lutomirski

Quoted 11 lines · Jeff Merkey

Hi Thomas,

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

Quoted 3 lines

So do you want me to expand the patch and clear the breakpoint? Just
give the word and I'll get busy and GIT -R- DONE.

Quoted 8 lines

It seems to me that you're papering over some issue instead of fixing
the root cause. If you're using the API, then either you're doing it
wrong or the API is broken. Can you figure out which and fix it?

--Andy

Andy,

Quoted 9 lines

Linux should not crash because someone triggered a breakpoint or one
got triggered due to a program leaving some bits lying in a read only
register (DR6) which for some strange reason someone in the linux world decided could be used as local storage and to pass arguments between subsystems - a register intel designed to be read from for
status. I did not design what's in that API, I have to live with
it.

The API appears to work, though. Are you *sure* you're using it correctly? Are you telling the code in kernel/hw_breakpoint.c about your breakpoint?

Quoted 4 lines

So all I am asking is that we fix this issue. It does not matter to my debugger is this is fixed or not in Linux, since I carry the fix
in my patch, but it does matter to the overall robustness of Linux.

Robust against what, exactly? What's the bug?

Quoted 7 lines

I will grant that the comments about lazy dr7 switching are mystifying, and cleaning them up might be nice. But there's no adequate explanation of what the failure mode is, how to trigger it, or why your patch is a reasonable fix. As it stands, you're duplicating code.

--Andy

Quoted 7 lines

Andy,

Couple of things:

Would you like a copy of the test harness that creates this bug to test for yourself? I previously posted it on the list. If you don't have it, I'll provide it.

If you can send a short, buildable thing that triggers it, I'll read it.

Quoted 6 lines

Since the dr6 bits get shifted around, it doesn't matter if the breakpoint was registered or not in the API because the broken handler
will call NULL bp structures and crash whether its registered or not.

Quoted 5 lines

And what exactly does this have to do with anything? Your patch is all about spurious breakpoints triggered by dr7 and should have nothing much to do with the value in dr6. Unless dr6 is missing a bit due to some issue, but you never suggested any problem like that.

Quoted 10 lines

It's about setting the resume flag when an execute breakpoint occurs, no
matter
what caused the breakpoint. If is not set, the system will hang with that processor
hung on the same execution address. You cannot have an int1 exception path
that does not set the resume flag which is the case here -- there should be no path
where this flag does not get set on an execute breakpoint.

There are many, many ways that one can corrupt kernel state to break things. You could screw up IST state basically anywhere and crash. You could screw up GSBASE. You could poke bad values into pt_regs in a fast syscall and hit the infamous SYSRET failure. You can write a buggy .fault handler that returns success and doesn't actually do anything. And yes, you can set a bit in dr7 without telling the hw_breakpoint code about it and thus infinite loop.

Quoted 6 lines

Quoted 4 lines

Meanwhile, you keep claiming that kernel has a bug and that the bug can't be triggered without out-of-tree code. In my book, that's not a bug.

The handler that fails to set the resume flag is in tree code.

It's an unreachable code path.

Quoted 7 lines

Quoted 6 lines

If you want to submit a nice clean patch to hw_breakpoint_handler to change the behavior on an unmatched breakpoint, then submit such a patch and justify why (a) the new behavior is better and (b) why it doesn't break any actual in-tree code.

Quoted 5 lines

At last, a compromise -- accepted. In the meantime, put this patch in to get rid of the crash. I'll code up another series and you can help me by
reviewing it and keeping me on my toes.

No, because it still doesn't fix a bug *and* it's not a cleanup.

Anyway Andy,

If Linus wants to pull it, be my guest -- I think its a bug. If you
guys want to wait that's ok too. In the meantime, I totally agree that someone needs to fix the busted hw_breakpoint notify handling and I will focus on that. I appreciate the opportunity to help out and thanks for suggesting some areas I can.

Jeff

Quoted 53 lines · Jeff V. Merkey

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(-)

arch/x86/include/uapi/asm/debugreg.h +1 -0

index 3c0874d..78fc83c 100644
@@ -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)

arch/x86/kernel/hw_breakpoint.c +1 -1

index 50a3fad..d199834 100644
@@ -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)

arch/x86/kernel/hw_breakpoint.c static int hw_breakpoint_handler(struct die_args · line 487

Quoted 10 lines

arch/x86/kernel/hw_breakpoint.c +6 -0

                        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?