Hi Quentin and Marc
I noticed Marc had sent out new version on behalf of me, thanks for the help.
I hated the time difference, sorry for the late.

Just answer the comments below to make it clear.

-----Original Message-----
From: Quentin Perret <redacted>
Sent: Wednesday, March 3, 2021 7:09 PM
To: Marc Zyngier <maz@kernel.org>
Cc: Justin He <redacted>; kvmarm@lists.cs.columbia.edu; James
Morse [off-list ref]; Julien Thierry [off-list ref];
Suzuki Poulose [off-list ref]; Catalin Marinas
[off-list ref]; Will Deacon [off-list ref]; Gavin Shan
[off-list ref]; Yanan Wang [off-list ref]; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: arm64: Fix unaligned addr case in mmu walking

On Wednesday 03 Mar 2021 at 09:54:25 (+0000), Marc Zyngier wrote:

quoted

Hi Jia,

On Wed, 03 Mar 2021 02:42:25 +0000,
Jia He [off-list ref] wrote:

quoted

If the start addr is not aligned with the granule size of that level.
loop step size should be adjusted to boundary instead of simple
kvm_granual_size(level) increment. Otherwise, some mmu entries might

miss

quoted

quoted

the chance to be walked through.
E.g. Assume the unmap range [data->addr, data->end] is
[0xff00ab2000,0xff00cb2000] in level 2 walking and NOT block mapping.

When does this occur? Upgrade from page mappings to block? Swap out?

quoted

And the 1st part of that pmd entry is [0xff00ab2000,0xff00c00000]. The
pmd value is 0x83fbd2c1002 (not valid entry). In this case, data->addr
should be adjusted to 0xff00c00000 instead of 0xff00cb2000.

Let me see if I understand this. Assuming 4k pages, the region
described above spans *two* 2M entries:

(a) ff00ab2000-ff00c00000, part of ff00a00000-ff00c00000
(b) ff00c00000-ff00db2000, part of ff00c00000-ff00e00000

(a) has no valid mapping, but (b) does. Because we fail to correctly
align on a block boundary when skipping (a), we also skip (b), which
is then left mapped.

Did I get it right? If so, yes, this is... annoying.

Yes, exactly the case

quoted

Understanding the circumstances this triggers in would be most
interesting. This current code seems to assume that we get ranges
aligned to mapping boundaries, but I seem to remember that the old
code did use the stage2_*_addr_end() helpers to deal with this case.

Will: I don't think things have changed in that respect, right?

Indeed we should still use stage2_*_addr_end(), especially in the unmap
path that is mentioned here, so it would be helpful to have a little bit
more context.

Yes, stage2_pgd_addr_end() was still there but the stage2_pmd_addr_end() was removed.

quoted

quoted

Without this fix, userspace "segment fault" error can be easily
triggered by running simple gVisor runsc cases on an Ampere Altra
server:
    docker run --runtime=runsc -it --rm  ubuntu /bin/bash

In container:
    for i in `seq 1 100`;do ls;done

The workload on its own isn't that interesting. What I'd like to
understand is what happens on the host during that time.

Okay

quoted

quoted

Reported-by: Howard Zhang <redacted>
Signed-off-by: Jia He <redacted>
---
 arch/arm64/kvm/hyp/pgtable.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kvm/hyp/pgtable.c

b/arch/arm64/kvm/hyp/pgtable.c

quoted

quoted

index bdf8e55ed308..4d99d07c610c 100644

--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c

@@ -225,6 +225,7 @@ static inline int __kvm_pgtable_visit(struct

kvm_pgtable_walk_data *data,

quoted

quoted

 		goto out;

 	if (!table) {
+		data->addr = ALIGN_DOWN(data->addr, kvm_granule_size(level));
 		data->addr += kvm_granule_size(level);
 		goto out;
 	}

It otherwise looks good to me. Quentin, Will: unless you object to
this, I plan to take it in the next round of fixes with

Though I'm still unsure how we hit that today, the change makes sense on
its own I think, so no objection from me.

Thanks,
Quentin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel