linux-kernel - Re: [PATCH v7 09/11] KVM: MMU: introduce kvm_mmu_prepare_zap_obsolete

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: <20130523073708.GE26157@redhat.com>
Date:	Thu, 23 May 2013 10:37:08 +0300
From:	Gleb Natapov <gleb@...hat.com>
To:	Xiao Guangrong <xiaoguangrong@...ux.vnet.ibm.com>
Cc:	avi.kivity@...il.com, mtosatti@...hat.com, pbonzini@...hat.com,
	linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v7 09/11] KVM: MMU: introduce
 kvm_mmu_prepare_zap_obsolete_page

On Thu, May 23, 2013 at 02:31:47PM +0800, Xiao Guangrong wrote:
> On 05/23/2013 02:18 PM, Gleb Natapov wrote:
> > On Thu, May 23, 2013 at 02:13:06PM +0800, Xiao Guangrong wrote:
> >> On 05/23/2013 01:57 PM, Gleb Natapov wrote:
> >>> On Thu, May 23, 2013 at 03:55:58AM +0800, Xiao Guangrong wrote:
> >>>> It is only used to zap the obsolete page. Since the obsolete page
> >>>> will not be used, we need not spend time to find its unsync children
> >>>> out. Also, we delete the page from shadow page cache so that the page
> >>>> is completely isolated after call this function.
> >>>>
> >>>> The later patch will use it to collapse tlb flushes
> >>>>
> >>>> Signed-off-by: Xiao Guangrong <xiaoguangrong@...ux.vnet.ibm.com>
> >>>> ---
> >>>>  arch/x86/kvm/mmu.c |   46 +++++++++++++++++++++++++++++++++++++++++-----
> >>>>  1 files changed, 41 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> >>>> index 9b57faa..e676356 100644
> >>>> --- a/arch/x86/kvm/mmu.c
> >>>> +++ b/arch/x86/kvm/mmu.c
> >>>> @@ -1466,7 +1466,7 @@ static inline void kvm_mod_used_mmu_pages(struct kvm *kvm, int nr)
> >>>>  static void kvm_mmu_free_page(struct kvm_mmu_page *sp)
> >>>>  {
> >>>>  	ASSERT(is_empty_shadow_page(sp->spt));
> >>>> -	hlist_del(&sp->hash_link);
> >>>> +	hlist_del_init(&sp->hash_link);
> >>> Why do you need hlist_del_init() here? Why not move it into
> >>
> >> Since the hlist will be double freed. We will it like this:
> >>
> >> kvm_mmu_prepare_zap_obsolete_page(page, list);
> >> kvm_mmu_commit_zap_page(list);
> >>    kvm_mmu_free_page(page);
> >>
> >> The first place is kvm_mmu_prepare_zap_obsolete_page(page), which have
> >> deleted the hash list.
> >>
> >>> kvm_mmu_prepare_zap_page() like we discussed it here:
> >>> https://patchwork.kernel.org/patch/2580351/ instead of doing
> >>> it differently for obsolete and non obsolete pages?
> >>
> >> It is can break the hash-list walking: we should rescan the
> >> hash list once the page is prepared-ly zapped.
> >>
> >> I mentioned it in the changelog:
> >>
> >>   4): drop the patch which deleted page from hash list at the "prepare"
> >>       time since it can break the walk based on hash list.
> > Can you elaborate on how this can happen?
> 
> There is a example:
> 
> int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
> {
> 	struct kvm_mmu_page *sp;
> 	LIST_HEAD(invalid_list);
> 	int r;
> 
> 	pgprintk("%s: looking for gfn %llx\n", __func__, gfn);
> 	r = 0;
> 	spin_lock(&kvm->mmu_lock);
> 	for_each_gfn_indirect_valid_sp(kvm, sp, gfn) {
> 		pgprintk("%s: gfn %llx role %x\n", __func__, gfn,
> 			 sp->role.word);
> 		r = 1;
> 		kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
> 	}
> 	kvm_mmu_commit_zap_page(kvm, &invalid_list);
> 	spin_unlock(&kvm->mmu_lock);
> 
> 	return r;
> }
> 
> It works fine since kvm_mmu_prepare_zap_page does not touch the hash list.
> If we delete hlist in kvm_mmu_prepare_zap_page(), this kind of codes should
> be changed to:
> 
> restart:
> 	for_each_gfn_indirect_valid_sp(kvm, sp, gfn) {
> 		pgprintk("%s: gfn %llx role %x\n", __func__, gfn,
> 			 sp->role.word);
> 		r = 1;
> 		if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
> 			goto restart;
> 	}
> 	kvm_mmu_commit_zap_page(kvm, &invalid_list);
> 
Hmm, yes. So lets leave it as is and always commit invalid_list before
releasing lock in kvm_zap_obsolete_pages() or skip obsolete pages while
walking hash table. Former is clearer I think.

--
			Gleb.
--
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/