Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: runtime: manage off-heap memory lifecycle using the garbage collector #70224

Open
ti-mo opened this issue Nov 6, 2024 · 9 comments
Labels
Milestone

Comments

@ti-mo
Copy link

ti-mo commented Nov 6, 2024

Proposal Details

Proposal

Hi folks, I'd like to explore the possibility for the runtime to 'adopt' externally-allocated memory by tracking pointers to the span and unmapping the underlying memory if there are no more references:

// TrackPointers tells the garbage collector to consider the off-heap memory
// span described by ptr and size as Go memory. finalize is scheduled for
// execution when the span is no longer referenced. The span is never reused
// to satisfy any other allocations.
//
// TrackPointers panics if the span overlaps with any existing memory span
// known to the Go runtime, heap or otherwise.
func TrackPointers(ptr unsafe.Pointer, size int, finalize func())

To be used as:

alloc, _ := unix.MmapPtr(mapFD, 0, nil, size, unix.PROT_READ|unix.PROT_WRITE, unix.MAP_SHARED)

runtime.TrackPointers(ptr, size, func(p unsafe.Pointer) {
_ = unix.MunmapPtr(p, size)
})

Or, alternatively, a variant without the finalize argument that allows setting a finalizer explicitly:

alloc, _ := unix.MmapPtr(mapFD, 0, nil, size, unix.PROT_READ|unix.PROT_WRITE, unix.MAP_SHARED)

runtime.TrackPointers(ptr, size)

runtime.SetFinalizer(alloc, func(p unsafe.Pointer) {
_ = unix.MunmapPtr(p, size)
})

Or, if that's equally undesirable, an internal symbol we can //go:linkname and make unix.Mmap use it transparently.


Background

I'm working on a new feature in ebpf-go. A while ago, the Linux kernel gained the abillity to map the contents of a bpf map into process memory using mmap(), essentially treating bpf map contents as a file. Historically, all map accesses required preparing buffers for the key and value to pass to a bpf() syscall, a costly operation for busy maps. The mmapable map change made it so map accesses can be done by simply reading or writing to a memory location in a user space process, speeding things up by an order of magnitude or more.

Naturally, we'd like to enjoy the benefits of mmapable maps over in Go land as well, but this poses some unique challenges around memory management, specifically for managing the lifecycle of the underlying memory mappings. A common use case for mmapable maps is interacting with global BPF C variables. These are laid out in the typical data sections like .bss, .data and .rodata and are exposed to user space as plain BPF array maps. My goal is to be able to represent a global C variable like

volatile __u16 global_u16;

as a canonical Go variable, albeit a pointer. For example:

var GlobalUint16 *uint16

This becomes even more interesting if the global variable is only accessed atomically (using __sync_* primitives) on the C side, allowing the shared memory to be reinterpreted as a Go atomic type like atomic.Uint16, automatically giving the caller access to all operations implemented on those types.

Here's a playground link sketching the overall idea: https://go.dev/play/p/NyoPxKZbK5R. (Note: run this locally, playground runners lack CAP_ADMIN and/or CAP_BPF.)

Since the runtime doesn't track references to this mmap()ed region, we need to bind the lifecycle of the memory mapping to some Go object (in my ebpf-go proposal, this is modeled as an ebpf.Memory struct), but care needs to be taken not to lose the reference to this object if we allow the caller to take pointers to the underlying memory. The risk of a use-after-free is high.


This is somewhat the inverse of Go arenas, yet tangentially-related. I was sad to find out the arenas proposal is on hold indefinitely, since it would've opened the door for some more manual memory management in Go. Aside from accessing bpf maps, I can imagine this mechanism being useful for databases or zero-copy file parsers, as it would enable passing Go pointers to structs that reside in file-backed memory, without worrying about use-after-free.

I originally got this idea from https://pkg.go.dev/github.com/Jille/gcmmap, a package that mmaps over the Go heap using MAP_FIXED. It uses runtime.mallocgc() but allocating a byte slice works just as well. I experimented with this approach for a few weeks and it happens to work beautifully, but it makes several hard assumptions:

  • there's no moving garbage collector (although we have runtime.heapObjectsCanMove nowadays)
  • the heap is always mapped using PROT_READ|PROT_WRITE and MAP_ANON|MAP_PRIVATE
  • there are no other protections like mseal()

Not to mention the risk of accidentally clearing a part of, or leaving a hole in the middle of the heap. Our package powers many mission-critical systems, and this feature would be enabled by default, which means we need to be careful. ebpf-go already made itself into the go:linkname hall of shame, so I'll try not to exacerbate that issue further. 🙂

Please let me know what you think. Thank you!

@ti-mo ti-mo added the Proposal label Nov 6, 2024
@gopherbot gopherbot added this to the Proposal milestone Nov 6, 2024
@randall77
Copy link
Contributor

I'm not entirely sure why this wouldn't work for you:

type int32holder struct {
    p *atomic.Int32
}
func (h *int32holder) Load() int32 {
    v := h.p.Load()
    runtime.KeepAlive(h)
    return v
}
// Same for store
var variable1 *int32holder
func init() {
    p := (*atomic.Int32)(mmap(...))
    h := &int32holder{p}
    runtime.SetFinalizer(h, func(h *int32holder) { munmap(h.p) })
    variable1 = h
    // or pass h around wherever
}

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Nov 6, 2024
@ianlancetaylor
Copy link
Contributor

CC @golang/runtime

@mknyszek
Copy link
Contributor

mknyszek commented Nov 6, 2024

FWIW this has come up between a few of us before (CC @dr2chase) and there are some benefits to direct support vs. @randall77's approach (which would also work). Mainly, it just smooths over some friction with interacting with non-Go managed memory, including C values, and makes them a little less error-prone to work with.

Aside from that, I don't like the name TrackPointers very much because it might make the caller think it tracks pointers inside the provided memory, which it very much does not. You absolutely cannot write Go pointers into that memory. What about runtime.AddForeignCleanup ("foreign" in the FFI sense), which matches the new proposal-accepted runtime.AddCleanup?

Lastly, I wonder if this is something that should go in the unsafe package? I am concerned about giving any user a false sense of security about the memory passed to this function.

@prattmic
Copy link
Member

prattmic commented Nov 6, 2024

Leaving aside the question of whether this should go in unsafe for sense-of-security reasons, is there some reason that this couldn't just be part of AddCleanup itself? i.e., AddCleanup would allow passing non-Go pointers?

@mknyszek
Copy link
Contributor

mknyszek commented Nov 6, 2024

@prattmic It almost could, but AddCleanup isn't sufficiently general enough. To pass a "size" parameter you'd need to model your value in the type system, otherwise the runtime has no idea what the actual span of memory it needs to cover is. (Slices also won't be supported to begin with, and it seems a little weird to use the slice bounds as the cleanup bounds.)

@mejedi
Copy link

mejedi commented Nov 7, 2024

tracking pointers to the [mmapped memory] and unmapping the underlying memory if there are no more references

I believe that proposed feature is valuable beyond ebpf use cases as well.

The challenge with Mmap/MmapPtr is that it is inherently unsafe. A dangling pointer left behind faults on access if we are lucky. If the address space was reused in meantime, we may end up corrupting something else.

It is true that Golang has unsafe anyway and there are many ways to sabotage oneself. Unsafe requires extreme caution though. It is easier to use correctly if the context we have to reason about is small. That's why packages normally offer safe interfaces, even if leveraging unsafe internally.

What if a package needs to expose data in mmap-ed memory? Making copies is rarely an option (overhead). Wrappers do not interface well with existing packages. If it was possible to expose *T or []T backed by mmap safely, it would be a tremendous improvement.

(For a concrete example, consider memory-mapped AF_PACKET and AF_XDP sockets in Linux. It is essentially a ring buffer shared between user space and the kernel. If we build a reusable package for interfacing with this ring buffer, we aren't going to handle queued packets internally, but rather expose them to the package user.)

@ti-mo
Copy link
Author

ti-mo commented Nov 7, 2024

@randall77 Thanks for the suggestion, but as you'd expect I glossed over a lot of the details that led us here. We've prototyped nearly every API we could come up with, but we keep running into potential memory safety issues. Check the two PRs I linked from if you're interested.

My first attempt looked a little like what you proposed, but since this is library code, it's hard to build something flexible enough to be useful to the majority of users. The underlying types represented by this memory can be any C type, including structs, including ones with fields accessed atomically, etc. Initially we had a bunch of functions returning atomic.(U)int* types embedded into some unexported types (to prevent the caller coaxing out the real pointers), but this got messy fast and still had the potential for use-after-free.

Also, a mapping represents a datasec, so potentially contains n amount of variables. We could implement a refcount mechanism on the *Memory managed by finalizers, but then the caller must be very careful not to copy any of the pointers, which is equally dangerous. In the end, we concluded that the only way to make this both safe and ergonomic and fast was to rely on the GC to free the underlying mapping, so we ended up with something like this:

// Memory is just a []byte and a bool as a readonly flag.
func MemoryPointer[T any](mm *Memory, off uint64) (*T, error)

// A Variable is carved out of a Memory at a given offset.
func VariablePointer[T any](v *Variable) (*T, error)

Another thing I tried was returning a **T so I could set a finalizer holding a *Memory reference, but I mean...

u32, _ := VariablePointer[uint32](v)
(**u32) += 1

// or

a64, _ := VariablePointer[atomic.Uint64](v)
(*a64).Add(1)

This technically works, but it's clunky and requires more unnecessary pointer chasing. If we ever end up in a situation where we can return *T instead of **T, we'd need to break the API, so I'd rather hold off.


@mknyszek Thanks for the input!

Aside from that, I don't like the name TrackPointers very much [...] What about runtime.AddForeignCleanup ("foreign" in the FFI sense), which matches the new proposal-accepted runtime.AddCleanup?

This is still a rough proposal, naming should probably be decided by someone more knowledgeable about the runtime/allocator/GC. 😉 runtime.AddForeignCleanup() sounds good, but it doesn't accurately describe 'what' that 'Foreign' is supposed to represent. (memory, not Go objects) runtime.AddForeignMemoryCleanup()? That's getting long. runtime.AddPointerCleanup()? Let's take some time to mull this over. 🙂

because it might make the caller think it tracks pointers inside the provided memory, which it very much does not. You absolutely cannot write Go pointers into that memory.

Actually... 😉 Linux' bpf uapi is now frozen, which means no new map types will be introduced solely for bringing new data types to bpf. Going forward, new shared user/kernel datatypes will need to be implemented on top of a so-called 'bpf arena', which is a 4GiB range of memory that can be mapped into user space, exactly like the array map I demonstrated above. These contain pages allocated dynamically by bpf programs. Either side can write pointers into this arena, and as long as they point to somewhere inside the arena itself, the kernel will translate the pointers between kernel and user address space. (Note: it knows which values are pointers, but that's another story) Any pointers pointing outside the arena cause an exception when dereferenced from within a bpf program.

Just to add some nuance to your statement, 'cannot' should really be 'should not'. The user indeed shouldn't expect the GC to follow an off-heap pointer into the Go heap when scanning for references, so when designing these data types, care needs to be taken not to take a caller-provided Go pointer and stuff it somewhere off-heap. This property is definitely something we should document if/when working on implementing this proposal. As I understand it, the Go Arenas concept is stalled for similar reasons, though in reverse. (Go pointers into an arena can become dangling when the arena is freed)

Lastly, I wonder if this is something that should go in the unsafe package? I am concerned about giving any user a false sense of security about the memory passed to this function.

Makes sense, but nothing in the unsafe package currently modifies runtime state, so I'm not sure it fits. I'd argue there's nothing inherently unsafe about this API, since it doesn't need to dereference the given pointer and doesn't necessarily sidestep Go's type system. If anything, it aims to enhance memory safety.

@mejedi floated making this an implicit part of unix.Mmap, which made me think of a potentially-unintended interaction. unix.Mmap maintains a strong (unsafe.Pointer) reference to the underlying memory in some sort of internal cache. This isn't really documented and requires some spelunking through sys.mmapper. It would act as a permanent reference if used as follows, causing the finalizer to never run:

b, _ := unix.Mmap(...)
runtime.TrackPointers(unsafe.SliceData(b), len(b), func (ptr unsafe.Pointer) {
  unix.Munmap(unsafe.Slice(ptr, len(b)))
})

Using unix.MmapPtr instead of unix.Mmap would be necessary to make this work. Integrating this as part of unix.Mmap would make sense, but would technically be a breaking change. Although one could argue that if you lose the reference to b, you clearly have a memory leak. (barring bugs in uintptr() conversions, though this is a well-known footgun of unsafe)

@mknyszek
Copy link
Contributor

mknyszek commented Nov 7, 2024

This is still a rough proposal, naming should probably be decided by someone more knowledgeable about the runtime/allocator/GC. 😉 runtime.AddForeignCleanup() sounds good, but it doesn't accurately describe 'what' that 'Foreign' is supposed to represent. (memory, not Go objects) runtime.AddForeignMemoryCleanup()? That's getting long. runtime.AddPointerCleanup()? Let's take some time to mull this over. 🙂

AddForeignMemoryCleanup is not bad. I think it's OK for it to be long. While useful, this is a fairly niche concept that I suspect most Go developers will not need to interact with, so being clear in the API seems more important than being concise here.

Actually... 😉 Linux' bpf uapi is now frozen, which means no new map types will be introduced solely for bringing new data types to bpf. Going forward, new shared user/kernel datatypes will need to be implemented on top of a so-called 'bpf arena', which is a 4GiB range of memory that can be mapped into user space, exactly like the array map I demonstrated above. These contain pages allocated dynamically by bpf programs. Either side can write pointers into this arena, and as long as they point to somewhere inside the arena itself, the kernel will translate the pointers between kernel and user address space. (Note: it knows which values are pointers, but that's another story) Any pointers pointing outside the arena cause an exception when dereferenced from within a bpf program.

I mean that's fine. That's just writing a non-Go pointer into that memory. Maybe I misunderstand your point.

Just to add some nuance to your statement, 'cannot' should really be 'should not'.

Er, sure. You can pin the memory. This is closely related to the cgo pointer rules, so what I really meant to say was you absolutely cannot write unpinned Go pointers into C memory.

Lastly, I wonder if this is something that should go in the unsafe package? I am concerned about giving any user a false sense of security about the memory passed to this function.

Makes sense, but nothing in the unsafe package currently modifies runtime state, so I'm not sure it fits. I'd argue there's nothing inherently unsafe about this API, since it doesn't need to dereference the given pointer and doesn't necessarily sidestep Go's type system. If anything, it aims to enhance memory safety.

That is a fair point; it's just an idea. Mainly what I'm trying to get across is that there should be alarm bells ringing inside anyone's head when they see this function being used that something subtle is happening. The runtime APIs all tend to be more memory safe than this. One option is to put it in runtime/cgo even though it doesn't strictly require cgo, it does certainly help. (And I think it fits OK alongside something like runtime/cgo.Handle too.)

@mejedi floated making this an implicit part of unix.Mmap, which made me think of a potentially-unintended interaction.

Yeah, that would be a more specific subset of this functionality. I'm pretty sure this has also come up before a bunch of times, though I'm not certain there's an issue open for it (wouldn't be surprised if there is).

Thing is, if we have mechanism in the runtime for unix.Mmap, it's really trivial to extend that to the API proposed here. EDIT: This is actually a substantially easier problem, because the minimum size is a 4 KiB aligned region. This is much easier to represent in the runtime's existing data structures.

One thing not discussed in this issue yet is the actual implementation of this functionality. That part is easy if we place restrictions on the size and shape of these regions (must be at least one physical page in size). If we want to support arbitrarily-sized things, like anything that comes from malloc, that becomes substantially harder, because we need full on byte-level tracking (although this would certainly be the right thing to do).

@apparentlymart
Copy link

For whatever this one opinion is worth, the fact that the first argument is an unsafe.Pointer already hinted to me that this was a pretty niche and subtle function, regardless of what package the function itself is placed in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

8 participants