variable: return native Go pointers to bpf map memory

[ti-mo]

Pending the proposal in golang/go#70224, here's the current API proposal for returning Go pointers to 'off-heap' bpf map memory:

func MemoryPointer[T any](mm *Memory, off uint64) (*T, error)
func VariablePointer[T any](v *Variable) (*T, error)

We need a bit of help from the Go runtime to manage the lifecycle of the underlying memory mapping. Currently, this mmaps over a piece of Go heap to allow the GC to track pointers into the bpf map memory, obviating the need for tying the mmap's lifetime to an ebpf.Memory. Instead, any Go pointers into the mmap will keep it alive, removing the risk of use-after-free.

Clearly, this involves some dark arts, so we're not comfortable pushing this on users since Collection.Variables is populated by default. Any change in the runtime or allocator may break this, and the fallout means Go programs segfaulting without any hint as to why, which is not acceptable.

--- Edit ---

Since the runtime.AddManualMemoryCleanup proposal may take a while to implement, land and ship in a Go version we target, I've revived the patch set and put the heap-mapping behaviour behind CollectionOptions.UnsafeVariableExperiment. MemoryPointer is yet unexported, since Map.Memory() is typically called directly by the user and we'd need to store the feature flag in *Map, which doesn't feel ideal.

Alternatively, we could go for an env feature flag instead of CollectionOptions (EBPFUNSAFEVARIABLES=true?), which would allow us to export MemoryPointer and transparantly switch between implementations.

[lmb]

Can either of you refresh my mind again what the killer feature for this is? Seems like @mejedi is interested in using this for slices?

[lmb]

Why is this binary.Size and not unsafe.Sizeof?

[lmb]

This also doesn't take unsafe.Alignof into account.

[ti-mo]

Why is this binary.Size and not unsafe.Sizeof?

unsafe.Sizeof() tolerates variable-sized types like untyped ints and also returns the size of the slice descriptor if the argument is a slice. binary.Size() rejects both. I think it's safer to reject variable-sized for now to avoid potential surprises.

This also doesn't take unsafe.Alignof into account.

Don't the next few lines take care of ensuring aligned access?

[lmb]

I think it's safer to reject variable-sized for now to avoid potential surprises.

Unfortunately binary.Size ignores trailing padding, which means that it is not safe to use that function here.

struct foo {
  uint64_t a;
  uint32_t b;
  // implicit 4 byte padding
}

I agree with you that these types shouldn't be allowed, but I think it might be better to do that in the caller of this function. Then we have a separation of concerns:

• this function ensures that we only transmute memory that is "safe" per the memory model, respecting full size and alignment.
• the caller can further restrict this to sized types, maybe ones that embed structs.HostLayout, etc.

Don't the next few lines take care of ensuring aligned access?

The alignment of a type can be smaller than its size, so I think that the check below is incorrect in the sense that it rejects correctly aligned structs. For example:

struct foo {
  uint64_t a, b;
}

This must be aligned to 8 bytes, not 16.

[ti-mo]

Unfortunately binary.Size ignores trailing padding, which means that it is not safe to use that function here.

I see. I guess that's the reasoning behind unsafeBackingMemory as well; there you're using reflect.Type.Size(), which is analogous to unsafe.Sizeof(). Then, you compare it to binary.Size() to detect padding.

Thanks for the feedback! I'll see what I can come up with. Looks like we also need better tests. 🙂

[ti-mo]

This should now be addressed, PTAL.

[ti-mo]

Can either of you refresh my mind again what the killer feature for this is? Seems like @mejedi is interested in using this for slices?

I think the most pressing use case is atomics, which is currently not supported at all. Array variables currently work through Get() and Set(), but there's always a marshaling cost. Also, simply having the convenience of manipulating BPF variables like they are Go variables is nice.

[lmb]

I think it's safer to reject variable-sized for now to avoid potential surprises.

Unfortunately binary.Size ignores trailing padding, which means that it is not safe to use that function here.

struct foo {
  uint64_t a;
  uint32_t b;
  // implicit 4 byte padding
}

I agree with you that these types shouldn't be allowed, but I think it might be better to do that in the caller of this function. Then we have a separation of concerns:

• this function ensures that we only transmute memory that is "safe" per the memory model, respecting full size and alignment.
• the caller can further restrict this to sized types, maybe ones that embed structs.HostLayout, etc.

Don't the next few lines take care of ensuring aligned access?

The alignment of a type can be smaller than its size, so I think that the check below is incorrect in the sense that it rejects correctly aligned structs. For example:

struct foo {
  uint64_t a, b;
}

This must be aligned to 8 bytes, not 16.

[lmb]

I should've said: it also only contains unexported fields. Passing the type to binary.Read or Write will cause an error. Is that the correct choice to make here?

[lmb]

Thanks for adding all those tests, they look excellent to me! Left one thought around how to enable / disable this, feel free to deal with that as you wish. I have concerns around what types should be allowed, where I'd appreciate it if we can be even more conservative for a start.

[lmb]

I remember that you mentioned using an environment variable for this. You ended up deciding against it? I kind of came to like that idea.

Using an env variable has the nice property that the author of the application has to opt in to enabling this. It would discourage / prevent use of this feature in libraries. The downside is that applications have to os.Setenv in main.

Another way to tackle that would be using a (much hated by you) build tag. That has similar behaviour (the author of the binary gets to decide) without having to much with the environment.

[ti-mo]

Yep, didn't manage to get that done last week. Initially, I was tempted to extend the experiment to MemoryPointer() as well, but while implementing the type checker, I realized MemoryPointer() for arenas would be relatively useless without allowing pointers, so that needs a bit more work.

[lmb]

Out of curiosity: What happens when doing Memory() and then Freeze()? Does freezing fail?

[ti-mo]

EBUSY: https://elixir.bootlin.com/linux/v6.13.7/source/kernel/bpf/syscall.c#L2182

[lmb]

I'd argue yes; it would be quite hamstrung otherwise.

There are atomic free functions that you can use without relying on the atomic types.

but if someone wants to stuff a mutex in there, it feels far-fetched to try and stop them.

Why would they want to though? The shape and internal semantics of a Go mutex are not documented, so the only way to share one between C and Go is to look at the implementation details of the Go runtime. That seems like a place of hurt to me. Makes a lot more sense to me the other way around, where C exposes a "stable" API for a mutex that is shared between kernel and user space. We'd then provide a blessed implementation of that (using free atomic functions for example).

I saw that you added structs.HostLayout to one of the tests and that made me think it would make a lot of sense to add this as a requirement for any struct passed via this mechanism. Because in a world where layout is reordered it doesn't make sense to pass anything else. In turn, I realised that this would mean atomic.Bool etc. don't work anymore, since they don't have a guaranteed layout. That's a shame but not without a work around, and I'm convinced that allowing sync.Mutex is a bad idea.

Playing that out in my head has me pretty convinced that we should adopt the same rules as package reflect or binary has:

• Only structs without unexported fields
• Containing structs.HostLayout

Maybe we can add special cases for atomic.Bool etc. on the basis that they are always the size of the basic element and therefore no reordering is possible?

[lmb]

I have a hunch that you'll have to allow uintptr because that is the natural way to represent a C pointer in Go. That only works as long as user space and kernel space are both 64 bit of course xD

[ti-mo]

Good point, we can allow uintptr if it's 8 bytes.

Reminder: we still treat btf.Pointer as 64 bits #963.