From ca8d05a657efae805851d0ab95dc9b7aea98bbf2 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Tue, 1 Oct 2024 17:03:56 -0700 Subject: [PATCH] platform/kvm: refactor handleBluepillFault to reduce stack usage PiperOrigin-RevId: 681217279 --- pkg/sentry/platform/kvm/BUILD | 17 ----------- pkg/sentry/platform/kvm/address_space.go | 2 +- pkg/sentry/platform/kvm/bluepill_allocator.go | 2 +- pkg/sentry/platform/kvm/bluepill_fault.go | 30 ++++++------------- pkg/sentry/platform/kvm/machine.go | 12 ++++---- .../platform/kvm/machine_amd64_unsafe.go | 6 +++- pkg/sentry/platform/kvm/seccomp_mmap_dbg.go | 27 ----------------- ...mp_mmap_real.go => seccomp_mmap_unsafe.go} | 2 +- 8 files changed, 22 insertions(+), 76 deletions(-) delete mode 100644 pkg/sentry/platform/kvm/seccomp_mmap_dbg.go rename pkg/sentry/platform/kvm/{seccomp_mmap_real.go => seccomp_mmap_unsafe.go} (97%) diff --git a/pkg/sentry/platform/kvm/BUILD b/pkg/sentry/platform/kvm/BUILD index 8578affd7..35b0c8120 100644 --- a/pkg/sentry/platform/kvm/BUILD +++ b/pkg/sentry/platform/kvm/BUILD @@ -24,23 +24,6 @@ config_setting( }, ) -# @unused -glaze_ignore = [ - "seccomp_mmap_dbg.go", - "seccomp_mmap_real.go", -] - -# Use either seccomp_mmap_dbg.go or seccomp_mmap_real.go as seccomp_mmap.go. -genrule( - name = "seccomp_mmap", - srcs = select({ - ":debug_build": ["seccomp_mmap_dbg.go"], - "//conditions:default": ["seccomp_mmap_real.go"], - }), - outs = ["seccomp_mmap_unsafe.go"], - cmd = "cat < $(SRCS) > $(OUTS)", -) - go_library( name = "kvm", srcs = [ diff --git a/pkg/sentry/platform/kvm/address_space.go b/pkg/sentry/platform/kvm/address_space.go index 79ccbea35..1b16dcb3c 100644 --- a/pkg/sentry/platform/kvm/address_space.go +++ b/pkg/sentry/platform/kvm/address_space.go @@ -121,7 +121,7 @@ func (as *addressSpace) mapLocked(addr hostarch.Addr, m hostMapEntry, at hostarc // not have physical mappings, the KVM module may inject // spurious exceptions when emulation fails (i.e. it tries to // emulate because the RIP is pointed at those pages). - as.machine.mapPhysical(physical, length, physicalRegions) + as.machine.mapPhysical(physical, length) // Install the page table mappings. Note that the ordering is // important; if the pagetable mappings were installed before diff --git a/pkg/sentry/platform/kvm/bluepill_allocator.go b/pkg/sentry/platform/kvm/bluepill_allocator.go index 3e666010a..f36f3498e 100644 --- a/pkg/sentry/platform/kvm/bluepill_allocator.go +++ b/pkg/sentry/platform/kvm/bluepill_allocator.go @@ -73,7 +73,7 @@ func (a *allocator) PhysicalFor(ptes *pagetables.PTEs) uintptr { // //go:nosplit func (a *allocator) LookupPTEs(physical uintptr) *pagetables.PTEs { - virtualStart, physicalStart, _, pr := calculateBluepillFault(physical, physicalRegions) + virtualStart, physicalStart, _, pr := calculateBluepillFault(physical) if pr == nil { panic(fmt.Sprintf("LookupPTEs failed for 0x%x", physical)) // escapes: panic. } diff --git a/pkg/sentry/platform/kvm/bluepill_fault.go b/pkg/sentry/platform/kvm/bluepill_fault.go index 3d5a8cd1a..4e51e64ae 100644 --- a/pkg/sentry/platform/kvm/bluepill_fault.go +++ b/pkg/sentry/platform/kvm/bluepill_fault.go @@ -47,9 +47,9 @@ func yield() { // calculateBluepillFault calculates the fault address range. // //go:nosplit -func calculateBluepillFault(physical uintptr, phyRegions []physicalRegion) (virtualStart, physicalStart, length uintptr, pr *physicalRegion) { +func calculateBluepillFault(physical uintptr) (virtualStart, physicalStart, length uintptr, pr *physicalRegion) { alignedPhysical := physical &^ uintptr(hostarch.PageSize-1) - for i, pr := range phyRegions { + for i, pr := range physicalRegions { end := pr.physical + pr.length if physical < pr.physical || physical >= end { continue @@ -63,27 +63,14 @@ func calculateBluepillFault(physical uintptr, phyRegions []physicalRegion) (virt physicalEnd = end } length = physicalEnd - physicalStart - return virtualStart, physicalStart, length, &phyRegions[i] + return virtualStart, physicalStart, length, &physicalRegions[i] } return 0, 0, 0, nil } -// handleBluepillFault handles a physical fault. -// -// The corresponding virtual address is returned. This may throw on error. -// //go:nosplit -func handleBluepillFault(m *machine, physical uintptr, phyRegions []physicalRegion) (uintptr, bool) { - // Paging fault: we need to map the underlying physical pages for this - // fault. This all has to be done in this function because we're in a - // signal handler context. (We can't call any functions that might - // split the stack.) - virtualStart, physicalStart, length, pr := calculateBluepillFault(physical, phyRegions) - if pr == nil { - return 0, false - } - +func (m *machine) mapMemorySlot(virtualStart, physicalStart, length uintptr, readOnly bool) { // Set the KVM slot. // // First, we need to acquire the exclusive right to set a slot. See @@ -94,7 +81,7 @@ func handleBluepillFault(m *machine, physical uintptr, phyRegions []physicalRegi slot = m.nextSlot.Swap(^uint32(0)) } flags := _KVM_MEM_FLAGS_NONE - if pr.readOnly { + if readOnly { flags |= _KVM_MEM_READONLY } errno := m.setMemoryRegion(int(slot), physicalStart, length, virtualStart, flags) @@ -106,7 +93,7 @@ func handleBluepillFault(m *machine, physical uintptr, phyRegions []physicalRegi // Successfully added region; we can increment nextSlot and // allow another set to proceed here. m.nextSlot.Store(slot + 1) - return virtualStart + (physical - physicalStart), true + return } // Release our slot (still available). @@ -117,7 +104,7 @@ func handleBluepillFault(m *machine, physical uintptr, phyRegions []physicalRegi // The region already exists. It's possible that we raced with // another vCPU here. We just revert nextSlot and return true, // because this must have been satisfied by some other vCPU. - return virtualStart + (physical - physicalStart), true + return case unix.EINVAL: throw("set memory region failed; out of slots") case unix.ENOMEM: @@ -125,8 +112,9 @@ func handleBluepillFault(m *machine, physical uintptr, phyRegions []physicalRegi case unix.EFAULT: throw("set memory region failed: invalid physical range") default: + printHex([]byte("set memory region failed:"), uint64(errno)) throw("set memory region failed: unknown reason") } - panic("unreachable") + throw("unreachable") } diff --git a/pkg/sentry/platform/kvm/machine.go b/pkg/sentry/platform/kvm/machine.go index ebaccb2eb..3fefbadfb 100644 --- a/pkg/sentry/platform/kvm/machine.go +++ b/pkg/sentry/platform/kvm/machine.go @@ -371,7 +371,7 @@ func newMachine(vm int) (*machine, error) { } // Ensure the physical range is mapped. - m.mapPhysical(physical, length, physicalRegions) + m.mapPhysical(physical, length) virtual += length } } @@ -396,7 +396,7 @@ func newMachine(vm int) (*machine, error) { }) if mapEntireAddressSpace { for _, r := range physicalRegions { - m.mapPhysical(r.physical, r.length, physicalRegions) + m.mapPhysical(r.physical, r.length) } } enableAsyncPreemption() @@ -438,9 +438,9 @@ func (m *machine) hasSlot(physical uintptr) bool { // This throws on error. // //go:nosplit -func (m *machine) mapPhysical(physical, length uintptr, phyRegions []physicalRegion) { +func (m *machine) mapPhysical(physical, length uintptr) { for end := physical + length; physical < end; { - _, physicalStart, length, pr := calculateBluepillFault(physical, phyRegions) + virtualStart, physicalStart, length, pr := calculateBluepillFault(physical) if pr == nil { // Should never happen. throw("mapPhysical on unknown physical address") @@ -448,9 +448,7 @@ func (m *machine) mapPhysical(physical, length uintptr, phyRegions []physicalReg // Is this already mapped? Check the usedSlots. if !m.hasSlot(physicalStart) { - if _, ok := handleBluepillFault(m, physical, phyRegions); !ok { - throw("handleBluepillFault failed") - } + m.mapMemorySlot(virtualStart, physicalStart, length, pr.readOnly) } // Move to the next chunk. diff --git a/pkg/sentry/platform/kvm/machine_amd64_unsafe.go b/pkg/sentry/platform/kvm/machine_amd64_unsafe.go index 3ec39ca80..9fb09a449 100644 --- a/pkg/sentry/platform/kvm/machine_amd64_unsafe.go +++ b/pkg/sentry/platform/kvm/machine_amd64_unsafe.go @@ -192,7 +192,11 @@ func seccompMmapSyscall(context unsafe.Pointer) (uintptr, uintptr, unix.Errno) { // MAP_DENYWRITE is deprecated and ignored by kernel. We use it only for seccomp filters. addr, e := hostsyscall.RawSyscall6(uintptr(ctx.Rax), uintptr(ctx.Rdi), uintptr(ctx.Rsi), uintptr(ctx.Rdx), uintptr(ctx.R10)|unix.MAP_DENYWRITE, uintptr(ctx.R8), uintptr(ctx.R9)) - ctx.Rax = uint64(addr) + if e != 0 { + ctx.Rax = uint64(-e) + } else { + ctx.Rax = uint64(addr) + } return addr, uintptr(ctx.Rsi), unix.Errno(e) } diff --git a/pkg/sentry/platform/kvm/seccomp_mmap_dbg.go b/pkg/sentry/platform/kvm/seccomp_mmap_dbg.go deleted file mode 100644 index 4d902233f..000000000 --- a/pkg/sentry/platform/kvm/seccomp_mmap_dbg.go +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2024 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//go:build linux -// +build linux - -package kvm - -import ( - "unsafe" -) - -//go:nosplit -func seccompMmapHandler(context unsafe.Pointer) { - throw("seccompMmapHandler isn't implemented for debug builds") -} diff --git a/pkg/sentry/platform/kvm/seccomp_mmap_real.go b/pkg/sentry/platform/kvm/seccomp_mmap_unsafe.go similarity index 97% rename from pkg/sentry/platform/kvm/seccomp_mmap_real.go rename to pkg/sentry/platform/kvm/seccomp_mmap_unsafe.go index b5048c625..48adb4f3a 100644 --- a/pkg/sentry/platform/kvm/seccomp_mmap_real.go +++ b/pkg/sentry/platform/kvm/seccomp_mmap_unsafe.go @@ -61,7 +61,7 @@ func seccompMmapHandler(context unsafe.Pointer) { } // Ensure the physical range is mapped. - m.mapPhysical(physical, length, physicalRegions) + m.mapPhysical(physical, length) virtual += length } }