treewide: use Pinner to resolve safety issues
Prior to Go 1.21 the only way to pass Go-managed pointers to the kernel
was to convert them to a uintptr inside the syscall argument expression.
This pattern was special-cased in the compiler to prevent the referenced
memory from being moved by an eventual moving GC in Go while the syscall
is running (thus corrupting the Go heap).
But this was very restrictive as there are syscalls which take inputs
containing further pointers. According to the official rules this could
not be implemented safely.
In practice you could just do it anyways as the current Go GC does
in general not move objects, but it was always kind of a hack.
With Go 1.21 there is a new Pinner API which can be used to pin the
memory which is going to be referenced in these structures, allowing
them to be constructed and used over multiple calls.
runtime.KeepAlive is still required to prevent finalizers from running
prematurely.
Use this new API and remove the relevant comments.
Change-Id: I26bce06e1c20a5fe0c41f9ae736a895f533674c1
Reviewed-on: https://review.monogon.dev/c/monogon/+/2316
Tested-by: Jenkins CI
Reviewed-by: Serge Bazanski <serge@monogon.tech>
diff --git a/metropolis/pkg/nvme/cmd_linux.go b/metropolis/pkg/nvme/cmd_linux.go
index e4353cc..96054ff 100644
--- a/metropolis/pkg/nvme/cmd_linux.go
+++ b/metropolis/pkg/nvme/cmd_linux.go
@@ -64,21 +64,13 @@
cdw15: cmd.CDW15,
timeoutMs: uint32(cmd.Timeout.Milliseconds()),
}
- // NOTE: Currently this is safe (even if the documentation says otherwise)
- // as the runtime.KeepAlive call below ensures that the GC cannot clean up
- // the memory segments passed as data and metadata. This is sufficient as
- // Go's runtime currently does not use a moving GC, meaning that these
- // pointers do not get invalidated as long as they are considered alive.
- // In case Go introduces a moving GC, which they might want to do this will
- // no longer be safe as a GC-initiated move can happen while the syscall is
- // running, causing the kernel to overwrite random memory of the calling
- // process. To avoid this, these data structures need to be pinned. But Go
- // doesn't have a pinning API yet [1], so all I can do is note this here.
- // [1] https://github.com/golang/go/issues/46787
+ var ioctlPins runtime.Pinner
+ defer ioctlPins.Unpin()
if cmd.Data != nil {
if len(cmd.Data) > math.MaxUint32 {
return errors.New("data buffer larger than uint32, this is unsupported")
}
+ ioctlPins.Pin(&cmd.Data[0])
cmdRaw.dataLen = uint32(len(cmd.Data))
cmdRaw.addr = uint64(uintptr(unsafe.Pointer(&cmd.Data[0])))
}
@@ -86,6 +78,7 @@
if len(cmd.Metadata) > math.MaxUint32 {
return errors.New("metadata buffer larger than uint32, this is unsupported")
}
+ ioctlPins.Pin(&cmd.Metadata[0])
cmdRaw.metadataLen = uint32(len(cmd.Metadata))
cmdRaw.metadata = uint64(uintptr(unsafe.Pointer(&cmd.Metadata[0])))
}
diff --git a/metropolis/pkg/scsi/scsi_linux.go b/metropolis/pkg/scsi/scsi_linux.go
index be937d0..21af01d 100644
--- a/metropolis/pkg/scsi/scsi_linux.go
+++ b/metropolis/pkg/scsi/scsi_linux.go
@@ -47,6 +47,13 @@
return errors.New("CDB larger than 2^8 bytes, unable to issue")
}
var senseBuf [32]byte
+
+ var ioctlPins runtime.Pinner
+ ioctlPins.Pin(&c.Data[0])
+ ioctlPins.Pin(&cdb[0])
+ ioctlPins.Pin(&senseBuf[0])
+ defer ioctlPins.Unpin()
+
cmdRaw := sgIOHdr{
Interface_id: 'S',
Dxfer_direction: dxferDir,
diff --git a/net/dump/hwaddr_compat.go b/net/dump/hwaddr_compat.go
index 1e79e36..9cd99a8 100644
--- a/net/dump/hwaddr_compat.go
+++ b/net/dump/hwaddr_compat.go
@@ -48,13 +48,17 @@
}
}
defer unix.Close(fd)
+
+ var ioctlPins runtime.Pinner
+ defer ioctlPins.Unpin()
+
var data ethtoolPermAddr
data.Cmd = unix.ETHTOOL_GPERMADDR
data.Size = uint32(len(data.Data))
+
var req ifreq
copy(req.ifname[:], ifName)
- // See //metropolis/pkg/nvme:cmd_linux.go RawCommand notice on the safety
- // of this.
+ ioctlPins.Pin(&data)
req.data = uintptr(unsafe.Pointer(&data))
for {
_, _, err := unix.Syscall(unix.SYS_IOCTL, uintptr(fd), unix.SIOCETHTOOL, uintptr(unsafe.Pointer(&req)))