From mboxrd@z Thu Jan  1 00:00:00 1970
From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Subject: Re: [PATCH] KVM: x86: fix missed hardware breakpoints
Date: Fri, 26 Feb 2016 18:42:09 +0800
Message-ID: <56D02C01.2040400@linux.intel.com>
References: <1455879402-23009-1-git-send-email-pbonzini@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: oleg@redhat.com, namit@cs.technion.ac.il, avagin@gmail.com,
	stable@vger.kernel.org
To: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org
Return-path: <stable-owner@vger.kernel.org>
In-Reply-To: <1455879402-23009-1-git-send-email-pbonzini@redhat.com>
Sender: stable-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org



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?