On Mon, Jun 11, 2018 at 11:19:02AM -0400, nilal@xxxxxxxxxx wrote:
> From: Nitesh Narayan Lal <nilal@xxxxxxxxxx>
>
> This patch disables page poisoning if guest page hinting is enabled.
> It is required to avoid possible guest memory corruption errors.
> Page Poisoning is a feature in which the page is filled with a specific
> pattern of (0x00 or 0xaa) after arch_free_page and the same is verified
> before arch_alloc_page to prevent following issues:
> *information leak from the freed data
> *use after free bugs
> *memory corruption
> Selection of the pattern depends on the CONFIG_PAGE_POISONING_ZERO
> Once the guest pages which are supposed to be freed are sent to the
> hypervisor it frees them. After freeing the pages in the global list
> following things may happen:
> *Hypervisor reallocates the freed memory back to the guest
> *Hypervisor frees the memory and maps a different physical memory
> In order to prevent any information leak hypervisor before allocating
> memory to the guest fills it with zeroes.
> The issue arises when the pattern used for Page Poisoning is 0xaa while
> the newly allocated page received from the hypervisor by the guest is
> filled with the pattern 0x00. This will result in memory corruption errors.
Looks like it's a bugfix for your previous patches.
If true pls put it 1st in series, we don't want to
break things then unbreak them back.
> Signed-off-by: Nitesh Narayan Lal <nilal@xxxxxxxxxx>
I'd rather tell hypervisor what the poison value is.
See
virtio-balloon: VIRTIO_BALLOON_F_PAGE_POISON
if that's not advertised, I think you should disable free page hinting,
not poisoning.
> ---
> include/linux/page_hinting.h | 9 +++++++++
> mm/page_poison.c | 2 +-
> virt/kvm/page_hinting.c | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h
> index dd30644..b639078 100644
> --- a/include/linux/page_hinting.h
> +++ b/include/linux/page_hinting.h
> @@ -1,3 +1,4 @@
> +#include <linux/poison.h>
> #define MAX_FGPT_ENTRIES 1000
> /*
> * hypervisor_pages - It is a dummy structure passed with the hypercall.
> @@ -14,6 +15,7 @@ struct hypervisor_pages {
> extern struct hypervisor_pages hypervisor_pagelist[MAX_FGPT_ENTRIES];
> extern void (*request_hypercall)(void *, int);
> extern void *balloon_ptr;
> +extern bool want_page_poisoning;
>
> extern struct static_key_false guest_page_hinting_key;
> int guest_page_hinting_sysctl(struct ctl_table *table, int write,
> @@ -21,3 +23,10 @@ int guest_page_hinting_sysctl(struct ctl_table *table, int write,
> extern int guest_page_hinting_flag;
> void guest_alloc_page(struct page *page, int order);
> void guest_free_page(struct page *page, int order);
> +
> +static inline void disable_page_poisoning(void)
> +{
> +#ifdef CONFIG_PAGE_POISONING
> + want_page_poisoning = 0;
> +#endif
> +}
What if value is read at the same time? Seems racy...
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index aa2b3d3..91ce8ad 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -7,7 +7,7 @@
> #include <linux/poison.h>
> #include <linux/ratelimit.h>
>
> -static bool want_page_poisoning __read_mostly;
> +bool want_page_poisoning __read_mostly;
>
> static int __init early_page_poison_param(char *buf)
> {
> diff --git a/virt/kvm/page_hinting.c b/virt/kvm/page_hinting.c
> index 078a3be..57eddd0 100644
> --- a/virt/kvm/page_hinting.c
> +++ b/virt/kvm/page_hinting.c
> @@ -302,6 +302,7 @@ void guest_free_page(struct page *page, int order)
> * process context causing unwanted overwrites. This will be replaced
> * with a better solution to prevent such race conditions.
> */
> + disable_page_poisoning();
> local_irq_save(flags);
> free_page_obj = &get_cpu_var(kvm_pt)[0];
> trace_guest_free_page(page, order);
I think we would want to revert everything e.g. if balloon
is removed.
> --
> 2.9.5
