Re: [PATCH 0/2] KVM: fix overflow of zero page refcount with ksm running

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Apr 01, 2020 at 08:50:54PM +0800, Zhuang Yanying wrote:
> From: LinFeng <linfeng23@huawei.com>
> 
> We found that the !is_zero_page() in kvm_is_mmio_pfn() was
> submmited in commit:90cff5a8cc("KVM: check for !is_zero_pfn() in
> kvm_is_mmio_pfn()"), but reverted in commit:0ef2459983("kvm: fix
> kvm_is_mmio_pfn() and rename to kvm_is_reserved_pfn()").
> 
> Maybe just adding !is_zero_page() to kvm_is_reserved_pfn() is too
> rough. According to commit:e433e83bc3("KVM: MMU: Do not treat
> ZONE_DEVICE pages as being reserved"), special handling in some
> other flows is also need by zero_page, if we would treat zero_page
> as being reserved.
> 
> Well, as fixing all functions reference to kvm_is_reserved_pfn() in
> this patch, we found that only kvm_release_pfn_clean() and
> kvm_get_pfn() don't need special handling.
> 
> So, we thought why not only check is_zero_page() in before get and
> put page, and revert our last commit:31e813f38f("KVM: fix overflow
> of zero page refcount with ksm running") in master.
> Instead of adding !is_zero_page() in kvm_is_reserved_pfn(),
> new idea is as follow:
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 7f9ee2929cfe..f9a1f9cf188e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1695,7 +1695,8 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean);
> 
>  void kvm_release_pfn_clean(kvm_pfn_t pfn)
>  {
> -	if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
> +	if (!is_error_noslot_pfn(pfn) &&
> +	    (!kvm_is_reserved_pfn(pfn) || is_zero_pfn(pfn)))
>  		put_page(pfn_to_page(pfn));
>  }
>  EXPORT_SYMBOL_GPL(kvm_release_pfn_clean);
> @@ -1734,7 +1735,7 @@ EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed);
> 
>  void kvm_get_pfn(kvm_pfn_t pfn)
>  {
> -	if (!kvm_is_reserved_pfn(pfn))
> +	if (!kvm_is_reserved_pfn(pfn) || is_zero_pfn(pfn))
>  		get_page(pfn_to_page(pfn));
>  }
>  EXPORT_SYMBOL_GPL(kvm_get_pfn);
> 
> We are confused why ZONE_DEVICE not do this, but treating it as
> no reserved. Is it racy if we change only use the patch in cover letter,
> but not the series patches.
> 
> And we check the code of v4.9.y v4.10.y v4.11.y v4.12.y, this bug exists
> in v4.11.y and later, but not in v4.9.y v4.10.y or before.
> After commit:e86c59b1b1("mm/ksm: improve deduplication of zero pages
> with colouring"), ksm will use zero pages with colouring. This feature
> was added in v4.11.y, so I wonder why v4.9.y has this bug.
> 
> We use crash tools attaching to /proc/kcore to check the refcount of
> zero_page, then create and destroy vm. The refcount stays at 1 on v4.9.y,
> well it increases only after v4.11.y. Are you sure it is the same bug
> you run into? Is there something we missing?
> 
> LinFeng (1):
>   KVM: special handling of zero_page in some flows
> 
> Zhuang Yanying (1):
>   KVM: fix overflow of zero page refcount with ksm running
> 
>  arch/x86/kvm/mmu.c  | 2 ++
>  virt/kvm/kvm_main.c | 9 +++++----
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> -- 
> 2.23.0
> 
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>