Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1344070 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] KVM: x86: fix missed hardware breakpoints Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-02-26 11:50 +0100
    Re: [PATCH] KVM: x86: fix missed hardware breakpoints Nadav Amit <nadav.amit@gmail.com> - 2016-02-26 12:30 +0100
      Re: [PATCH] KVM: x86: fix missed hardware breakpoints Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-02-26 12:50 +0100
      Re: [PATCH] KVM: x86: fix missed hardware breakpoints Paolo Bonzini <pbonzini@redhat.com> - 2016-02-26 13:00 +0100
    Re: [PATCH] KVM: x86: fix missed hardware breakpoints Paolo Bonzini <pbonzini@redhat.com> - 2016-02-26 12:50 +0100

#1344070 — Re: [PATCH] KVM: x86: fix missed hardware breakpoints

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?

[toc] | [next] | [standalone]

#1344181

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

[toc] | [prev] | [next] | [standalone]

#1344192

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

[toc] | [prev] | [next] | [standalone]

#1344195

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

[toc] | [prev] | [next] | [standalone]

#1344189

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web