* [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages
@ 2009-03-26 23:52 Rusty Russell
2009-03-27 0:17 ` Jeremy Fitzhardinge
0 siblings, 1 reply; 6+ messages in thread
From: Rusty Russell @ 2009-03-26 23:52 UTC (permalink / raw)
To: lguest; +Cc: linux-kernel, Ingo Molnar, virtualization, Ingo Molnar
Impact: potential bugfix
In theory, the kernel could reuse the same page as pgdir for a new process
while the hypervisor keeps it cached. This would have undesirable results.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
arch/x86/include/asm/lguest_hcall.h | 1 +
arch/x86/lguest/boot.c | 8 ++++++++
drivers/lguest/hypercalls.c | 3 +++
drivers/lguest/lg.h | 1 +
drivers/lguest/page_tables.c | 16 ++++++++++++++++
5 files changed, 29 insertions(+)
diff --git a/drivers/lguest/hypercalls.c b/drivers/lguest/hypercalls.c
index 54d66f0..a160894 100644
--- a/drivers/lguest/hypercalls.c
+++ b/drivers/lguest/hypercalls.c
@@ -92,6 +92,9 @@ static void do_hcall(struct lg_cpu *cpu, struct hcall_args *args)
case LHCALL_NOTIFY:
cpu->pending_notify = args->arg1;
break;
+ case LHCALL_INVALIDATE_PGTABLE:
+ invalidate_pagetable(cpu, args->arg1);
+ break;
default:
/* It should be an architecture-specific hypercall. */
if (lguest_arch_do_hcall(cpu, args))
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index 65f0b8a..c3bdf0b 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -540,6 +551,13 @@ static void lguest_flush_tlb_kernel(void)
lazy_hcall(LHCALL_FLUSH_TLB, 1, 0, 0);
}
+/* This routine is called when a process exits, and we're throwing away the
+ * page table. */
+static void lguest_pgd_free(struct mm_struct *mm, pgd_t *pgd)
+{
+ lazy_hcall(LHCALL_INVALIDATE_PGTABLE, __pa(pgd), 0, 0);
+}
+
/*
* The Unadvanced Programmable Interrupt Controller.
*
@@ -1018,6 +1046,7 @@ __init void lguest_init(void)
pv_mmu_ops.lazy_mode.leave = lguest_leave_lazy_mode;
pv_mmu_ops.pte_update = lguest_pte_update;
pv_mmu_ops.pte_update_defer = lguest_pte_update;
+ pv_mmu_ops.pgd_free = lguest_pgd_free;
#ifdef CONFIG_X86_LOCAL_APIC
/* apic read/write intercepts */
diff --git a/drivers/lguest/lg.h b/drivers/lguest/lg.h
index 5faefea..363c231 100644
--- a/drivers/lguest/lg.h
+++ b/drivers/lguest/lg.h
@@ -174,6 +174,7 @@ void guest_set_pte(struct lg_cpu *cpu, unsigned long gpgdir,
unsigned long vaddr, pte_t val);
void map_switcher_in_guest(struct lg_cpu *cpu, struct lguest_pages *pages);
int demand_page(struct lg_cpu *cpu, unsigned long cr2, int errcode);
+void invalidate_pagetable(struct lg_cpu *cpu, unsigned long pgtable);
void pin_page(struct lg_cpu *cpu, unsigned long vaddr);
unsigned long guest_pa(struct lg_cpu *cpu, unsigned long vaddr);
void page_table_guest_data_init(struct lg_cpu *cpu);
diff --git a/drivers/lguest/page_tables.c b/drivers/lguest/page_tables.c
index 81d0c60..fd3e1f5 100644
--- a/drivers/lguest/page_tables.c
+++ b/drivers/lguest/page_tables.c
@@ -448,6 +571,22 @@ void guest_new_pagetable(struct lg_cpu *cpu, unsigned long pgtable)
pin_stack_pages(cpu);
}
+/* The Guest tells us when a page is no longer being used as a pagetable.
+ * Without this there can be a subtle bug where the same page gets re-used
+ * for a different process, and we think it's the same one as the one we have
+ * in our little pgdir cache. */
+void invalidate_pagetable(struct lg_cpu *cpu, unsigned long pgtable)
+{
+ unsigned int pgdir = find_pgdir(cpu->lg, pgtable);
+
+ if (pgdir != ARRAY_SIZE(cpu->lg->pgdirs)) {
+ if (pgdir == cpu->cpu_pgd)
+ kill_guest(cpu, "Attempt to invalidate in-use pgdir");
+ /* We set it to an invalid value. */
+ cpu->lg->pgdirs[pgdir].gpgdir = -1UL;
+ }
+}
+
/*H:470 Finally, a routine which throws away everything: all PGD entries in all
* the shadow page tables, including the Guest's kernel mappings. This is used
* when we destroy the Guest. */
diff --git a/arch/x86/include/asm/lguest_hcall.h b/arch/x86/include/asm/lguest_hcall.h
index 8f034ba..3e200ea 100644
--- a/arch/x86/include/asm/lguest_hcall.h
+++ b/arch/x86/include/asm/lguest_hcall.h
@@ -17,6 +17,7 @@
#define LHCALL_SET_PMD 15
#define LHCALL_LOAD_TLS 16
#define LHCALL_NOTIFY 17
+#define LHCALL_INVALIDATE_PGTABLE 18
#define LGUEST_TRAP_ENTRY 0x1F
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages
2009-03-26 23:52 [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages Rusty Russell
@ 2009-03-27 0:17 ` Jeremy Fitzhardinge
2009-03-27 1:24 ` Rusty Russell
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2009-03-27 0:17 UTC (permalink / raw)
To: Rusty Russell; +Cc: lguest, Ingo Molnar, linux-kernel, virtualization
Rusty Russell wrote:
> Impact: potential bugfix
>
> In theory, the kernel could reuse the same page as pgdir for a new process
> while the hypervisor keeps it cached. This would have undesirable results.
>
You can't just do this in tlb flush?
J
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
> arch/x86/include/asm/lguest_hcall.h | 1 +
> arch/x86/lguest/boot.c | 8 ++++++++
> drivers/lguest/hypercalls.c | 3 +++
> drivers/lguest/lg.h | 1 +
> drivers/lguest/page_tables.c | 16 ++++++++++++++++
> 5 files changed, 29 insertions(+)
>
> diff --git a/drivers/lguest/hypercalls.c b/drivers/lguest/hypercalls.c
> index 54d66f0..a160894 100644
> --- a/drivers/lguest/hypercalls.c
> +++ b/drivers/lguest/hypercalls.c
> @@ -92,6 +92,9 @@ static void do_hcall(struct lg_cpu *cpu, struct hcall_args *args)
> case LHCALL_NOTIFY:
> cpu->pending_notify = args->arg1;
> break;
> + case LHCALL_INVALIDATE_PGTABLE:
> + invalidate_pagetable(cpu, args->arg1);
> + break;
> default:
> /* It should be an architecture-specific hypercall. */
> if (lguest_arch_do_hcall(cpu, args))
> diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
> index 65f0b8a..c3bdf0b 100644
> --- a/arch/x86/lguest/boot.c
> +++ b/arch/x86/lguest/boot.c
> @@ -540,6 +551,13 @@ static void lguest_flush_tlb_kernel(void)
> lazy_hcall(LHCALL_FLUSH_TLB, 1, 0, 0);
> }
>
> +/* This routine is called when a process exits, and we're throwing away the
> + * page table. */
> +static void lguest_pgd_free(struct mm_struct *mm, pgd_t *pgd)
> +{
> + lazy_hcall(LHCALL_INVALIDATE_PGTABLE, __pa(pgd), 0, 0);
> +}
> +
> /*
> * The Unadvanced Programmable Interrupt Controller.
> *
> @@ -1018,6 +1046,7 @@ __init void lguest_init(void)
> pv_mmu_ops.lazy_mode.leave = lguest_leave_lazy_mode;
> pv_mmu_ops.pte_update = lguest_pte_update;
> pv_mmu_ops.pte_update_defer = lguest_pte_update;
> + pv_mmu_ops.pgd_free = lguest_pgd_free;
>
> #ifdef CONFIG_X86_LOCAL_APIC
> /* apic read/write intercepts */
> diff --git a/drivers/lguest/lg.h b/drivers/lguest/lg.h
> index 5faefea..363c231 100644
> --- a/drivers/lguest/lg.h
> +++ b/drivers/lguest/lg.h
> @@ -174,6 +174,7 @@ void guest_set_pte(struct lg_cpu *cpu, unsigned long gpgdir,
> unsigned long vaddr, pte_t val);
> void map_switcher_in_guest(struct lg_cpu *cpu, struct lguest_pages *pages);
> int demand_page(struct lg_cpu *cpu, unsigned long cr2, int errcode);
> +void invalidate_pagetable(struct lg_cpu *cpu, unsigned long pgtable);
> void pin_page(struct lg_cpu *cpu, unsigned long vaddr);
> unsigned long guest_pa(struct lg_cpu *cpu, unsigned long vaddr);
> void page_table_guest_data_init(struct lg_cpu *cpu);
> diff --git a/drivers/lguest/page_tables.c b/drivers/lguest/page_tables.c
> index 81d0c60..fd3e1f5 100644
> --- a/drivers/lguest/page_tables.c
> +++ b/drivers/lguest/page_tables.c
> @@ -448,6 +571,22 @@ void guest_new_pagetable(struct lg_cpu *cpu, unsigned long pgtable)
> pin_stack_pages(cpu);
> }
>
> +/* The Guest tells us when a page is no longer being used as a pagetable.
> + * Without this there can be a subtle bug where the same page gets re-used
> + * for a different process, and we think it's the same one as the one we have
> + * in our little pgdir cache. */
> +void invalidate_pagetable(struct lg_cpu *cpu, unsigned long pgtable)
> +{
> + unsigned int pgdir = find_pgdir(cpu->lg, pgtable);
> +
> + if (pgdir != ARRAY_SIZE(cpu->lg->pgdirs)) {
> + if (pgdir == cpu->cpu_pgd)
> + kill_guest(cpu, "Attempt to invalidate in-use pgdir");
> + /* We set it to an invalid value. */
> + cpu->lg->pgdirs[pgdir].gpgdir = -1UL;
> + }
> +}
> +
> /*H:470 Finally, a routine which throws away everything: all PGD entries in all
> * the shadow page tables, including the Guest's kernel mappings. This is used
> * when we destroy the Guest. */
> diff --git a/arch/x86/include/asm/lguest_hcall.h b/arch/x86/include/asm/lguest_hcall.h
> index 8f034ba..3e200ea 100644
> --- a/arch/x86/include/asm/lguest_hcall.h
> +++ b/arch/x86/include/asm/lguest_hcall.h
> @@ -17,6 +17,7 @@
> #define LHCALL_SET_PMD 15
> #define LHCALL_LOAD_TLS 16
> #define LHCALL_NOTIFY 17
> +#define LHCALL_INVALIDATE_PGTABLE 18
>
> #define LGUEST_TRAP_ENTRY 0x1F
>
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/virtualization
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages
2009-03-27 0:17 ` Jeremy Fitzhardinge
@ 2009-03-27 1:24 ` Rusty Russell
2009-03-27 16:28 ` Jeremy Fitzhardinge
2009-03-27 16:33 ` Jeremy Fitzhardinge
0 siblings, 2 replies; 6+ messages in thread
From: Rusty Russell @ 2009-03-27 1:24 UTC (permalink / raw)
To: Jeremy Fitzhardinge; +Cc: lguest, Ingo Molnar, linux-kernel, virtualization
On Friday 27 March 2009 10:47:32 Jeremy Fitzhardinge wrote:
> Rusty Russell wrote:
> > Impact: potential bugfix
> >
> > In theory, the kernel could reuse the same page as pgdir for a new process
> > while the hypervisor keeps it cached. This would have undesirable results.
> >
>
> You can't just do this in tlb flush?
I don't think so. The problem is that lguest tracks 4 toplevels, using random
replacement. This cache is indexed by cr3 value.
Lguest assumes it's told about all pte removals or changes, but simple
additions get faulted in. If a pgdir page gets reused we'll potentially have
stale values from its previous life as a pgdir, no?
Now, I haven't *seen* this happen...
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages
2009-03-27 1:24 ` Rusty Russell
@ 2009-03-27 16:28 ` Jeremy Fitzhardinge
2009-03-27 16:33 ` Jeremy Fitzhardinge
1 sibling, 0 replies; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2009-03-27 16:28 UTC (permalink / raw)
To: Rusty Russell; +Cc: lguest, Ingo Molnar, linux-kernel, virtualization
Rusty Russell wrote:
>> You can't just do this in tlb flush?
>>
>
> I don't think so. The problem is that lguest tracks 4 toplevels, using random
> replacement. This cache is indexed by cr3 value.
>
> Lguest assumes it's told about all pte removals or changes, but simple
> additions get faulted in. If a pgdir page gets reused we'll potentially have
> stale values from its previous life as a pgdir, no?
>
Yes, but when you get a tlb flush hypercall, couldn't you also look up
the corresponding shadow pte and zap it so that it will get repopulated
next time around? And a full tlb flush would just zap the entire
shadow. After all, the shadow pagetable is just a glorified
software-managed tlb...
J
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages
2009-03-27 1:24 ` Rusty Russell
2009-03-27 16:28 ` Jeremy Fitzhardinge
@ 2009-03-27 16:33 ` Jeremy Fitzhardinge
2009-03-28 6:37 ` Rusty Russell
1 sibling, 1 reply; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2009-03-27 16:33 UTC (permalink / raw)
To: Rusty Russell; +Cc: lguest, Ingo Molnar, linux-kernel, virtualization
Rusty Russell wrote:
> Now, I haven't *seen* this happen...
>
Oh, maybe its because you already zap the shadows on tlb flush, and the
kernel is careful to tlb flush every pagetable page before freeing...
J
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages
2009-03-27 16:33 ` Jeremy Fitzhardinge
@ 2009-03-28 6:37 ` Rusty Russell
0 siblings, 0 replies; 6+ messages in thread
From: Rusty Russell @ 2009-03-28 6:37 UTC (permalink / raw)
To: Jeremy Fitzhardinge; +Cc: lguest, Ingo Molnar, linux-kernel, virtualization
On Saturday 28 March 2009 03:03:04 Jeremy Fitzhardinge wrote:
> Rusty Russell wrote:
> > Now, I haven't *seen* this happen...
> >
>
> Oh, maybe its because you already zap the shadows on tlb flush, and the
> kernel is careful to tlb flush every pagetable page before freeing...
OK, you've convinced me. I'll drop this patch. It was as I was looking for
the inconsistent pte values that I thought of this issue.
Thanks!
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-28 6:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-26 23:52 [PATCH 3/5] lguest: avoid accidental recycling of pgdir pages Rusty Russell
2009-03-27 0:17 ` Jeremy Fitzhardinge
2009-03-27 1:24 ` Rusty Russell
2009-03-27 16:28 ` Jeremy Fitzhardinge
2009-03-27 16:33 ` Jeremy Fitzhardinge
2009-03-28 6:37 ` Rusty Russell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox