linux-kernel - Re: [PATCH] KVM: Remove redundant smp_mb() in the kvm_mmu_commit_zap

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: <56E1A9D3.7080803@redhat.com>
Date:	Thu, 10 Mar 2016 18:07:31 +0100
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Lan Tianyu <tianyu.lan@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>
Cc:	gleb@...nel.org, mingo@...hat.com, hpa@...or.com, x86@...nel.org,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Xiao Guangrong <guangrong.xiao@...ux.intel.com>
Subject: Re: [PATCH] KVM: Remove redundant smp_mb() in the
 kvm_mmu_commit_zap_page()

On 09/03/2016 08:18, Lan Tianyu wrote:
> How about the following comments.
> 
> Log for kvm_mmu_commit_zap_page()
> 	/*
> 	 * We need to make sure everyone sees our modifications to
> 	 * the page tables and see changes to vcpu->mode here.

Please mention that this pairs with vcpu_enter_guest and
walk_shadow_page_lockless_begin/end.

> The
> 	 * barrier in the kvm_flush_remote_tlbs() helps us to achieve
> 	 * these. Otherwise, wait for all vcpus to exit guest mode
> 	 * and/or lockless shadow page table walks.
> 	 */
> 	kvm_flush_remote_tlbs(kvm);

The rest of the comment is okay, but please replace "Otherwise" with "In
addition, we need to".

> Log for kvm_flush_remote_tlbs()
> 	/*
> 	 * We want to publish modifications to the page tables before
> 	 * reading mode. Pairs with a memory barrier in arch-specific
> 	 * code.
> 	 * - x86: smp_mb__after_srcu_read_unlock in vcpu_enter_guest.

... and smp_mb in walk_shadow_page_lockless_begin/end.

> 	 * - powerpc: smp_mb in kvmppc_prepare_to_enter.
> 	 */
>  	 smp_mb__before_atomic();

The comment looks good, but the smp_mb__before_atomic() is not needed.
As mentioned in the reply to Guangrong, only a smp_load_acquire is required.

So the comment should say something like "There is already an smp_mb()
before kvm_make_all_cpus_request reads vcpu->mode.  We reuse that
barrier here.".

On top of this there is:

- the change to paging_tmpl.h that Guangrong posted, adding smp_wmb()
before each increment of vcpu->kvm->tlbs_dirty

- the change to smp_mb__after_atomic() in kvm_make_all_cpus_request

- if you want :) you can also replace the store+mb in
walk_shadow_page_lockless_begin with smp_store_mb, and the mb+store in
walk_shadow_page_lockless_end with smp_store_release.

Thanks,

Paolo