[PATCH] KVM: x86: fix missed hardware breakpoints

[PATCH] KVM: x86: fix missed hardware breakpoints

[Paolo Bonzini]

Sometimes when setting a breakpoint a process doesn't stop on it.
This is because the debug registers are not loaded correctly on
VCPU load.

The following simple reproducer from Oleg Nesterov tries using debug
registers in two threads.  To see the bug, run a 2-VCPU guest under
"taskset -c 0", then run "./bp 0 1" inside the guest.

    #include <unistd.h>
    #include <signal.h>
    #include <stdlib.h>
    #include <stdio.h>
    #include <sys/wait.h>
    #include <sys/ptrace.h>
    #include <sys/user.h>
    #include <asm/debugreg.h>
    #include <assert.h>

    #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)

    unsigned long encode_dr7(int drnum, int enable, unsigned int type, unsigned int len)
    {
        unsigned long dr7;

        dr7 = ((len | type) & 0xf)
            << (DR_CONTROL_SHIFT + drnum * DR_CONTROL_SIZE);
        if (enable)
            dr7 |= (DR_GLOBAL_ENABLE << (drnum * DR_ENABLE_SIZE));

        return dr7;
    }

    int write_dr(int pid, int dr, unsigned long val)
    {
        return ptrace(PTRACE_POKEUSER, pid,
                offsetof (struct user, u_debugreg[dr]),
                val);
    }

    void set_bp(pid_t pid, void *addr)
    {
        unsigned long dr7;
        assert(write_dr(pid, 0, (long)addr) == 0);
        dr7 = encode_dr7(0, 1, DR_RW_EXECUTE, DR_LEN_1);
        assert(write_dr(pid, 7, dr7) == 0);
    }

    void *get_rip(int pid)
    {
        return (void*)ptrace(PTRACE_PEEKUSER, pid,
                offsetof(struct user, regs.rip), 0);
    }

    void test(int nr)
    {
        void *bp_addr = &&label + nr, *bp_hit;
        int pid;

        printf("test bp %d\n", nr);
        assert(nr < 16); // see 16 asm nops below

        pid = fork();
        if (!pid) {
            assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
            kill(getpid(), SIGSTOP);
            for (;;) {
                label: asm (
                    "nop; nop; nop; nop;"
                    "nop; nop; nop; nop;"
                    "nop; nop; nop; nop;"
                    "nop; nop; nop; nop;"
                );
            }
        }

        assert(pid == wait(NULL));
        set_bp(pid, bp_addr);

        for (;;) {
            assert(ptrace(PTRACE_CONT, pid, 0, 0) == 0);
            assert(pid == wait(NULL));

            bp_hit = get_rip(pid);
            if (bp_hit != bp_addr)
                fprintf(stderr, "ERR!! hit wrong bp %ld != %d\n",
                    bp_hit - &&label, nr);
        }
    }

    int main(int argc, const char *argv[])
    {
        while (--argc) {
            int nr = atoi(*++argv);
            if (!fork())
                test(nr);
        }

        while (wait(NULL) > 0)
            ;
        return 0;
    }

Cc: stable@vger.kernel.org
Suggested-by: Nadav Amit <namit@cs.technion.ac.il>
Reported-by: Andrey Wagin <avagin@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Andrey, sorry for the time it took to get to this one.

 arch/x86/kvm/x86.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4244c2baf57d..f4891f2ece23 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2752,6 +2752,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	}
 
 	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
+	vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
-- 
1.8.3.1

Re: [PATCH] KVM: x86: fix missed hardware breakpoints

[Xiao Guangrong]

On 02/19/2016 06:56 PM, Paolo Bonzini wrote:
> Sometimes when setting a breakpoint a process doesn't stop on it.
> This is because the debug registers are not loaded correctly on
> VCPU load.
>
> The following simple reproducer from Oleg Nesterov tries using debug
> registers in two threads.  To see the bug, run a 2-VCPU guest under
> "taskset -c 0", then run "./bp 0 1" inside the guest.
>
>      #include <unistd.h>
>      #include <signal.h>
>      #include <stdlib.h>
>      #include <stdio.h>
>      #include <sys/wait.h>
>      #include <sys/ptrace.h>
>      #include <sys/user.h>
>      #include <asm/debugreg.h>
>      #include <assert.h>
>
>      #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
>
>      unsigned long encode_dr7(int drnum, int enable, unsigned int type, unsigned int len)
>      {
>          unsigned long dr7;
>
>          dr7 = ((len | type) & 0xf)
>              << (DR_CONTROL_SHIFT + drnum * DR_CONTROL_SIZE);
>          if (enable)
>              dr7 |= (DR_GLOBAL_ENABLE << (drnum * DR_ENABLE_SIZE));
>
>          return dr7;
>      }
>
>      int write_dr(int pid, int dr, unsigned long val)
>      {
>          return ptrace(PTRACE_POKEUSER, pid,
>                  offsetof (struct user, u_debugreg[dr]),
>                  val);
>      }
>
>      void set_bp(pid_t pid, void *addr)
>      {
>          unsigned long dr7;
>          assert(write_dr(pid, 0, (long)addr) == 0);
>          dr7 = encode_dr7(0, 1, DR_RW_EXECUTE, DR_LEN_1);
>          assert(write_dr(pid, 7, dr7) == 0);
>      }
>
>      void *get_rip(int pid)
>      {
>          return (void*)ptrace(PTRACE_PEEKUSER, pid,
>                  offsetof(struct user, regs.rip), 0);
>      }
>
>      void test(int nr)
>      {
>          void *bp_addr = &&label + nr, *bp_hit;
>          int pid;
>
>          printf("test bp %d\n", nr);
>          assert(nr < 16); // see 16 asm nops below
>
>          pid = fork();
>          if (!pid) {
>              assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
>              kill(getpid(), SIGSTOP);
>              for (;;) {
>                  label: asm (
>                      "nop; nop; nop; nop;"
>                      "nop; nop; nop; nop;"
>                      "nop; nop; nop; nop;"
>                      "nop; nop; nop; nop;"
>                  );
>              }
>          }
>
>          assert(pid == wait(NULL));
>          set_bp(pid, bp_addr);
>
>          for (;;) {
>              assert(ptrace(PTRACE_CONT, pid, 0, 0) == 0);
>              assert(pid == wait(NULL));
>
>              bp_hit = get_rip(pid);
>              if (bp_hit != bp_addr)
>                  fprintf(stderr, "ERR!! hit wrong bp %ld != %d\n",
>                      bp_hit - &&label, nr);
>          }
>      }
>
>      int main(int argc, const char *argv[])
>      {
>          while (--argc) {
>              int nr = atoi(*++argv);
>              if (!fork())
>                  test(nr);
>          }
>
>          while (wait(NULL) > 0)
>              ;
>          return 0;
>      }
>
> Cc: stable@vger.kernel.org
> Suggested-by: Nadav Amit <namit@cs.technion.ac.il>
> Reported-by: Andrey Wagin <avagin@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	Andrey, sorry for the time it took to get to this one.
>
>   arch/x86/kvm/x86.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4244c2baf57d..f4891f2ece23 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2752,6 +2752,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   	}
>
>   	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
> +	vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;

Er, i do not understand how it works. The BP is enabled in this test case so
the debug registers are always reloaded before entering guest as
KVM_DEBUGREG_BP_ENABLED bit is always set on switch_db_regs. What did i miss?

Another impact of this fix is when vcpu is rescheduled we need to always reload
debug registers even if guest does not enable it, it is really needed?

Re: [PATCH] KVM: x86: fix missed hardware breakpoints

[Nadav Amit]

Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 02/19/2016 06:56 PM, Paolo Bonzini wrote:
>> Sometimes when setting a breakpoint a process doesn't stop on it.
>> This is because the debug registers are not loaded correctly on
>> VCPU load.
>> 
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 4244c2baf57d..f4891f2ece23 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2752,6 +2752,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  	}
>> 
>>  	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>> +	vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
> 
> Er, i do not understand how it works. The BP is enabled in this test case so
> the debug registers are always reloaded before entering guest as
> KVM_DEBUGREG_BP_ENABLED bit is always set on switch_db_regs. What did i miss?

Note that KVM_DEBUGREG_BP_ENABLED does not have to be set, since
kvm_update_dr7() is not called once the guest has KVM_DEBUGREG_WONT_EXIT.

Nadav

Re: [PATCH] KVM: x86: fix missed hardware breakpoints

[Xiao Guangrong]

On 02/26/2016 07:28 PM, Nadav Amit wrote:
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> On 02/19/2016 06:56 PM, Paolo Bonzini wrote:
>>> Sometimes when setting a breakpoint a process doesn't stop on it.
>>> This is because the debug registers are not loaded correctly on
>>> VCPU load.
>>>
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 4244c2baf57d..f4891f2ece23 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -2752,6 +2752,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>>   	}
>>>
>>>   	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>>> +	vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
>>
>> Er, i do not understand how it works. The BP is enabled in this test case so
>> the debug registers are always reloaded before entering guest as
>> KVM_DEBUGREG_BP_ENABLED bit is always set on switch_db_regs. What did i miss?
>
> Note that KVM_DEBUGREG_BP_ENABLED does not have to be set, since
> kvm_update_dr7() is not called once the guest has KVM_DEBUGREG_WONT_EXIT.

Ah, yes. So that we will enter guest without reloading debug registers under this
case. Is it safe? What about if gdb/perf attached to QEMU and reset the debug0－３
(in this case, the vcpu is interrupted by IPI and it is not rescheduled)？

Re: [PATCH] KVM: x86: fix missed hardware breakpoints

[Paolo Bonzini]

On 26/02/2016 12:28, Nadav Amit wrote:
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> 
>> On 02/19/2016 06:56 PM, Paolo Bonzini wrote:
>>> Sometimes when setting a breakpoint a process doesn't stop on it.
>>> This is because the debug registers are not loaded correctly on
>>> VCPU load.
>>>
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 4244c2baf57d..f4891f2ece23 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -2752,6 +2752,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>>  	}
>>>
>>>  	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>>> +	vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
>>
>> Er, i do not understand how it works. The BP is enabled in this test case so
>> the debug registers are always reloaded before entering guest as
>> KVM_DEBUGREG_BP_ENABLED bit is always set on switch_db_regs. What did i miss?
> 
> Note that KVM_DEBUGREG_BP_ENABLED does not have to be set, since
> kvm_update_dr7() is not called once the guest has KVM_DEBUGREG_WONT_EXIT.

Looks like our emails crossed; could you review the patch I have just sent?

Paolo

Re: [PATCH] KVM: x86: fix missed hardware breakpoints

[Paolo Bonzini]

On 26/02/2016 11:42, Xiao Guangrong wrote:
>>
>> +    vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
> 
> Er, i do not understand how it works. The BP is enabled in this test case so
> the debug registers are always reloaded before entering guest as
> KVM_DEBUGREG_BP_ENABLED bit is always set on switch_db_regs. What did i
> miss?
> 
> Another impact of this fix is when vcpu is rescheduled we need to always
> reload debug registers even if guest does not enable it, it is really needed?

Hi,

I have looked further at the bug and the issue is that the lazy debug
register optimization doesn't call kvm_update_dr7 and thus does not set
KVM_DEBUGREG_BP_ENABLED.  I will post a better patch shortly.  However,
I still think this one is simpler to have in stable kernel releases,
because it doesn't have any dependencies.

Paolo