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

Groups > linux.kernel > #1344070

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

Show all headers | View raw

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?

Back to linux.kernel | Previous | Next — Next in thread | Find similar | Unroll thread

Thread

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

csiph-web