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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <550B3ACF.4050908@hp.com>
Date:	Thu, 19 Mar 2015 17:08:31 -0400
From:	Waiman Long <waiman.long@...com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
	paolo.bonzini@...il.com, konrad.wilk@...cle.com,
	boris.ostrovsky@...cle.com, paulmck@...ux.vnet.ibm.com,
	riel@...hat.com, torvalds@...ux-foundation.org,
	raghavendra.kt@...ux.vnet.ibm.com, david.vrabel@...rix.com,
	oleg@...hat.com, scott.norton@...com, doug.hatch@...com,
	linux-arch@...r.kernel.org, x86@...nel.org,
	linux-kernel@...r.kernel.org,
	virtualization@...ts.linux-foundation.org,
	xen-devel@...ts.xenproject.org, kvm@...r.kernel.org,
	luto@...capital.net
Subject: Re: [PATCH 9/9] qspinlock,x86,kvm: Implement KVM support for paravirt
 qspinlock

On 03/19/2015 06:01 AM, Peter Zijlstra wrote:
> 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'.

You are probably right. The initial apply_paravirt() was done before the 
SMP boot. Subsequent ones were at kernel module load time. I put a 
counter in the __native_queue_spin_unlock() and it registered 26949 
unlock calls in a 16-cpu guest before it got patched out.

The panic that I observed before might be due to some coding error of my 
own.

-Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/