* [PATCH 0/2] A couple of EL2 refcounts fixes @ 2021-10-04 9:03 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team Hi all, This addresses a couple of issues Will has found with the refcounting of page-tables at EL2. Patch 01 fixes a nasty bug, and probably wants to go in -stable. Patch 02 fixes a small inconsistency which made it harder to find refcount-related bugs at EL2. Feedback welcome ! Thanks, Quentin Quentin Perret (2): KVM: arm64: Fix host stage-2 PGD refcount KVM: arm64: Report corrupted refcount at EL2 arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) -- 2.33.0.800.g4c38ced690-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/2] A couple of EL2 refcounts fixes @ 2021-10-04 9:03 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team Hi all, This addresses a couple of issues Will has found with the refcounting of page-tables at EL2. Patch 01 fixes a nasty bug, and probably wants to go in -stable. Patch 02 fixes a small inconsistency which made it harder to find refcount-related bugs at EL2. Feedback welcome ! Thanks, Quentin Quentin Perret (2): KVM: arm64: Fix host stage-2 PGD refcount KVM: arm64: Report corrupted refcount at EL2 arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) -- 2.33.0.800.g4c38ced690-goog ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/2] A couple of EL2 refcounts fixes @ 2021-10-04 9:03 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team Hi all, This addresses a couple of issues Will has found with the refcounting of page-tables at EL2. Patch 01 fixes a nasty bug, and probably wants to go in -stable. Patch 02 fixes a small inconsistency which made it harder to find refcount-related bugs at EL2. Feedback welcome ! Thanks, Quentin Quentin Perret (2): KVM: arm64: Fix host stage-2 PGD refcount KVM: arm64: Report corrupted refcount at EL2 arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) -- 2.33.0.800.g4c38ced690-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount 2021-10-04 9:03 ` Quentin Perret (?) @ 2021-10-04 9:03 ` Quentin Perret -1 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team The KVM page-table library refcounts the pages of concatenated stage-2 PGDs individually. However, the host's stage-2 PGD is currently managed by EL2 as a single high-order compound page, which can cause the refcount of the tail pages to reach 0 when they really shouldn't, hence corrupting the page-table. Fix this by introducing a new hyp_split_page() helper in the EL2 page allocator (matching EL1's split_page() function), and make use of it from host_s2_zalloc_page(). Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h index fb0f523d1492..0a048dc06a7d 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h @@ -24,6 +24,7 @@ struct hyp_pool { /* Allocation */ void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); +void hyp_split_page(struct hyp_page *page); void hyp_get_page(struct hyp_pool *pool, void *addr); void hyp_put_page(struct hyp_pool *pool, void *addr); diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index bacd493a4eac..93a79736c283 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; static void *host_s2_zalloc_pages_exact(size_t size) { - return hyp_alloc_pages(&host_s2_pool, get_order(size)); + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); + + hyp_split_page(hyp_virt_to_page(addr)); + + return addr; } static void *host_s2_zalloc_page(void *pool) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index 41fc25bdfb34..a6e874e61a40 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) hyp_spin_unlock(&pool->lock); } +void hyp_split_page(struct hyp_page *p) +{ + unsigned short order = p->order; + unsigned int i; + + p->order = 0; + for (i = 1; i < (1 << order); i++) { + struct hyp_page *tail = p + i; + + tail->order = 0; + hyp_set_page_refcounted(tail); + } +} + void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) { unsigned short i = order; -- 2.33.0.800.g4c38ced690-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-04 9:03 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team The KVM page-table library refcounts the pages of concatenated stage-2 PGDs individually. However, the host's stage-2 PGD is currently managed by EL2 as a single high-order compound page, which can cause the refcount of the tail pages to reach 0 when they really shouldn't, hence corrupting the page-table. Fix this by introducing a new hyp_split_page() helper in the EL2 page allocator (matching EL1's split_page() function), and make use of it from host_s2_zalloc_page(). Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h index fb0f523d1492..0a048dc06a7d 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h @@ -24,6 +24,7 @@ struct hyp_pool { /* Allocation */ void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); +void hyp_split_page(struct hyp_page *page); void hyp_get_page(struct hyp_pool *pool, void *addr); void hyp_put_page(struct hyp_pool *pool, void *addr); diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index bacd493a4eac..93a79736c283 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; static void *host_s2_zalloc_pages_exact(size_t size) { - return hyp_alloc_pages(&host_s2_pool, get_order(size)); + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); + + hyp_split_page(hyp_virt_to_page(addr)); + + return addr; } static void *host_s2_zalloc_page(void *pool) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index 41fc25bdfb34..a6e874e61a40 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) hyp_spin_unlock(&pool->lock); } +void hyp_split_page(struct hyp_page *p) +{ + unsigned short order = p->order; + unsigned int i; + + p->order = 0; + for (i = 1; i < (1 << order); i++) { + struct hyp_page *tail = p + i; + + tail->order = 0; + hyp_set_page_refcounted(tail); + } +} + void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) { unsigned short i = order; -- 2.33.0.800.g4c38ced690-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-04 9:03 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team The KVM page-table library refcounts the pages of concatenated stage-2 PGDs individually. However, the host's stage-2 PGD is currently managed by EL2 as a single high-order compound page, which can cause the refcount of the tail pages to reach 0 when they really shouldn't, hence corrupting the page-table. Fix this by introducing a new hyp_split_page() helper in the EL2 page allocator (matching EL1's split_page() function), and make use of it from host_s2_zalloc_page(). Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h index fb0f523d1492..0a048dc06a7d 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h @@ -24,6 +24,7 @@ struct hyp_pool { /* Allocation */ void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); +void hyp_split_page(struct hyp_page *page); void hyp_get_page(struct hyp_pool *pool, void *addr); void hyp_put_page(struct hyp_pool *pool, void *addr); diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index bacd493a4eac..93a79736c283 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; static void *host_s2_zalloc_pages_exact(size_t size) { - return hyp_alloc_pages(&host_s2_pool, get_order(size)); + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); + + hyp_split_page(hyp_virt_to_page(addr)); + + return addr; } static void *host_s2_zalloc_page(void *pool) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index 41fc25bdfb34..a6e874e61a40 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) hyp_spin_unlock(&pool->lock); } +void hyp_split_page(struct hyp_page *p) +{ + unsigned short order = p->order; + unsigned int i; + + p->order = 0; + for (i = 1; i < (1 << order); i++) { + struct hyp_page *tail = p + i; + + tail->order = 0; + hyp_set_page_refcounted(tail); + } +} + void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) { unsigned short i = order; -- 2.33.0.800.g4c38ced690-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount 2021-10-04 9:03 ` Quentin Perret (?) @ 2021-10-04 9:55 ` Marc Zyngier -1 siblings, 0 replies; 21+ messages in thread From: Marc Zyngier @ 2021-10-04 9:55 UTC (permalink / raw) To: Quentin Perret Cc: kernel-team, Will Deacon, Catalin Marinas, linux-kernel, linux-arm-kernel, kvmarm Hi Quentin, On Mon, 04 Oct 2021 10:03:13 +0100, Quentin Perret <qperret@google.com> wrote: > > The KVM page-table library refcounts the pages of concatenated stage-2 > PGDs individually. However, the host's stage-2 PGD is currently managed > by EL2 as a single high-order compound page, which can cause the > refcount of the tail pages to reach 0 when they really shouldn't, hence > corrupting the page-table. nit: this comment only applies to the protected mode, right? As far as I can tell, 'classic' KVM is just fine. > > Fix this by introducing a new hyp_split_page() helper in the EL2 page > allocator (matching EL1's split_page() function), and make use of it uber nit: split_page() is not an EL1 function. more of a standard kernel function. > from host_s2_zalloc_page(). > > Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- > arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > index fb0f523d1492..0a048dc06a7d 100644 > --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h > +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > @@ -24,6 +24,7 @@ struct hyp_pool { > > /* Allocation */ > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); > +void hyp_split_page(struct hyp_page *page); > void hyp_get_page(struct hyp_pool *pool, void *addr); > void hyp_put_page(struct hyp_pool *pool, void *addr); > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > index bacd493a4eac..93a79736c283 100644 > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; > > static void *host_s2_zalloc_pages_exact(size_t size) > { > - return hyp_alloc_pages(&host_s2_pool, get_order(size)); > + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); > + > + hyp_split_page(hyp_virt_to_page(addr)); The only reason this doesn't lead to a subsequent memory leak is that concatenated page tables are always a power of two, right? If so, that deserves a comment, because I don't think this works in the general case unless you actively free the pages that are between size and (1 << order). > + > + return addr; > } > > static void *host_s2_zalloc_page(void *pool) > diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c > index 41fc25bdfb34..a6e874e61a40 100644 > --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c > +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c > @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) > hyp_spin_unlock(&pool->lock); > } > > +void hyp_split_page(struct hyp_page *p) > +{ > + unsigned short order = p->order; > + unsigned int i; > + > + p->order = 0; > + for (i = 1; i < (1 << order); i++) { > + struct hyp_page *tail = p + i; > + > + tail->order = 0; > + hyp_set_page_refcounted(tail); > + } > +} > + > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) > { > unsigned short i = order; Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-04 9:55 ` Marc Zyngier 0 siblings, 0 replies; 21+ messages in thread From: Marc Zyngier @ 2021-10-04 9:55 UTC (permalink / raw) To: Quentin Perret Cc: James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel, kernel-team Hi Quentin, On Mon, 04 Oct 2021 10:03:13 +0100, Quentin Perret <qperret@google.com> wrote: > > The KVM page-table library refcounts the pages of concatenated stage-2 > PGDs individually. However, the host's stage-2 PGD is currently managed > by EL2 as a single high-order compound page, which can cause the > refcount of the tail pages to reach 0 when they really shouldn't, hence > corrupting the page-table. nit: this comment only applies to the protected mode, right? As far as I can tell, 'classic' KVM is just fine. > > Fix this by introducing a new hyp_split_page() helper in the EL2 page > allocator (matching EL1's split_page() function), and make use of it uber nit: split_page() is not an EL1 function. more of a standard kernel function. > from host_s2_zalloc_page(). > > Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- > arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > index fb0f523d1492..0a048dc06a7d 100644 > --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h > +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > @@ -24,6 +24,7 @@ struct hyp_pool { > > /* Allocation */ > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); > +void hyp_split_page(struct hyp_page *page); > void hyp_get_page(struct hyp_pool *pool, void *addr); > void hyp_put_page(struct hyp_pool *pool, void *addr); > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > index bacd493a4eac..93a79736c283 100644 > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; > > static void *host_s2_zalloc_pages_exact(size_t size) > { > - return hyp_alloc_pages(&host_s2_pool, get_order(size)); > + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); > + > + hyp_split_page(hyp_virt_to_page(addr)); The only reason this doesn't lead to a subsequent memory leak is that concatenated page tables are always a power of two, right? If so, that deserves a comment, because I don't think this works in the general case unless you actively free the pages that are between size and (1 << order). > + > + return addr; > } > > static void *host_s2_zalloc_page(void *pool) > diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c > index 41fc25bdfb34..a6e874e61a40 100644 > --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c > +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c > @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) > hyp_spin_unlock(&pool->lock); > } > > +void hyp_split_page(struct hyp_page *p) > +{ > + unsigned short order = p->order; > + unsigned int i; > + > + p->order = 0; > + for (i = 1; i < (1 << order); i++) { > + struct hyp_page *tail = p + i; > + > + tail->order = 0; > + hyp_set_page_refcounted(tail); > + } > +} > + > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) > { > unsigned short i = order; Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-04 9:55 ` Marc Zyngier 0 siblings, 0 replies; 21+ messages in thread From: Marc Zyngier @ 2021-10-04 9:55 UTC (permalink / raw) To: Quentin Perret Cc: James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel, kernel-team Hi Quentin, On Mon, 04 Oct 2021 10:03:13 +0100, Quentin Perret <qperret@google.com> wrote: > > The KVM page-table library refcounts the pages of concatenated stage-2 > PGDs individually. However, the host's stage-2 PGD is currently managed > by EL2 as a single high-order compound page, which can cause the > refcount of the tail pages to reach 0 when they really shouldn't, hence > corrupting the page-table. nit: this comment only applies to the protected mode, right? As far as I can tell, 'classic' KVM is just fine. > > Fix this by introducing a new hyp_split_page() helper in the EL2 page > allocator (matching EL1's split_page() function), and make use of it uber nit: split_page() is not an EL1 function. more of a standard kernel function. > from host_s2_zalloc_page(). > > Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- > arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > index fb0f523d1492..0a048dc06a7d 100644 > --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h > +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > @@ -24,6 +24,7 @@ struct hyp_pool { > > /* Allocation */ > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); > +void hyp_split_page(struct hyp_page *page); > void hyp_get_page(struct hyp_pool *pool, void *addr); > void hyp_put_page(struct hyp_pool *pool, void *addr); > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > index bacd493a4eac..93a79736c283 100644 > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; > > static void *host_s2_zalloc_pages_exact(size_t size) > { > - return hyp_alloc_pages(&host_s2_pool, get_order(size)); > + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); > + > + hyp_split_page(hyp_virt_to_page(addr)); The only reason this doesn't lead to a subsequent memory leak is that concatenated page tables are always a power of two, right? If so, that deserves a comment, because I don't think this works in the general case unless you actively free the pages that are between size and (1 << order). > + > + return addr; > } > > static void *host_s2_zalloc_page(void *pool) > diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c > index 41fc25bdfb34..a6e874e61a40 100644 > --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c > +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c > @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) > hyp_spin_unlock(&pool->lock); > } > > +void hyp_split_page(struct hyp_page *p) > +{ > + unsigned short order = p->order; > + unsigned int i; > + > + p->order = 0; > + for (i = 1; i < (1 << order); i++) { > + struct hyp_page *tail = p + i; > + > + tail->order = 0; > + hyp_set_page_refcounted(tail); > + } > +} > + > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) > { > unsigned short i = order; Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount 2021-10-04 9:55 ` Marc Zyngier (?) @ 2021-10-04 10:05 ` Quentin Perret -1 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 10:05 UTC (permalink / raw) To: Marc Zyngier Cc: kernel-team, Will Deacon, Catalin Marinas, linux-kernel, linux-arm-kernel, kvmarm Hey Marc, On Monday 04 Oct 2021 at 10:55:13 (+0100), Marc Zyngier wrote: > Hi Quentin, > > On Mon, 04 Oct 2021 10:03:13 +0100, > Quentin Perret <qperret@google.com> wrote: > > > > The KVM page-table library refcounts the pages of concatenated stage-2 > > PGDs individually. However, the host's stage-2 PGD is currently managed > > by EL2 as a single high-order compound page, which can cause the > > refcount of the tail pages to reach 0 when they really shouldn't, hence > > corrupting the page-table. > > nit: this comment only applies to the protected mode, right? As far as > I can tell, 'classic' KVM is just fine. Correct, this really only applies to the host stage-2, which implies we're in protected mode. I'll make that a bit more explicit. > > Fix this by introducing a new hyp_split_page() helper in the EL2 page > > allocator (matching EL1's split_page() function), and make use of it > > uber nit: split_page() is not an EL1 function. more of a standard > kernel function. Fair enough :) > > from host_s2_zalloc_page(). > > > > Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") > > Suggested-by: Will Deacon <will@kernel.org> > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + > > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- > > arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > index fb0f523d1492..0a048dc06a7d 100644 > > --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > @@ -24,6 +24,7 @@ struct hyp_pool { > > > > /* Allocation */ > > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); > > +void hyp_split_page(struct hyp_page *page); > > void hyp_get_page(struct hyp_pool *pool, void *addr); > > void hyp_put_page(struct hyp_pool *pool, void *addr); > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > index bacd493a4eac..93a79736c283 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; > > > > static void *host_s2_zalloc_pages_exact(size_t size) > > { > > - return hyp_alloc_pages(&host_s2_pool, get_order(size)); > > + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); > > + > > + hyp_split_page(hyp_virt_to_page(addr)); > > The only reason this doesn't lead to a subsequent memory leak is that > concatenated page tables are always a power of two, right? Indeed, and also because the host stage-2 is _never_ freed, so that's not memory we're going to reclaim anyway -- we don't have an implementation of ->free_pages_exact() in the host stage-2 mm_ops. > If so, that deserves a comment, because I don't think this works in > the general case unless you actively free the pages that are between > size and (1 << order). Ack, that'll probably confuse me too in a few weeks, so a comment won't hurt. I'll re-spin shortly. Thanks, Quentin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-04 10:05 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 10:05 UTC (permalink / raw) To: Marc Zyngier Cc: James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel, kernel-team Hey Marc, On Monday 04 Oct 2021 at 10:55:13 (+0100), Marc Zyngier wrote: > Hi Quentin, > > On Mon, 04 Oct 2021 10:03:13 +0100, > Quentin Perret <qperret@google.com> wrote: > > > > The KVM page-table library refcounts the pages of concatenated stage-2 > > PGDs individually. However, the host's stage-2 PGD is currently managed > > by EL2 as a single high-order compound page, which can cause the > > refcount of the tail pages to reach 0 when they really shouldn't, hence > > corrupting the page-table. > > nit: this comment only applies to the protected mode, right? As far as > I can tell, 'classic' KVM is just fine. Correct, this really only applies to the host stage-2, which implies we're in protected mode. I'll make that a bit more explicit. > > Fix this by introducing a new hyp_split_page() helper in the EL2 page > > allocator (matching EL1's split_page() function), and make use of it > > uber nit: split_page() is not an EL1 function. more of a standard > kernel function. Fair enough :) > > from host_s2_zalloc_page(). > > > > Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") > > Suggested-by: Will Deacon <will@kernel.org> > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + > > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- > > arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > index fb0f523d1492..0a048dc06a7d 100644 > > --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > @@ -24,6 +24,7 @@ struct hyp_pool { > > > > /* Allocation */ > > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); > > +void hyp_split_page(struct hyp_page *page); > > void hyp_get_page(struct hyp_pool *pool, void *addr); > > void hyp_put_page(struct hyp_pool *pool, void *addr); > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > index bacd493a4eac..93a79736c283 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; > > > > static void *host_s2_zalloc_pages_exact(size_t size) > > { > > - return hyp_alloc_pages(&host_s2_pool, get_order(size)); > > + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); > > + > > + hyp_split_page(hyp_virt_to_page(addr)); > > The only reason this doesn't lead to a subsequent memory leak is that > concatenated page tables are always a power of two, right? Indeed, and also because the host stage-2 is _never_ freed, so that's not memory we're going to reclaim anyway -- we don't have an implementation of ->free_pages_exact() in the host stage-2 mm_ops. > If so, that deserves a comment, because I don't think this works in > the general case unless you actively free the pages that are between > size and (1 << order). Ack, that'll probably confuse me too in a few weeks, so a comment won't hurt. I'll re-spin shortly. Thanks, Quentin ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-04 10:05 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 10:05 UTC (permalink / raw) To: Marc Zyngier Cc: James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel, kernel-team Hey Marc, On Monday 04 Oct 2021 at 10:55:13 (+0100), Marc Zyngier wrote: > Hi Quentin, > > On Mon, 04 Oct 2021 10:03:13 +0100, > Quentin Perret <qperret@google.com> wrote: > > > > The KVM page-table library refcounts the pages of concatenated stage-2 > > PGDs individually. However, the host's stage-2 PGD is currently managed > > by EL2 as a single high-order compound page, which can cause the > > refcount of the tail pages to reach 0 when they really shouldn't, hence > > corrupting the page-table. > > nit: this comment only applies to the protected mode, right? As far as > I can tell, 'classic' KVM is just fine. Correct, this really only applies to the host stage-2, which implies we're in protected mode. I'll make that a bit more explicit. > > Fix this by introducing a new hyp_split_page() helper in the EL2 page > > allocator (matching EL1's split_page() function), and make use of it > > uber nit: split_page() is not an EL1 function. more of a standard > kernel function. Fair enough :) > > from host_s2_zalloc_page(). > > > > Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") > > Suggested-by: Will Deacon <will@kernel.org> > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + > > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- > > arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > index fb0f523d1492..0a048dc06a7d 100644 > > --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h > > @@ -24,6 +24,7 @@ struct hyp_pool { > > > > /* Allocation */ > > void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); > > +void hyp_split_page(struct hyp_page *page); > > void hyp_get_page(struct hyp_pool *pool, void *addr); > > void hyp_put_page(struct hyp_pool *pool, void *addr); > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > index bacd493a4eac..93a79736c283 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; > > > > static void *host_s2_zalloc_pages_exact(size_t size) > > { > > - return hyp_alloc_pages(&host_s2_pool, get_order(size)); > > + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); > > + > > + hyp_split_page(hyp_virt_to_page(addr)); > > The only reason this doesn't lead to a subsequent memory leak is that > concatenated page tables are always a power of two, right? Indeed, and also because the host stage-2 is _never_ freed, so that's not memory we're going to reclaim anyway -- we don't have an implementation of ->free_pages_exact() in the host stage-2 mm_ops. > If so, that deserves a comment, because I don't think this works in > the general case unless you actively free the pages that are between > size and (1 << order). Ack, that'll probably confuse me too in a few weeks, so a comment won't hurt. I'll re-spin shortly. Thanks, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] KVM: arm64: Report corrupted refcount at EL2 2021-10-04 9:03 ` Quentin Perret (?) @ 2021-10-04 9:03 ` Quentin Perret -1 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team Some of the refcount manipulation helpers used at EL2 are instrumented to catch a corrupt state, but not all of them are treated equally. Let's make things more consistent by instrumenting hyp_page_ref_dec_and_test() as well. Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index a6e874e61a40..0bd7701ad1df 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -152,6 +152,7 @@ static inline void hyp_page_ref_inc(struct hyp_page *p) static inline int hyp_page_ref_dec_and_test(struct hyp_page *p) { + BUG_ON(!p->refcount); p->refcount--; return (p->refcount == 0); } -- 2.33.0.800.g4c38ced690-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/2] KVM: arm64: Report corrupted refcount at EL2 @ 2021-10-04 9:03 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team Some of the refcount manipulation helpers used at EL2 are instrumented to catch a corrupt state, but not all of them are treated equally. Let's make things more consistent by instrumenting hyp_page_ref_dec_and_test() as well. Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index a6e874e61a40..0bd7701ad1df 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -152,6 +152,7 @@ static inline void hyp_page_ref_inc(struct hyp_page *p) static inline int hyp_page_ref_dec_and_test(struct hyp_page *p) { + BUG_ON(!p->refcount); p->refcount--; return (p->refcount == 0); } -- 2.33.0.800.g4c38ced690-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/2] KVM: arm64: Report corrupted refcount at EL2 @ 2021-10-04 9:03 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-04 9:03 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team Some of the refcount manipulation helpers used at EL2 are instrumented to catch a corrupt state, but not all of them are treated equally. Let's make things more consistent by instrumenting hyp_page_ref_dec_and_test() as well. Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index a6e874e61a40..0bd7701ad1df 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -152,6 +152,7 @@ static inline void hyp_page_ref_inc(struct hyp_page *p) static inline int hyp_page_ref_dec_and_test(struct hyp_page *p) { + BUG_ON(!p->refcount); p->refcount--; return (p->refcount == 0); } -- 2.33.0.800.g4c38ced690-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 0/2] A couple of EL2 refcounts fixes 2021-10-04 9:03 ` Quentin Perret (?) @ 2021-10-04 9:39 ` Will Deacon -1 siblings, 0 replies; 21+ messages in thread From: Will Deacon @ 2021-10-04 9:39 UTC (permalink / raw) To: Quentin Perret Cc: kernel-team, Marc Zyngier, linux-kernel, linux-arm-kernel, Catalin Marinas, kvmarm On Mon, Oct 04, 2021 at 10:03:12AM +0100, Quentin Perret wrote: > This addresses a couple of issues Will has found with the refcounting of > page-tables at EL2. Patch 01 fixes a nasty bug, and probably wants to go > in -stable. Patch 02 fixes a small inconsistency which made it harder to > find refcount-related bugs at EL2. > > Feedback welcome ! For both patches: Acked-by: Will Deacon <will@kernel.org> Thanks! Will _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/2] A couple of EL2 refcounts fixes @ 2021-10-04 9:39 ` Will Deacon 0 siblings, 0 replies; 21+ messages in thread From: Will Deacon @ 2021-10-04 9:39 UTC (permalink / raw) To: Quentin Perret Cc: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel, kernel-team On Mon, Oct 04, 2021 at 10:03:12AM +0100, Quentin Perret wrote: > This addresses a couple of issues Will has found with the refcounting of > page-tables at EL2. Patch 01 fixes a nasty bug, and probably wants to go > in -stable. Patch 02 fixes a small inconsistency which made it harder to > find refcount-related bugs at EL2. > > Feedback welcome ! For both patches: Acked-by: Will Deacon <will@kernel.org> Thanks! Will ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/2] A couple of EL2 refcounts fixes @ 2021-10-04 9:39 ` Will Deacon 0 siblings, 0 replies; 21+ messages in thread From: Will Deacon @ 2021-10-04 9:39 UTC (permalink / raw) To: Quentin Perret Cc: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel, kernel-team On Mon, Oct 04, 2021 at 10:03:12AM +0100, Quentin Perret wrote: > This addresses a couple of issues Will has found with the refcounting of > page-tables at EL2. Patch 01 fixes a nasty bug, and probably wants to go > in -stable. Patch 02 fixes a small inconsistency which made it harder to > find refcount-related bugs at EL2. > > Feedback welcome ! For both patches: Acked-by: Will Deacon <will@kernel.org> Thanks! Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/2] A couple of EL2 refcounts fixes @ 2021-10-05 9:01 Quentin Perret 2021-10-05 9:01 ` Quentin Perret 0 siblings, 1 reply; 21+ messages in thread From: Quentin Perret @ 2021-10-05 9:01 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team Hi all, This addresses a couple of issues Will has found with the refcounting of page-tables at EL2. Patch 01 fixes a nasty bug, and probably wants to go in -stable. Patch 02 fixes a small inconsistency which made it harder to find refcount-related bugs at EL2. Feedback welcome ! Thanks, Quentin Quentin Perret (2): KVM: arm64: Fix host stage-2 PGD refcount KVM: arm64: Report corrupted refcount at EL2 arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) -- 2.33.0.800.g4c38ced690-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount 2021-10-05 9:01 Quentin Perret 2021-10-05 9:01 ` Quentin Perret @ 2021-10-05 9:01 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-05 9:01 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team The KVM page-table library refcounts the pages of concatenated stage-2 PGDs individually. However, the host's stage-2 PGD is currently managed by EL2 as a single high-order compound page, which can cause the refcount of the tail pages to reach 0 when they really shouldn't, hence corrupting the page-table. Fix this by introducing a new hyp_split_page() helper in the EL2 page allocator (matching EL1's split_page() function), and make use of it from host_s2_zalloc_page(). Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h index fb0f523d1492..0a048dc06a7d 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h @@ -24,6 +24,7 @@ struct hyp_pool { /* Allocation */ void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); +void hyp_split_page(struct hyp_page *page); void hyp_get_page(struct hyp_pool *pool, void *addr); void hyp_put_page(struct hyp_pool *pool, void *addr); diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index bacd493a4eac..93a79736c283 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; static void *host_s2_zalloc_pages_exact(size_t size) { - return hyp_alloc_pages(&host_s2_pool, get_order(size)); + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); + + hyp_split_page(hyp_virt_to_page(addr)); + + return addr; } static void *host_s2_zalloc_page(void *pool) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index 41fc25bdfb34..a6e874e61a40 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) hyp_spin_unlock(&pool->lock); } +void hyp_split_page(struct hyp_page *p) +{ + unsigned short order = p->order; + unsigned int i; + + p->order = 0; + for (i = 1; i < (1 << order); i++) { + struct hyp_page *tail = p + i; + + tail->order = 0; + hyp_set_page_refcounted(tail); + } +} + void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) { unsigned short i = order; -- 2.33.0.800.g4c38ced690-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-05 9:01 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-05 9:01 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team The KVM page-table library refcounts the pages of concatenated stage-2 PGDs individually. However, the host's stage-2 PGD is currently managed by EL2 as a single high-order compound page, which can cause the refcount of the tail pages to reach 0 when they really shouldn't, hence corrupting the page-table. Fix this by introducing a new hyp_split_page() helper in the EL2 page allocator (matching EL1's split_page() function), and make use of it from host_s2_zalloc_page(). Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h index fb0f523d1492..0a048dc06a7d 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h @@ -24,6 +24,7 @@ struct hyp_pool { /* Allocation */ void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); +void hyp_split_page(struct hyp_page *page); void hyp_get_page(struct hyp_pool *pool, void *addr); void hyp_put_page(struct hyp_pool *pool, void *addr); diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index bacd493a4eac..93a79736c283 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; static void *host_s2_zalloc_pages_exact(size_t size) { - return hyp_alloc_pages(&host_s2_pool, get_order(size)); + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); + + hyp_split_page(hyp_virt_to_page(addr)); + + return addr; } static void *host_s2_zalloc_page(void *pool) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index 41fc25bdfb34..a6e874e61a40 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) hyp_spin_unlock(&pool->lock); } +void hyp_split_page(struct hyp_page *p) +{ + unsigned short order = p->order; + unsigned int i; + + p->order = 0; + for (i = 1; i < (1 << order); i++) { + struct hyp_page *tail = p + i; + + tail->order = 0; + hyp_set_page_refcounted(tail); + } +} + void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) { unsigned short i = order; -- 2.33.0.800.g4c38ced690-goog ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount @ 2021-10-05 9:01 ` Quentin Perret 0 siblings, 0 replies; 21+ messages in thread From: Quentin Perret @ 2021-10-05 9:01 UTC (permalink / raw) To: Marc Zyngier, James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Quentin Perret, Fuad Tabba, David Brazdil, linux-arm-kernel, kvmarm, linux-kernel Cc: kernel-team The KVM page-table library refcounts the pages of concatenated stage-2 PGDs individually. However, the host's stage-2 PGD is currently managed by EL2 as a single high-order compound page, which can cause the refcount of the tail pages to reach 0 when they really shouldn't, hence corrupting the page-table. Fix this by introducing a new hyp_split_page() helper in the EL2 page allocator (matching EL1's split_page() function), and make use of it from host_s2_zalloc_page(). Fixes: 1025c8c0c6ac ("KVM: arm64: Wrap the host with a stage 2") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h index fb0f523d1492..0a048dc06a7d 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h @@ -24,6 +24,7 @@ struct hyp_pool { /* Allocation */ void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order); +void hyp_split_page(struct hyp_page *page); void hyp_get_page(struct hyp_pool *pool, void *addr); void hyp_put_page(struct hyp_pool *pool, void *addr); diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index bacd493a4eac..93a79736c283 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -35,7 +35,11 @@ const u8 pkvm_hyp_id = 1; static void *host_s2_zalloc_pages_exact(size_t size) { - return hyp_alloc_pages(&host_s2_pool, get_order(size)); + void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size)); + + hyp_split_page(hyp_virt_to_page(addr)); + + return addr; } static void *host_s2_zalloc_page(void *pool) diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c index 41fc25bdfb34..a6e874e61a40 100644 --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c @@ -193,6 +193,20 @@ void hyp_get_page(struct hyp_pool *pool, void *addr) hyp_spin_unlock(&pool->lock); } +void hyp_split_page(struct hyp_page *p) +{ + unsigned short order = p->order; + unsigned int i; + + p->order = 0; + for (i = 1; i < (1 << order); i++) { + struct hyp_page *tail = p + i; + + tail->order = 0; + hyp_set_page_refcounted(tail); + } +} + void *hyp_alloc_pages(struct hyp_pool *pool, unsigned short order) { unsigned short i = order; -- 2.33.0.800.g4c38ced690-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-10-05 9:04 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-04 9:03 [PATCH 0/2] A couple of EL2 refcounts fixes Quentin Perret 2021-10-04 9:03 ` Quentin Perret 2021-10-04 9:03 ` Quentin Perret 2021-10-04 9:03 ` [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount Quentin Perret 2021-10-04 9:03 ` Quentin Perret 2021-10-04 9:03 ` Quentin Perret 2021-10-04 9:55 ` Marc Zyngier 2021-10-04 9:55 ` Marc Zyngier 2021-10-04 9:55 ` Marc Zyngier 2021-10-04 10:05 ` Quentin Perret 2021-10-04 10:05 ` Quentin Perret 2021-10-04 10:05 ` Quentin Perret 2021-10-04 9:03 ` [PATCH 2/2] KVM: arm64: Report corrupted refcount at EL2 Quentin Perret 2021-10-04 9:03 ` Quentin Perret 2021-10-04 9:03 ` Quentin Perret 2021-10-04 9:39 ` [PATCH 0/2] A couple of EL2 refcounts fixes Will Deacon 2021-10-04 9:39 ` Will Deacon 2021-10-04 9:39 ` Will Deacon -- strict thread matches above, loose matches on Subject: below -- 2021-10-05 9:01 Quentin Perret 2021-10-05 9:01 ` [PATCH 1/2] KVM: arm64: Fix host stage-2 PGD refcount Quentin Perret 2021-10-05 9:01 ` Quentin Perret 2021-10-05 9:01 ` Quentin Perret
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.