sys: overhaul Pointer constructors #908
Conversation
Use generics to make the Pointer constructors more useful. Also remove the "New" prefix from constructors that do not allocate. Introduce SliceLen and SliceElems which return 0 if the length of a slice exceeds uint32. This will make it much more obvious when a buffer is too large to be passed to a syscall, since passing a pointer with zero length will cause EFAULT or similar. Signed-off-by: Lorenz Bauer <oss@lmb.io>
lmb
force-pushed
the
sys-generic-slice-pointer
branch
from
e28295b
to
6bc950a
Compare
| // SliceElems returns the number of equal sized elements in a slice. | ||
| // | ||
| // Returns zero if the number of elements exceeds uint32. | ||
| func SliceElems[E any](slice []E, elemSize int) uint32 { |
There was a problem hiding this comment.
I don't think this is a necessary addition; the majority of its uses are through SliceLen(), where elemSize is hardcoded to 1. elemSize > 1 only really makes sense if E is []byte, so doesn't really belong in a generic function. To me, this isn't more expressive than FuncInfoCnt = len(fib) / btf.FuncInfoSize. Maybe I'm too used to reading the existing code, but it's immediately clear what it does, the new code takes some digging around to be sure.
The overflow cutoff at maxu32 also feels somewhat arbitrarily tacked on without context around Func/LineInfoCnt and friends. If overflows are really a concern, newProgramWithOptions() should error instead of silently falling back to 0. EFAULT instead of EINVAL will still be a pain to debug.
If we want to avoid the many uint32(len(..., maybe declaring a method Elems(size int) uint32 over a []byte alias would make more sense? Or a proper new type sys.SlicePointer that wraps an unsafe.Pointer and a pointer to the initial slice for length calculations? Either way, as long as len() returns a signed int, we'll have to do conversions somewhere. :) A concrete type that expects its length to be represented in u32 would suit this better.
There was a problem hiding this comment.
elemSize > 1 only really makes sense if E is []byte, so doesn't really belong in a generic function.
If we want to avoid the many uint32(len(..., maybe declaring a method Elems(size int) uint32 over a []byte alias would make more sense?
Fair point. What about SliceElems([]byte, int) instead then? I'm not sure what the alias type would do.
In a way, using []byte in a couple of places is kind of a kludge. The syscall really expects []bpfInstruction and []bpfFuncInfo, etc. We've just decided to represent those as []byte since we use binary.Write to marshal into the kernel representation. This is nice and safe, but in reality all the BPF UAPI types are manually laid out anyways, and we frequently just do unsafe.Pointer(&sys.ProgLoadAttr).
I'm not at all convinced by my idea, but maybe we should just stick to "handwritten" structs instead? MarshalExtInfos would return []bpfLineInfo, []bpfFuncInfo (naming to be bikeshedded, you get the idea) that we pass via SlicePointer. Those types need not have exported fields, which would keep them opaque from the users perspective. (Could also start generating func info and line info from BTF and stick them in sys?)
The upside is that we'd remove a bunch of allocations and reflection from commonly called code. The downside is that it seems like a big change just to get rid of SliceElems :D
The overflow cutoff at maxu32 also feels somewhat arbitrarily tacked on without context around Func/LineInfoCnt and friends. If overflows are really a concern, newProgramWithOptions() should error instead of silently falling back to 0. EFAULT instead of EINVAL will still be a pain to debug.
There are three options:
- Silently truncate as we do right now. The failure mode is that some calls will probably go through, other calls will fail in mysterious ways (thinking of func info / line info as you mentioned).
- Silently use 0, as proposed by the PR. Calls fail reliably, but with a weird error.
- Return an error, as proposed by you. Calls fail with a nice error.
I think the likelihood of someone running into this error is low, so I'm reluctant to go for (3) since it clutters call sites. Out of the remaining options 2 seems better than 1 to me.
There was a problem hiding this comment.
We've just decided to represent those as []byte since we use binary.Write to marshal into the kernel representation.
This decision probably stems from reading files through io.Readers. Aren't C vs. Go struct alignment rules a potential show stopper here?
The upside is that we'd remove a bunch of allocations and reflection from commonly called code.
(fyi, there's no such thing as bpfInstruction; asm.Instruction marshals directly to and from a binary stream)
For asm.Instruction specifically (our largest object pool by far), we could interpret a chunk of bytes as a hypothetical []bpfInstruction for a speed gain, but a copy is still needed into asm.Instruction. Whether we copy from []byte or []bpfInstruction makes no difference in that scenario. Also, instructions are read from files (thus io.Readers) in the majority of cases, not from existing memory.
The downside is that it seems like a big change just to get rid of SliceElems
Could be worth it for clarity and overall maintainability. If we can generate types from uapi, that might be preferred over hand-written marshalers. Whether the output is []byte or []bpfFoo, allocs need to be made regardless, so performance would be equal to []byte at best.
Out of the remaining options 2 seems better than 1 to me.
Sure, guess it doesn't matter that much.
There was a problem hiding this comment.
Aren't C vs. Go struct alignment rules a potential show stopper here?
They kind of are, but we already have that problem with structs in sys. I think if we use gentypes and add some additional tests we should be OK.
Whether we copy from []byte or []bpfInstruction makes no difference in that scenario.
I'm specifically talking about Instructions.Marshal: instead of building []byte we'd assemble []sys.Instruction (or whatever you want to call it). That avoids reflection and / or manually writing the marshaler.
| "unsafe" | ||
|
|
||
| "github.com/cilium/ebpf/internal/unix" | ||
| ) | ||
|
|
||
| // NewPointer creates a 64-bit pointer from an unsafe Pointer. | ||
| func NewPointer(ptr unsafe.Pointer) Pointer { | ||
| // UnsafePointer creates a 64-bit pointer from an unsafe Pointer. |
There was a problem hiding this comment.
This makes some cases stutter (sys.UnsafePointer(unsafe.Pointer(&info))) and makes for longer lines overall. It still allocates, just not necessarily on the heap. 😉 I think NewPointer was just fine, it still returns 'a new Pointer'.
There was a problem hiding this comment.
True about the stuttering! Pointer is a value type, so in my mind that means it doesn't allocate on it's own. Contrast that with NewSlicePointer, etc.
| func NewSlicePointerLen(buf []byte) (Pointer, uint32) { | ||
| return NewSlicePointer(buf), uint32(len(buf)) | ||
| // Returns zero if the length of the slice exceeds uint32. | ||
| func SliceLen[E any](slice []E) uint32 { |
There was a problem hiding this comment.
This is only really useful if E is []byte.
There was a problem hiding this comment.
Maybe you meant SliceElems? This is useful with other types, check QueryPrograms in query.go that calls it with []ProgramID.
|
See #914 instead. |
Use generics to make the Pointer constructors more useful. Also remove the "New" prefix from constructors that do not allocate.
Introduce SliceLen and SliceElems which return 0 if the length of a slice exceeds uint32. This will make it much more obvious when a buffer is too large to be passed to a syscall, since passing a pointer with zero length will cause EFAULT or similar.
Signed-off-by: Lorenz Bauer oss@lmb.io