| Message ID | 20190204201854.2328-5-nitesh@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Guest Free Page Hinting | expand |
On Mon, Feb 4, 2019 at 2:11 PM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > > 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. > > Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> This seems kind of backwards to me. Why disable page poisoning instead of just not hinting about the free pages? There shouldn't be that many instances when page poisoning is enabled, and when it is it would make more sense to leave it enabled rather than silently disable it. > --- > include/linux/page_hinting.h | 8 ++++++++ > mm/page_poison.c | 2 +- > virt/kvm/page_hinting.c | 1 + > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h > index 2d7ff59f3f6a..e800c6b07561 100644 > --- a/include/linux/page_hinting.h > +++ b/include/linux/page_hinting.h > @@ -19,7 +19,15 @@ struct hypervisor_pages { > extern int guest_page_hinting_flag; > extern struct static_key_false guest_page_hinting_key; > extern struct smp_hotplug_thread hinting_threads; > +extern bool want_page_poisoning; > > int guest_page_hinting_sysctl(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos); > 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 > +} > diff --git a/mm/page_poison.c b/mm/page_poison.c > index f0c15e9017c0..9af96021133b 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 636990e7fbb3..be529f6f2bc0 100644 > --- a/virt/kvm/page_hinting.c > +++ b/virt/kvm/page_hinting.c > @@ -103,6 +103,7 @@ void guest_free_page(struct page *page, int order) > > local_irq_save(flags); > if (page_hinting_obj->kvm_pt_idx != MAX_FGPT_ENTRIES) { > + disable_page_poisoning(); > page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].pfn = > page_to_pfn(page); > page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].zonenum = At a minimum it seems like you should have some sort of warning message that you are disabling page poisoning rather than just silently turning it off.
On 2/7/19 12:23 PM, Alexander Duyck wrote: > On Mon, Feb 4, 2019 at 2:11 PM Nitesh Narayan Lal <nitesh@redhat.com> wrote: >> 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. >> >> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> > This seems kind of backwards to me. Why disable page poisoning instead > of just not hinting about the free pages? There shouldn't be that many > instances when page poisoning is enabled, and when it is it would make > more sense to leave it enabled rather than silently disable it. As I have mentioned in the cover email, I intend to reuse Wei's already merged work. This will enable the guest to communicate the poison value which is in use to the host. > >> --- >> include/linux/page_hinting.h | 8 ++++++++ >> mm/page_poison.c | 2 +- >> virt/kvm/page_hinting.c | 1 + >> 3 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h >> index 2d7ff59f3f6a..e800c6b07561 100644 >> --- a/include/linux/page_hinting.h >> +++ b/include/linux/page_hinting.h >> @@ -19,7 +19,15 @@ struct hypervisor_pages { >> extern int guest_page_hinting_flag; >> extern struct static_key_false guest_page_hinting_key; >> extern struct smp_hotplug_thread hinting_threads; >> +extern bool want_page_poisoning; >> >> int guest_page_hinting_sysctl(struct ctl_table *table, int write, >> void __user *buffer, size_t *lenp, loff_t *ppos); >> 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 >> +} >> diff --git a/mm/page_poison.c b/mm/page_poison.c >> index f0c15e9017c0..9af96021133b 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 636990e7fbb3..be529f6f2bc0 100644 >> --- a/virt/kvm/page_hinting.c >> +++ b/virt/kvm/page_hinting.c >> @@ -103,6 +103,7 @@ void guest_free_page(struct page *page, int order) >> >> local_irq_save(flags); >> if (page_hinting_obj->kvm_pt_idx != MAX_FGPT_ENTRIES) { >> + disable_page_poisoning(); >> page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].pfn = >> page_to_pfn(page); >> page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].zonenum = > At a minimum it seems like you should have some sort of warning > message that you are disabling page poisoning rather than just > silently turning it off.
On Thu, Feb 7, 2019 at 9:56 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > > > On 2/7/19 12:23 PM, Alexander Duyck wrote: > > On Mon, Feb 4, 2019 at 2:11 PM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > >> 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. > >> > >> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> > > This seems kind of backwards to me. Why disable page poisoning instead > > of just not hinting about the free pages? There shouldn't be that many > > instances when page poisoning is enabled, and when it is it would make > > more sense to leave it enabled rather than silently disable it. > As I have mentioned in the cover email, I intend to reuse Wei's already > merged work. > > This will enable the guest to communicate the poison value which is in > use to the host. That is far from being reliable given that you are having to buffer the pages for some period of time. I really think it would be better to just allow page poisoning to function and when you can support applying poison to a newly allocated page then you could look at re-enabling it. What I am getting at is that those that care about poisoning won't likely care about performance and I would lump the memory hinting in with other performance features.
On Thu, Feb 07, 2019 at 10:24:20AM -0800, Alexander Duyck wrote: > On Thu, Feb 7, 2019 at 9:56 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > > > > > > On 2/7/19 12:23 PM, Alexander Duyck wrote: > > > On Mon, Feb 4, 2019 at 2:11 PM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > > >> 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. > > >> > > >> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> > > > This seems kind of backwards to me. Why disable page poisoning instead > > > of just not hinting about the free pages? There shouldn't be that many > > > instances when page poisoning is enabled, and when it is it would make > > > more sense to leave it enabled rather than silently disable it. > > As I have mentioned in the cover email, I intend to reuse Wei's already > > merged work. > > > > This will enable the guest to communicate the poison value which is in > > use to the host. > > That is far from being reliable given that you are having to buffer > the pages for some period of time. I really think it would be better > to just allow page poisoning to function and when you can support > applying poison to a newly allocated page then you could look at > re-enabling it. > > What I am getting at is that those that care about poisoning won't > likely care about performance and I would lump the memory hinting in > with other performance features. It's not just a performance issue. There is an issue is with the host/guest API. Once host discards pages it currently always gives you back zero-filled pages. So guest that looks for the poison using e.g. unpoison_page will crash unless the poison value is 0. Idea behind current code is just to let host know: it can either be more careful with these pages, or skip them completely.
On Mon, Feb 04, 2019 at 03:18:51PM -0500, Nitesh Narayan Lal wrote: > 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. > > Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> IMHO it's better to take the approach of the existing balloon code and just send the poison value to host. Host can then avoid filling memory with zeroes. > --- > include/linux/page_hinting.h | 8 ++++++++ > mm/page_poison.c | 2 +- > virt/kvm/page_hinting.c | 1 + > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h > index 2d7ff59f3f6a..e800c6b07561 100644 > --- a/include/linux/page_hinting.h > +++ b/include/linux/page_hinting.h > @@ -19,7 +19,15 @@ struct hypervisor_pages { > extern int guest_page_hinting_flag; > extern struct static_key_false guest_page_hinting_key; > extern struct smp_hotplug_thread hinting_threads; > +extern bool want_page_poisoning; > > int guest_page_hinting_sysctl(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos); > 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 > +} > diff --git a/mm/page_poison.c b/mm/page_poison.c > index f0c15e9017c0..9af96021133b 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 636990e7fbb3..be529f6f2bc0 100644 > --- a/virt/kvm/page_hinting.c > +++ b/virt/kvm/page_hinting.c > @@ -103,6 +103,7 @@ void guest_free_page(struct page *page, int order) > > local_irq_save(flags); > if (page_hinting_obj->kvm_pt_idx != MAX_FGPT_ENTRIES) { > + disable_page_poisoning(); > page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].pfn = > page_to_pfn(page); > page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].zonenum = > -- > 2.17.2
diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h index 2d7ff59f3f6a..e800c6b07561 100644 --- a/include/linux/page_hinting.h +++ b/include/linux/page_hinting.h @@ -19,7 +19,15 @@ struct hypervisor_pages { extern int guest_page_hinting_flag; extern struct static_key_false guest_page_hinting_key; extern struct smp_hotplug_thread hinting_threads; +extern bool want_page_poisoning; int guest_page_hinting_sysctl(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos); 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 +} diff --git a/mm/page_poison.c b/mm/page_poison.c index f0c15e9017c0..9af96021133b 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 636990e7fbb3..be529f6f2bc0 100644 --- a/virt/kvm/page_hinting.c +++ b/virt/kvm/page_hinting.c @@ -103,6 +103,7 @@ void guest_free_page(struct page *page, int order) local_irq_save(flags); if (page_hinting_obj->kvm_pt_idx != MAX_FGPT_ENTRIES) { + disable_page_poisoning(); page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].pfn = page_to_pfn(page); page_hinting_obj->kvm_pt[page_hinting_obj->kvm_pt_idx].zonenum =
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. Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> --- include/linux/page_hinting.h | 8 ++++++++ mm/page_poison.c | 2 +- virt/kvm/page_hinting.c | 1 + 3 files changed, 10 insertions(+), 1 deletion(-)