* [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2
@ 2023-04-21 7:16 Oliver Upton
2023-04-21 7:16 ` [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker Oliver Upton
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Oliver Upton @ 2023-04-21 7:16 UTC (permalink / raw)
To: kvmarm
Cc: Marc Zyngier, James Morse, Suzuki K Poulose, Zenghui Yu,
David Matlack, Reiji Watanabe, Oliver Upton
Ugh.
So it appears that there is a race between two parallel stage-2 map
walkers that could lead to mapping the incorrect PA for a given IPA, as
the IPA -> PA relationship picks up an unintended offset. This series
eliminates the problem by using the current IPA of the walk as the
source-of-truth regarding where we are in a map operation. If you're
curious about the race, it is spelled out in the first patch.
While there is no such race to update hyp's stage-1, the second patch
applies the same rationale to hyp stage-1 walks for the sake of
consistency.
Applies to 6.3-rc3, and merges w/o conflict into kvmarm/next. Took this
for a ride with selftests, kvm-unit-tests, QEMU, and our internal VMM
(affectionately referred to as Vanadium on the list from time to time).
I also ran through the gamut of nVHE, VHE, and pKVM given the effects on
hyp stage-1.
Marc, the bug can have some rather ugly (albeit rare) consequences, so
I'd like to get this in ASAP. The door is shut on 6.3, but it'd be nice
to squeeze in the 6.4 pull request if possible.
Oliver Upton (2):
KVM: arm64: Infer the PA offset from IPA in stage-2 map walker
KVM: arm64: Infer PA offset from VA in hyp map walker
arch/arm64/include/asm/kvm_pgtable.h | 1 +
arch/arm64/kvm/hyp/pgtable.c | 35 +++++++++++++++++++++++-----
2 files changed, 30 insertions(+), 6 deletions(-)
base-commit: e8d018dd0257f744ca50a729e3d042cf2ec9da65
--
2.40.0.634.g4ca3ef3211-goog
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker
2023-04-21 7:16 [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Oliver Upton
@ 2023-04-21 7:16 ` Oliver Upton
2023-04-21 9:28 ` Marc Zyngier
2023-04-21 7:16 ` [PATCH 2/2] KVM: arm64: Infer PA offset from VA in hyp " Oliver Upton
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Oliver Upton @ 2023-04-21 7:16 UTC (permalink / raw)
To: kvmarm
Cc: Marc Zyngier, James Morse, Suzuki K Poulose, Zenghui Yu,
David Matlack, Reiji Watanabe, Oliver Upton, stable
Until now, the page table walker counted increments to the PA and IPA
of a walk in two separate places. While the PA is incremented as soon as
a leaf PTE is installed in stage2_map_walker_try_leaf(), the IPA is
actually bumped in the generic table walker context. Critically,
__kvm_pgtable_visit() rereads the PTE after the LEAF callback returns
to work out if a table or leaf was installed, and only bumps the IPA for
a leaf PTE.
This arrangement worked fine when we handled faults behind the write lock,
as the walker had exclusive access to the stage-2 page tables. However,
commit 1577cb5823ce ("KVM: arm64: Handle stage-2 faults in parallel")
started handling all stage-2 faults behind the read lock, opening up a
race where a walker could increment the PA but not the IPA of a walk.
Nothing good ensues, as the walker starts mapping with the incorrect
IPA -> PA relationship.
For example, assume that two vCPUs took a data abort on the same IPA.
One observes that dirty logging is disabled, and the other observed that
it is enabled:
vCPU attempting PMD mapping vCPU attempting PTE mapping
====================================== =====================================
/* install PMD */
stage2_make_pte(ctx, leaf);
data->phys += granule;
/* replace PMD with a table */
stage2_try_break_pte(ctx, data->mmu);
stage2_make_pte(ctx, table);
/* table is observed */
ctx.old = READ_ONCE(*ptep);
table = kvm_pte_table(ctx.old, level);
/*
* map walk continues w/o incrementing
* IPA.
*/
__kvm_pgtable_walk(..., level + 1);
Bring an end to the whole mess by using the IPA as the single source of
truth for how far along a walk has gotten. Work out the correct PA to
map by calculating the IPA offset from the beginning of the walk and add
that to the starting physical address.
Cc: stable@vger.kernel.org
Fixes: 1577cb5823ce ("KVM: arm64: Handle stage-2 faults in parallel")
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
arch/arm64/include/asm/kvm_pgtable.h | 1 +
arch/arm64/kvm/hyp/pgtable.c | 32 ++++++++++++++++++++++++----
2 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 4cd6762bda80..dc3c072e862f 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -209,6 +209,7 @@ struct kvm_pgtable_visit_ctx {
kvm_pte_t old;
void *arg;
struct kvm_pgtable_mm_ops *mm_ops;
+ u64 start;
u64 addr;
u64 end;
u32 level;
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 3d61bd3e591d..140f82300db5 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -58,6 +58,7 @@
struct kvm_pgtable_walk_data {
struct kvm_pgtable_walker *walker;
+ u64 start;
u64 addr;
u64 end;
};
@@ -201,6 +202,7 @@ static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data,
.old = READ_ONCE(*ptep),
.arg = data->walker->arg,
.mm_ops = mm_ops,
+ .start = data->start,
.addr = data->addr,
.end = data->end,
.level = level,
@@ -293,6 +295,7 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size,
struct kvm_pgtable_walker *walker)
{
struct kvm_pgtable_walk_data walk_data = {
+ .start = ALIGN_DOWN(addr, PAGE_SIZE),
.addr = ALIGN_DOWN(addr, PAGE_SIZE),
.end = PAGE_ALIGN(walk_data.addr + size),
.walker = walker,
@@ -794,20 +797,43 @@ static bool stage2_pte_executable(kvm_pte_t pte)
return !(pte & KVM_PTE_LEAF_ATTR_HI_S2_XN);
}
+static u64 stage2_map_walker_phys_addr(const struct kvm_pgtable_visit_ctx *ctx,
+ const struct stage2_map_data *data)
+{
+ u64 phys = data->phys;
+
+ /*
+ * Stage-2 walks to update ownership data are communicated to the map
+ * walker using an invalid PA. Avoid offsetting an already invalid PA,
+ * which could overflow and make the address valid again.
+ */
+ if (!kvm_phys_is_valid(phys))
+ return phys;
+
+ /*
+ * Otherwise, work out the correct PA based on how far the walk has
+ * gotten.
+ */
+ return phys + (ctx->addr - ctx->start);
+}
+
static bool stage2_leaf_mapping_allowed(const struct kvm_pgtable_visit_ctx *ctx,
struct stage2_map_data *data)
{
+ u64 phys = stage2_map_walker_phys_addr(ctx, data);
+
if (data->force_pte && (ctx->level < (KVM_PGTABLE_MAX_LEVELS - 1)))
return false;
- return kvm_block_mapping_supported(ctx, data->phys);
+ return kvm_block_mapping_supported(ctx, phys);
}
static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx,
struct stage2_map_data *data)
{
kvm_pte_t new;
- u64 granule = kvm_granule_size(ctx->level), phys = data->phys;
+ u64 phys = stage2_map_walker_phys_addr(ctx, data);
+ u64 granule = kvm_granule_size(ctx->level);
struct kvm_pgtable *pgt = data->mmu->pgt;
struct kvm_pgtable_mm_ops *mm_ops = ctx->mm_ops;
@@ -841,8 +867,6 @@ static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx,
stage2_make_pte(ctx, new);
- if (kvm_phys_is_valid(phys))
- data->phys += granule;
return 0;
}
--
2.40.0.634.g4ca3ef3211-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] KVM: arm64: Infer PA offset from VA in hyp map walker
2023-04-21 7:16 [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Oliver Upton
2023-04-21 7:16 ` [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker Oliver Upton
@ 2023-04-21 7:16 ` Oliver Upton
2023-04-21 9:12 ` [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Marc Zyngier
2023-04-21 12:53 ` Marc Zyngier
3 siblings, 0 replies; 8+ messages in thread
From: Oliver Upton @ 2023-04-21 7:16 UTC (permalink / raw)
To: kvmarm
Cc: Marc Zyngier, James Morse, Suzuki K Poulose, Zenghui Yu,
David Matlack, Reiji Watanabe, Oliver Upton
Similar to the recently fixed stage-2 walker, the hyp map walker
increments the PA and VA of a walk separately. Unlike stage-2, there is
no bug here as the map walker has exclusive access to the stage-1 page
tables.
Nonetheless, in the interest of continuity throughout the page table
code, tweak the hyp map walker to avoid incrementing the PA and instead
use the VA as the authoritative source of how far along a table walk has
gotten. Calculate the PA to use for a leaf PTE by adding the offset of
the VA from the start of the walk to the starting PA.
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
arch/arm64/kvm/hyp/pgtable.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 140f82300db5..356a3fd5220c 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -410,13 +410,12 @@ enum kvm_pgtable_prot kvm_pgtable_hyp_pte_prot(kvm_pte_t pte)
static bool hyp_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx,
struct hyp_map_data *data)
{
+ u64 phys = data->phys + (ctx->addr - ctx->start);
kvm_pte_t new;
- u64 granule = kvm_granule_size(ctx->level), phys = data->phys;
if (!kvm_block_mapping_supported(ctx, phys))
return false;
- data->phys += granule;
new = kvm_init_valid_leaf_pte(phys, data->attr, ctx->level);
if (ctx->old == new)
return true;
--
2.40.0.634.g4ca3ef3211-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2
2023-04-21 7:16 [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Oliver Upton
2023-04-21 7:16 ` [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker Oliver Upton
2023-04-21 7:16 ` [PATCH 2/2] KVM: arm64: Infer PA offset from VA in hyp " Oliver Upton
@ 2023-04-21 9:12 ` Marc Zyngier
2023-04-21 9:29 ` Oliver Upton
2023-04-21 12:53 ` Marc Zyngier
3 siblings, 1 reply; 8+ messages in thread
From: Marc Zyngier @ 2023-04-21 9:12 UTC (permalink / raw)
To: Oliver Upton
Cc: kvmarm, James Morse, Suzuki K Poulose, Zenghui Yu, David Matlack,
Reiji Watanabe
On Fri, 21 Apr 2023 08:16:04 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Ugh.
>
> So it appears that there is a race between two parallel stage-2 map
> walkers that could lead to mapping the incorrect PA for a given IPA, as
> the IPA -> PA relationship picks up an unintended offset. This series
> eliminates the problem by using the current IPA of the walk as the
> source-of-truth regarding where we are in a map operation. If you're
> curious about the race, it is spelled out in the first patch.
Ugh indeed.
>
> While there is no such race to update hyp's stage-1, the second patch
> applies the same rationale to hyp stage-1 walks for the sake of
> consistency.
>
> Applies to 6.3-rc3, and merges w/o conflict into kvmarm/next. Took this
> for a ride with selftests, kvm-unit-tests, QEMU, and our internal VMM
> (affectionately referred to as Vanadium on the list from time to time).
> I also ran through the gamut of nVHE, VHE, and pKVM given the effects on
> hyp stage-1.
>
> Marc, the bug can have some rather ugly (albeit rare) consequences, so
> I'd like to get this in ASAP. The door is shut on 6.3, but it'd be nice
> to squeeze in the 6.4 pull request if possible.
This is a pretty invasive change, and I'd really like to give it some
-next exposure. I'm not doubting your testing, but experience shows
that there is always someone with a more tricky setup...
What I'd suggest is to not include it in the pull request that I'm
about to send today, but to let it simmer in -next for a week. This
will give us some confidence that we're OK, and also avoid being
shouted at for sending stuff that hasn't been in -next at all.
If everything checks out after a week, I'll send another PR with this
fix (and whatever will have landed in the meantime).
Would that work for you?
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker
2023-04-21 7:16 ` [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker Oliver Upton
@ 2023-04-21 9:28 ` Marc Zyngier
2023-04-21 9:35 ` Oliver Upton
0 siblings, 1 reply; 8+ messages in thread
From: Marc Zyngier @ 2023-04-21 9:28 UTC (permalink / raw)
To: Oliver Upton
Cc: kvmarm, James Morse, Suzuki K Poulose, Zenghui Yu, David Matlack,
Reiji Watanabe, stable
On Fri, 21 Apr 2023 08:16:05 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Until now, the page table walker counted increments to the PA and IPA
> of a walk in two separate places. While the PA is incremented as soon as
> a leaf PTE is installed in stage2_map_walker_try_leaf(), the IPA is
> actually bumped in the generic table walker context. Critically,
> __kvm_pgtable_visit() rereads the PTE after the LEAF callback returns
> to work out if a table or leaf was installed, and only bumps the IPA for
> a leaf PTE.
>
> This arrangement worked fine when we handled faults behind the write lock,
> as the walker had exclusive access to the stage-2 page tables. However,
> commit 1577cb5823ce ("KVM: arm64: Handle stage-2 faults in parallel")
> started handling all stage-2 faults behind the read lock, opening up a
> race where a walker could increment the PA but not the IPA of a walk.
> Nothing good ensues, as the walker starts mapping with the incorrect
> IPA -> PA relationship.
>
> For example, assume that two vCPUs took a data abort on the same IPA.
> One observes that dirty logging is disabled, and the other observed that
> it is enabled:
>
> vCPU attempting PMD mapping vCPU attempting PTE mapping
> ====================================== =====================================
> /* install PMD */
> stage2_make_pte(ctx, leaf);
> data->phys += granule;
> /* replace PMD with a table */
> stage2_try_break_pte(ctx, data->mmu);
> stage2_make_pte(ctx, table);
> /* table is observed */
> ctx.old = READ_ONCE(*ptep);
> table = kvm_pte_table(ctx.old, level);
>
> /*
> * map walk continues w/o incrementing
> * IPA.
> */
> __kvm_pgtable_walk(..., level + 1);
>
> Bring an end to the whole mess by using the IPA as the single source of
> truth for how far along a walk has gotten. Work out the correct PA to
> map by calculating the IPA offset from the beginning of the walk and add
> that to the starting physical address.
>
> Cc: stable@vger.kernel.org
> Fixes: 1577cb5823ce ("KVM: arm64: Handle stage-2 faults in parallel")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
> arch/arm64/include/asm/kvm_pgtable.h | 1 +
> arch/arm64/kvm/hyp/pgtable.c | 32 ++++++++++++++++++++++++----
> 2 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 4cd6762bda80..dc3c072e862f 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -209,6 +209,7 @@ struct kvm_pgtable_visit_ctx {
> kvm_pte_t old;
> void *arg;
> struct kvm_pgtable_mm_ops *mm_ops;
> + u64 start;
> u64 addr;
> u64 end;
> u32 level;
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 3d61bd3e591d..140f82300db5 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -58,6 +58,7 @@
> struct kvm_pgtable_walk_data {
> struct kvm_pgtable_walker *walker;
>
> + u64 start;
> u64 addr;
> u64 end;
> };
> @@ -201,6 +202,7 @@ static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data,
> .old = READ_ONCE(*ptep),
> .arg = data->walker->arg,
> .mm_ops = mm_ops,
> + .start = data->start,
> .addr = data->addr,
> .end = data->end,
> .level = level,
> @@ -293,6 +295,7 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size,
> struct kvm_pgtable_walker *walker)
> {
> struct kvm_pgtable_walk_data walk_data = {
> + .start = ALIGN_DOWN(addr, PAGE_SIZE),
> .addr = ALIGN_DOWN(addr, PAGE_SIZE),
> .end = PAGE_ALIGN(walk_data.addr + size),
> .walker = walker,
> @@ -794,20 +797,43 @@ static bool stage2_pte_executable(kvm_pte_t pte)
> return !(pte & KVM_PTE_LEAF_ATTR_HI_S2_XN);
> }
>
> +static u64 stage2_map_walker_phys_addr(const struct kvm_pgtable_visit_ctx *ctx,
> + const struct stage2_map_data *data)
> +{
> + u64 phys = data->phys;
> +
> + /*
> + * Stage-2 walks to update ownership data are communicated to the map
> + * walker using an invalid PA. Avoid offsetting an already invalid PA,
> + * which could overflow and make the address valid again.
> + */
> + if (!kvm_phys_is_valid(phys))
> + return phys;
> +
> + /*
> + * Otherwise, work out the correct PA based on how far the walk has
> + * gotten.
> + */
> + return phys + (ctx->addr - ctx->start);
> +}
> +
> static bool stage2_leaf_mapping_allowed(const struct kvm_pgtable_visit_ctx *ctx,
> struct stage2_map_data *data)
> {
> + u64 phys = stage2_map_walker_phys_addr(ctx, data);
> +
> if (data->force_pte && (ctx->level < (KVM_PGTABLE_MAX_LEVELS - 1)))
> return false;
>
> - return kvm_block_mapping_supported(ctx, data->phys);
> + return kvm_block_mapping_supported(ctx, phys);
> }
>
> static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx,
> struct stage2_map_data *data)
> {
> kvm_pte_t new;
> - u64 granule = kvm_granule_size(ctx->level), phys = data->phys;
> + u64 phys = stage2_map_walker_phys_addr(ctx, data);
> + u64 granule = kvm_granule_size(ctx->level);
> struct kvm_pgtable *pgt = data->mmu->pgt;
> struct kvm_pgtable_mm_ops *mm_ops = ctx->mm_ops;
>
> @@ -841,8 +867,6 @@ static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx,
>
> stage2_make_pte(ctx, new);
>
> - if (kvm_phys_is_valid(phys))
> - data->phys += granule;
> return 0;
> }
>
So my conclusion is that after these two patches, data->phys should
never be updated, right? Then I'd suggest an additional patch to
constify a couple of things and make sure we don't accidentally update
them. Something like the patch below (compile-tested only).
Thanks,
M.
From a2eb08ce793c1cf01c79df13a619815e9d7c1d41 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Fri, 21 Apr 2023 10:18:34 +0100
Subject: [PATCH] KVM: arm64: Constify start/end/phys fields of the pgtable
walker data
As we are revamping the way the pgtable walker evaluates some of the
data, make it clear that we rely on somew of the fields to be constant
across the lifetime of a walk.
For this, flag the start, end and pjys fields of the walk data as
'const', which will generate an error if we were to accidentally
update these fields again.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/hyp/pgtable.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 356a3fd5220c..5282cb9ca4cf 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -58,9 +58,9 @@
struct kvm_pgtable_walk_data {
struct kvm_pgtable_walker *walker;
- u64 start;
+ const u64 start;
u64 addr;
- u64 end;
+ const u64 end;
};
static bool kvm_phys_is_valid(u64 phys)
@@ -352,7 +352,7 @@ int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr,
}
struct hyp_map_data {
- u64 phys;
+ const u64 phys;
kvm_pte_t attr;
};
@@ -578,7 +578,7 @@ void kvm_pgtable_hyp_destroy(struct kvm_pgtable *pgt)
}
struct stage2_map_data {
- u64 phys;
+ const u64 phys;
kvm_pte_t attr;
u8 owner_id;
--
2.34.1
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2
2023-04-21 9:12 ` [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Marc Zyngier
@ 2023-04-21 9:29 ` Oliver Upton
0 siblings, 0 replies; 8+ messages in thread
From: Oliver Upton @ 2023-04-21 9:29 UTC (permalink / raw)
To: Marc Zyngier
Cc: kvmarm, James Morse, Suzuki K Poulose, Zenghui Yu, David Matlack,
Reiji Watanabe
Hey Marc,
On Fri, Apr 21, 2023 at 10:12:48AM +0100, Marc Zyngier wrote:
[...]
> > Marc, the bug can have some rather ugly (albeit rare) consequences, so
> > I'd like to get this in ASAP. The door is shut on 6.3, but it'd be nice
> > to squeeze in the 6.4 pull request if possible.
>
> This is a pretty invasive change, and I'd really like to give it some
> -next exposure. I'm not doubting your testing, but experience shows
> that there is always someone with a more tricky setup...
>
> What I'd suggest is to not include it in the pull request that I'm
> about to send today, but to let it simmer in -next for a week. This
> will give us some confidence that we're OK, and also avoid being
> shouted at for sending stuff that hasn't been in -next at all.
>
> If everything checks out after a week, I'll send another PR with this
> fix (and whatever will have landed in the meantime).
>
> Would that work for you?
Absolutely. I had the delusional thought that there was still time to
soak in -next and sneak in for 6.4. But, would you look at that, it's
already Friday :)
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker
2023-04-21 9:28 ` Marc Zyngier
@ 2023-04-21 9:35 ` Oliver Upton
0 siblings, 0 replies; 8+ messages in thread
From: Oliver Upton @ 2023-04-21 9:35 UTC (permalink / raw)
To: Marc Zyngier
Cc: kvmarm, James Morse, Suzuki K Poulose, Zenghui Yu, David Matlack,
Reiji Watanabe, stable
On Fri, Apr 21, 2023 at 10:28:58AM +0100, Marc Zyngier wrote:
[...]
> So my conclusion is that after these two patches, data->phys should
> never be updated, right?
Yep, that's the intention.
> Then I'd suggest an additional patch to constify a couple of things
> and make sure we don't accidentally update them. Something like the
> patch below (compile-tested only).
I'm always a fan of more idiot proofing, especially when I am said idiot
:)
> From a2eb08ce793c1cf01c79df13a619815e9d7c1d41 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Fri, 21 Apr 2023 10:18:34 +0100
> Subject: [PATCH] KVM: arm64: Constify start/end/phys fields of the pgtable
> walker data
>
> As we are revamping the way the pgtable walker evaluates some of the
> data, make it clear that we rely on somew of the fields to be constant
> across the lifetime of a walk.
>
> For this, flag the start, end and pjys fields of the walk data as
typo: phys
> 'const', which will generate an error if we were to accidentally
> update these fields again.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Looks good.
Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
Let's see if I've confused b4 into thinking I'm reviewing my own patches
:-P
> ---
> arch/arm64/kvm/hyp/pgtable.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 356a3fd5220c..5282cb9ca4cf 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -58,9 +58,9 @@
> struct kvm_pgtable_walk_data {
> struct kvm_pgtable_walker *walker;
>
> - u64 start;
> + const u64 start;
> u64 addr;
> - u64 end;
> + const u64 end;
> };
>
> static bool kvm_phys_is_valid(u64 phys)
> @@ -352,7 +352,7 @@ int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr,
> }
>
> struct hyp_map_data {
> - u64 phys;
> + const u64 phys;
> kvm_pte_t attr;
> };
>
> @@ -578,7 +578,7 @@ void kvm_pgtable_hyp_destroy(struct kvm_pgtable *pgt)
> }
>
> struct stage2_map_data {
> - u64 phys;
> + const u64 phys;
> kvm_pte_t attr;
> u8 owner_id;
>
> --
> 2.34.1
>
> --
> Without deviation from the norm, progress is not possible.
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2
2023-04-21 7:16 [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Oliver Upton
` (2 preceding siblings ...)
2023-04-21 9:12 ` [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Marc Zyngier
@ 2023-04-21 12:53 ` Marc Zyngier
3 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2023-04-21 12:53 UTC (permalink / raw)
To: Oliver Upton, kvmarm
Cc: James Morse, Suzuki K Poulose, David Matlack, Zenghui Yu,
Reiji Watanabe
On Fri, 21 Apr 2023 07:16:04 +0000, Oliver Upton wrote:
> Ugh.
>
> So it appears that there is a race between two parallel stage-2 map
> walkers that could lead to mapping the incorrect PA for a given IPA, as
> the IPA -> PA relationship picks up an unintended offset. This series
> eliminates the problem by using the current IPA of the walk as the
> source-of-truth regarding where we are in a map operation. If you're
> curious about the race, it is spelled out in the first patch.
>
> [...]
Applied to next, thanks!
[1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker
commit: 1f0f4a2ef7a5693b135ce174e71f116db4bd684d
[2/2] KVM: arm64: Infer PA offset from VA in hyp map walker
commit: 39bc95be3782ba88e55cd72e830f37e74395831b
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-04-21 12:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-21 7:16 [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Oliver Upton
2023-04-21 7:16 ` [PATCH 1/2] KVM: arm64: Infer the PA offset from IPA in stage-2 map walker Oliver Upton
2023-04-21 9:28 ` Marc Zyngier
2023-04-21 9:35 ` Oliver Upton
2023-04-21 7:16 ` [PATCH 2/2] KVM: arm64: Infer PA offset from VA in hyp " Oliver Upton
2023-04-21 9:12 ` [PATCH 0/2] KVM: arm64: Fix for mapping incorrect PA at stage-2 Marc Zyngier
2023-04-21 9:29 ` Oliver Upton
2023-04-21 12:53 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox