On 06/15/2018 10:44 AM, Michael S. Tsirkin wrote:
> 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.
Sure, I will put it 1st in my next patch-series.
>
>> 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.
The approach to notify the hypervisor about poison value makes sense. I
will make the required changes, thanks for pointing this out.
>
>
>> ---
>> 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...
Yeah, I agree. However, this will not be needed when I will notify the
hypervisor about the page poisoning value.
>
>> 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.
Yes, I missed this. I will make the required changes in my next
patch-series.
>
>> --
>> 2.9.5
--
Regards
Nitesh
Attachment:
signature.asc
Description: OpenPGP digital signature
