Open ti-mo opened 2 weeks ago
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
}
CC @golang/runtime
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.
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?
@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.)
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.)
@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. :wink: 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. :slightly_smiling_face:
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... :wink: 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
)
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).
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.
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:
To be used as:
Or, alternatively, a variant without the finalize argument that allows setting a finalizer explicitly:
Or, if that's equally undesirable, an internal symbol we can
//go:linkname
and makeunix.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 likeas a canonical Go variable, albeit a pointer. For example:
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 likeatomic.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:runtime.heapObjectsCanMove
nowadays)PROT_READ|PROT_WRITE
andMAP_ANON|MAP_PRIVATE
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. :slightly_smiling_face:Please let me know what you think. Thank you!