kvm: share upper halves among all pagtables

[laijs]

Fixes: #509

• request ACK from arm64 people
• Test on arm64 （via continuous-integration/travis-ci/pr）
• Test on amd64

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[laijs]

Hello! Would any arm man have a review on it please? @lubinszARM

@amscanne The patch will be helpful for #2256

[lubinszARM]

Hi @amscanne
What is your opinion about this clone method?
If you agree this method, I will continue to check this patch on Arm64.

Hi @laijs ,
Regarding to KPTI, we have the different methods on Arm64, such as:
1, TCR_EL1.AS = 1
2, ARM64_SW_TTBR0_PAN
3, Allocate ASIDs in pairs
..
By the way, there is a segv error in InitUpperHalf() on Arm64.

Thanks.

[laijs]

@lubinszARM KPTI is unrelated to this pr, but it is very helpful for #2256 and an important preparation for #2256.
This pr has its own merit - save a lot of pagetables used by the upper halves.

And #2256 doesn't implement KPTI for arm. I'm not capable to develop on arm by now.
I hope you add KPTI for arm after this pr and #2256 merged, if arm also needs it.

[laijs]

By the way, there is a segv error in InitUpperHalf() on Arm64.

updated

[lubinszARM]

By the way, there is a segv error in InitUpperHalf() on Arm64.

updated

If @amscanne approved this PR, I will help you to fix this issue.
Don't worry about it.

For your reference:
--- FAIL: TestKernelSyscall (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1d4e70]

goroutine 17 [running]:
testing.tRunner.func1.1(0x2520c0, 0x4b5ef0)
GOROOT/src/testing/testing.go:940 +0x230
testing.tRunner.func1(0x400013ea20)
GOROOT/src/testing/testing.go:943 +0x34c
panic(0x2520c0, 0x4b5ef0)
GOROOT/src/runtime/panic.go:969 +0x11c
gvisor.dev/gvisor/pkg/sentry/platform/ring0/pagetables.(*PageTables).InitUpperHalf(0x4000110ab0)

[lubinszARM]

@lubinszARM KPTI is unrelated to this pr, but it is very helpful for #2256 and an important preparation for #2256.
This pr has its own merit - save a lot of pagetables used by the upper halves.

And #2256 doesn't implement KPTI for arm. I'm not capable to develop on arm by now.
I hope you add KPTI for arm after this pr and #2256 merged, if arm also needs it.

Got it.
TCR_EL1.AS = 1 will be the 1st pr for this issue.
Thanks.

[laijs]

By the way, there is a segv error in InitUpperHalf() on Arm64.

updated

If @amscanne approved this PR, I will help you to fix this issue.
Don't worry about it.

updated again.

[laijs]

@lubinszARM Does the newest code work on arm64? Could you give it an Acked-by ?

[lubinszARM]

@lubinszARM Does the newest code work on arm64? Could you give it an Acked-by ?

Segv issue was gone and the results of the kvm test are passed on Arm64.

[laijs]

@amscanne I think the pr is ready to be merged.

[laijs]

rebased (conflicted with 983a55a) and updated.
@amscanne

[laijs]

ping @amscanne

[amscanne]

There are a few issues here, and I'll try to explain. I think there's a clear path to resolve them.

First, the internal allocator assumes unique ownership over the PTE entries. That means that it may reuse them when freed, since they go back into the pool.

Second, the "used" map tracks PTEs that are in use (owned by those page tables) so that they aren't garbage-collected.

As long as Unmap isn't called on the kernel range, then everything here should be fine... but I don't like to count on that. I'd prefer if there were a second structure and we maintained it properly. Could look something like this:

type SharedPageTables struct {
// mu protects below.
mu sync.Mutex

// impl is the set of PageTables. It holds ownership over all shared pages.
impl PageTables

// start is the end of the shared range.
//
// This must be accessed atomically for reads or with mu held for writes (still atomic).
start uint64

// end is the end of the shared range.
//
// This must be accessed atomically for reads or with mu held for writes (still atomic).
end uint64

// usedIn is the set of PageTables where this is mapped.
usedIn []*PageTables
}

// Map adds the given range to the SharedPageTables.
//
// The range will be contiguous coveraging range of all mapped ranges. Existing
// PageTables using this SharedPageTables will have mappings in this range
// removed and replaced with the SharedPageTables entries.
func (p *SharedPageTables) Map(addr usermem.Addr, length uintptr, opts MapOpts, physical uintptr) bool {
p.mu.Lock()
defer p.mu.Unlock()
if oldStart := p.start; addr < oldStart {
atomic.StoreUint64(&p.start, uint64(addr))
for _, other := range p.usedIn {
other.Unmap(addr, oldStart-p.start)
}
}
if oldEnd := p.end; uint64(addr+length) > oldEnd {
atomic.StoreUint64(&p.end, uint64(addr+length))
for _, other := range p.usedIn {
other.Unmap(usermem.Addr(oldEnd), uint64(addr+length)-oldEnd)
}
inv := p.impl.Map(addr, length, opts, physical)
for _, other := p.usedIn {
// Copy relevant page table entries...
}
}

func (p *SharedPageTables) Unmap(addr usermem.Addr, length uintptr) {
p.mu.Lock()
defer p.mu.Unlock()
// opposite of the above, and manually remove all entries in p.usedIn
// you could choose to shrink the shard range or let it grow only.
}

func (p *SharedPageTables) Apply(other *PageTables) {
p.mu.Lock()
defer p.mu.Unlock()
other.Unmap(p.start, p.end-p.start)
p.usedIn = append(p.usedIn, other)
// Copy all relevant page table entries.
...
}

struct PageTables struct {
...
shared *SharedPageTables
}

func (p *PageTables) Map(...) {
...
if p.shared != nil && atomic.LoadUintptr(&p.start) <= addr .... // Check for overlap and reject.
...
}

func (p *PageTables) Unmap(...) {
..
if p.shared != nil // checking for overlap, per above.
...
}

Then the machine.kernelPageTables would just become SharedPageTables. Anyhow, this is just a sketch of an idea to introduce a new type, track shared pages (and give them a clear owner), though I think there are other options here. Happy to discuss. I'm trying to merge the KPTI change right now (so that this can be rebase for merge as well).

[laijs]

@amscanne

Updated commit pushed.

Your concerns addressed.

And your suggestions are basically applied except that SharedPageTables is forced purely read-only, so there is no sync.Mutex and usedIn needed and start,end uint64 is folded into a constant with your suggestion for handling for overlapping is taken into code. So type SharedPageTables is just defined as type SharedPageTables PageTables. Oh, this line of code was dispensed with in the final commit.

[laijs]

@amscanne

Updated commit pushed again.

And execRegion is removed to meet your requirement in the KPTI patch since mapUpperHalf() is called only once now.

[amscanne]

Thanks, this looks great. I had some minor feedback and nits (and one enhancement), then I'm happy to get it merged.

[amscanne]

As discussion elsewhere, you should document the API semantics of upperHalfPageTables here, and include any preconditions. E.g.

// NewWithUpper returns new PageTables.
//
// upperHalfPageTables are used for mapping the upper half of addresses, starting at upperLimit.
// These pageTables should not be touched (as invalidations may be incorrect) after they are
// passed as an upperHalfPageTables. Only when all dependent PageTables are gone may they
// be used. The intenteded use case is for kernel page tables, which are static and fixed.
//
// Precondition: upperLimit must be between canonical ranges.
// Precondition: ...

[amscanne]

Thinking about it a bit further, it does make me nervous that someone could accidentally do something with kernelPageTables, and it would be really hard to identify the source of that bug. (Since e.g. page table memory might just get reused.)

I like the simplicity of just checking for things in Map/Unmap/etc. Can we just do something like this?

// MakeShared marks these PageTables as shared.
func (p *PageTables) MakeShared() {
   p.allocator.Drain() // Won't be modified.
   p.shared = true
}

// ...
// Precondition: p must not be shared.
func (p *PageTables) Map(...) {
   if p.shared {
       panic(...)
   }
   ...
}

// ...
// Precondition: upperHalfPageTables must be shared.
func NewWithUpper(a Allocator, upperHalfPageTables *pageTables) *PageTables {
    if upperHalfPageTables != nil {
         if !upperHalfPageTables.shared {
              panic(...)
         }
    }
    ...
}

That way everything is pretty simple (like the design you have here), and also still safe.

[amscanne]

One more good idea here: you can actually add an arch-hook for makeShared. E.g.

// MakeShared marks these PageTables as shared.
func (p *PageTables) MakeShared() {
   p.shared = true
   w := sharedWalker{
      pageTables: p,
      visitor: sharedVisitor{}
   }
   w.iterateRange(0, ^0)
   p.allocator.Drain() // Won't be modified.
}

On amd64, this could go and mark all relevant PTEs as G, so they are shared across all address spaces.

[laijs]

I like the idea to add the shared field in the Pagetables to make it read only. But I don't think we need to add a new sharedVisitor, we can just use G bit in mapUpperHalf() (and add X86_CR4_PGE bit in CR4).

[laijs]

@amscanne

The commit is updated as you requested.
(except sharedVisitor and G bit in PTEs)