Re: [PATCH 9/9] qspinlock,x86,kvm: Implement KVM support for paravirt qspinlock

[Peter Zijlstra]

On Wed, Mar 18, 2015 at 10:45:55PM -0400, Waiman Long wrote:
> On 03/16/2015 09:16 AM, Peter Zijlstra wrote:
> I do have some concern about this call site patching mechanism as the
> modification is not atomic. The spin_unlock() calls are in many places in
> the kernel. There is a possibility that a thread is calling a certain
> spin_unlock call site while it is being patched by another one with the
> alternative() function call.
> 
> So far, I don't see any problem with bare metal where paravirt_patch_insns()
> is used to patch it to the move instruction. However, in a virtual guest
> enivornment where paravirt_patch_call() was used, there were situations
> where the system panic because of page fault on some invalid memory in the
> kthread. If you look at the paravirt_patch_call(), you will see:
> 
>     :
> b->opcode = 0xe8; /* call */
> b->delta = delta;
> 
> If another CPU reads the instruction at the call site at the right moment,
> it will get the modified call instruction, but not the new delta value. It
> will then jump to a random location. I believe that was causing the system
> panic that I saw.
> 
> So I think it is kind of risky to use it here unless we can guarantee that
> call site patching is atomic wrt other CPUs.

Just look at where the patching is done:

init/main.c:start_kernel()
  check_bugs()
    alternative_instructions()
      apply_paravirt()

We're UP and not holding any locks, disable IRQs (see text_poke_early())
and have NMIs 'disabled'.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/