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.

Sign up for GitHub

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

Jump to bottom

Handle errno better and enoent better for batch operations #112

Closed
wants to merge 2 commits into from
Closed

Handle errno better and enoent better for batch operations #112

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d63dac8
Only check errno when error condition hit
derekparker Jan 11, 2022
acf2fa9
Still return collected values on batch operations for ENOENT
derekparker Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
130 changes: 70 additions & 60 deletions libbpfgo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ func NewModuleFromFileArgs(args NewModuleArgs) (*Module, error) {
}

obj, errno := C.bpf_object__open_file(bpfFile, &opts)
if errno != nil {
if obj == nil && errno != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is checking errno not already enough to make sure there's an error? It seems that Go does clear errno before each C function call.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to the comment of @grantseltzer
libbpf might return a non-null value in some cases.
For example: https://github.com/libbpf/libbpf/blob/master/src/libbpf.c#L7080.
In that case, we will mistakenly return that no error occured

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if Go does clear it before the call it's still possible that something within the call sets errno but we still don't actually have an error condition, the call that we originally made did succeed.

Without this change a few of the selftests error out. Also in this patch is a fix for map batch operations where errno could equal ENOENT. This happens when we've read an entire map, however we may still have gotten some data back so this patch fixes things to ensure whatever data we got is still returned, even if it's less than the batch amount.

return nil, fmt.Errorf("failed to open BPF object %s: %w", args.BPFObjPath, errno)
}

Expand Down Expand Up @@ -381,7 +381,7 @@ func NewModuleFromBufferArgs(args NewModuleArgs) (*Module, error) {
}

obj, errno := C.bpf_object__open_mem(bpfBuff, bpfBuffSize, &opts)
if errno != nil {
if obj == nil && errno != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

return nil, fmt.Errorf("failed to open BPF object %s: %w: errno", args.BPFObjName, args.BPFObjBuff[:20], errno)
}

Expand Down Expand Up @@ -418,8 +418,8 @@ func (m *Module) Close() {
}

func (m *Module) BPFLoadObject() error {
_, errno := C.bpf_object__load(m.obj)
if errno != nil {
ret, errno := C.bpf_object__load(m.obj)
if ret < 0 && errno != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good test, as bpf_object__load returns an int, and uses libbpf_err to set errno in case of an error:

static inline int libbpf_err(int ret)
{
	if (ret < 0)
		errno = -ret;
	return ret;
}

Same for all places that don't return a pointer

return fmt.Errorf("failed to load BPF object: %w", errno)
}

Expand All @@ -430,7 +430,7 @@ func (m *Module) GetMap(mapName string) (*BPFMap, error) {
cs := C.CString(mapName)
bpfMap, errno := C.bpf_object__find_map_by_name(m.obj, cs)
C.free(unsafe.Pointer(cs))
if errno != nil {
if bpfMap == nil && errno != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is ok to check for null bpfMap:
https://github.com/libbpf/libbpf/blob/master/src/libbpf.c#L9394

This inconsistency in libbpf is really confusing. I think it should be fixed before 1.0

return nil, fmt.Errorf("failed to find BPF map %s: %w", mapName, errno)
}

Expand All @@ -444,29 +444,29 @@ func (m *Module) GetMap(mapName string) (*BPFMap, error) {

func (b *BPFMap) Pin(pinPath string) error {
path := C.CString(pinPath)
_, errno := C.bpf_map__pin(b.bpfMap, path)
ret, errno := C.bpf_map__pin(b.bpfMap, path)
C.free(unsafe.Pointer(path))
if errno != nil {
if ret < 0 && errno != nil {
return fmt.Errorf("failed to pin map %s to path %s: %w", b.name, pinPath, errno)
}
return nil
}

func (b *BPFMap) Unpin(pinPath string) error {
path := C.CString(pinPath)
_, errno := C.bpf_map__unpin(b.bpfMap, path)
ret, errno := C.bpf_map__unpin(b.bpfMap, path)
C.free(unsafe.Pointer(path))
if errno != nil {
if ret < 0 && errno != nil {
return fmt.Errorf("failed to unpin map %s from path %s: %w", b.name, pinPath, errno)
}
return nil
}

func (b *BPFMap) SetPinPath(pinPath string) error {
path := C.CString(pinPath)
_, errno := C.bpf_map__set_pin_path(b.bpfMap, path)
ret, errno := C.bpf_map__set_pin_path(b.bpfMap, path)
C.free(unsafe.Pointer(path))
if errno != nil {
if ret < 0 && errno != nil {
return fmt.Errorf("failed to set pin for map %s to path %s: %w", b.name, pinPath, errno)
}
return nil
Expand All @@ -478,8 +478,8 @@ func (b *BPFMap) SetPinPath(pinPath string) error {
// Note: for ring buffer and perf buffer, maxEntries is the
// capacity in bytes.
func (b *BPFMap) Resize(maxEntries uint32) error {
_, errno := C.bpf_map__set_max_entries(b.bpfMap, C.uint(maxEntries))
if errno != nil {
ret, errno := C.bpf_map__set_max_entries(b.bpfMap, C.uint(maxEntries))
if ret < 0 && errno != nil {
return fmt.Errorf("failed to resize map %s to %v: %w", b.name, maxEntries, errno)
}
return nil
Expand Down Expand Up @@ -562,8 +562,8 @@ func (b *BPFMap) GetValue(key unsafe.Pointer) ([]byte, error) {
value := make([]byte, b.ValueSize())
valuePtr := unsafe.Pointer(&value[0])

_, errno := C.bpf_map_lookup_elem(b.fd, key, valuePtr)
if errno != nil {
ret, errno := C.bpf_map_lookup_elem(b.fd, key, valuePtr)
if ret < 0 && errno != nil {
return nil, fmt.Errorf("failed to lookup value %v in map %s: %w", key, b.name, errno)
}
return value, nil
Expand Down Expand Up @@ -608,14 +608,19 @@ func (b *BPFMap) GetValueBatch(keys unsafe.Pointer, startKey, nextKey unsafe.Poi
Flags: C.BPF_ANY,
}

_, errno := C.bpf_map_lookup_batch(b.fd, startKey, nextKey, keys, valuesPtr, &countC, bpfMapBatchOptsToC(opts))
if errno != nil {
return nil, fmt.Errorf("failed to batch lookup values %v in map %s: %w", keys, b.name, errno)
var reterr error
ret, errno := C.bpf_map_lookup_batch(b.fd, startKey, nextKey, keys, valuesPtr, &countC, bpfMapBatchOptsToC(opts))
if ret < 0 && errno != nil {
if errors.Is(errno, syscall.ENOENT) {
reterr = errno
} else {
return nil, fmt.Errorf("failed to batch lookup values %v in map %s: %w", keys, b.name, errno)
}
}

parsedVals := collectBatchValues(values, count, b.ValueSize())

return parsedVals, nil
return parsedVals, reterr
}

// GetValueAndDeleteBatch allows for batch lookups of multiple keys and deletes those keys.
Expand All @@ -632,14 +637,19 @@ func (b *BPFMap) GetValueAndDeleteBatch(keys, startKey, nextKey unsafe.Pointer,
Flags: C.BPF_ANY,
}

_, errno := C.bpf_map_lookup_and_delete_batch(b.fd, startKey, nextKey, keys, valuesPtr, &countC, bpfMapBatchOptsToC(opts))
if errno != nil {
return nil, fmt.Errorf("failed to batch lookup and delete values %v in map %s: %w", keys, b.name, errno)
var reterr error
ret, errno := C.bpf_map_lookup_and_delete_batch(b.fd, startKey, nextKey, keys, valuesPtr, &countC, bpfMapBatchOptsToC(opts))
if ret < 0 && errno != nil {
if errors.Is(errno, syscall.ENOENT) {
reterr = errno
} else {
return nil, fmt.Errorf("failed to batch lookup and delete values %v in map %s: %w", keys, b.name, errno)
}
}

parsedVals := collectBatchValues(values, count, b.ValueSize())

return parsedVals, nil
return parsedVals, reterr
}

func collectBatchValues(values []byte, count uint32, valueSize int) [][]byte {
Expand All @@ -660,8 +670,8 @@ func (b *BPFMap) UpdateBatch(keys, values unsafe.Pointer, count uint32) error {
ElemFlags: C.BPF_ANY,
Flags: C.BPF_ANY,
}
_, errno := C.bpf_map_update_batch(b.fd, keys, values, &countC, bpfMapBatchOptsToC(&opts))
if errno != nil {
ret, errno := C.bpf_map_update_batch(b.fd, keys, values, &countC, bpfMapBatchOptsToC(&opts))
if ret < 0 && errno != nil {
return fmt.Errorf("failed to update map %s: %w", b.name, errno)
}
return nil
Expand All @@ -676,8 +686,8 @@ func (b *BPFMap) DeleteKeyBatch(keys unsafe.Pointer, count uint32) error {
ElemFlags: C.BPF_ANY,
Flags: C.BPF_ANY,
}
_, errno := C.bpf_map_delete_batch(b.fd, keys, &countC, bpfMapBatchOptsToC(opts))
if errno != nil {
ret, errno := C.bpf_map_delete_batch(b.fd, keys, &countC, bpfMapBatchOptsToC(opts))
if ret < 0 && errno != nil {
return fmt.Errorf("failed to get lookup key %d from map %s: %w", keys, b.name, errno)
}
return nil
Expand All @@ -692,8 +702,8 @@ func (b *BPFMap) DeleteKeyBatch(keys unsafe.Pointer, count uint32) error {
// in the slice or array instead of the slice/array itself, as to
// avoid undefined behavior.
func (b *BPFMap) DeleteKey(key unsafe.Pointer) error {
_, errno := C.bpf_map_delete_elem(b.fd, key)
if errno != nil {
ret, errno := C.bpf_map_delete_elem(b.fd, key)
if ret < 0 && errno != nil {
return fmt.Errorf("failed to get lookup key %d from map %s: %w", key, b.name, errno)
}
return nil
Expand All @@ -715,8 +725,8 @@ func (b *BPFMap) DeleteKey(key unsafe.Pointer) error {
// bpfmap.Update(keyPtr, valuePtr)
//
func (b *BPFMap) Update(key, value unsafe.Pointer) error {
_, errno := C.bpf_map_update_elem(b.fd, key, value, C.BPF_ANY)
if errno != nil {
ret, errno := C.bpf_map_update_elem(b.fd, key, value, C.BPF_ANY)
if ret < 0 && errno != nil {
return fmt.Errorf("failed to update map %s: %w", b.name, errno)
}
return nil
Expand Down Expand Up @@ -750,8 +760,8 @@ func (it *BPFMapIterator) Next() bool {
next := make([]byte, it.b.KeySize())
nextPtr := unsafe.Pointer(&next[0])

_, errno := C.bpf_map_get_next_key(it.b.fd, prevPtr, nextPtr)
if errno != nil {
ret, errno := C.bpf_map_get_next_key(it.b.fd, prevPtr, nextPtr)
if ret < 0 && errno != nil {
if errors.Is(errno, syscall.ENOENT) {
it.err = errno
}
Expand All @@ -778,7 +788,7 @@ func (m *Module) GetProgram(progName string) (*BPFProg, error) {
cs := C.CString(progName)
prog, errno := C.bpf_object__find_program_by_name(m.obj, cs)
C.free(unsafe.Pointer(cs))
if errno != nil {
if prog == nil && errno != nil {
return nil, fmt.Errorf("failed to find BPF program %s: %w", progName, errno)
}

Expand All @@ -801,9 +811,9 @@ func (p *BPFProg) Pin(path string) error {
}

cs := C.CString(absPath)
_, errno := C.bpf_program__pin(p.prog, cs)
ret, errno := C.bpf_program__pin(p.prog, cs)
C.free(unsafe.Pointer(cs))
if errno != nil {
if ret < 0 && errno != nil {
return fmt.Errorf("failed to pin program %s to %s: %w", p.name, path, errno)
}
p.pinnedPath = absPath
Expand All @@ -812,9 +822,9 @@ func (p *BPFProg) Pin(path string) error {

func (p *BPFProg) Unpin(path string) error {
cs := C.CString(path)
_, errno := C.bpf_program__unpin(p.prog, cs)
ret, errno := C.bpf_program__unpin(p.prog, cs)
C.free(unsafe.Pointer(cs))
if errno != nil {
if ret < 0 && errno != nil {
return fmt.Errorf("failed to unpin program %s to %s: %w", p.name, path, errno)
}
p.pinnedPath = ""
Expand Down Expand Up @@ -876,16 +886,16 @@ func (p *BPFProg) GetType() uint32 {

func (p *BPFProg) SetAutoload(autoload bool) error {
cbool := C.bool(autoload)
_, errno := C.bpf_program__set_autoload(p.prog, cbool)
if errno != nil {
ret, errno := C.bpf_program__set_autoload(p.prog, cbool)
if ret < 0 && errno != nil {
return fmt.Errorf("failed to set bpf program autoload: %w", errno)
}
return nil
}

func (p *BPFProg) SetTracepoint() error {
_, errno := C.bpf_program__set_tracepoint(p.prog)
if errno != nil {
ret, errno := C.bpf_program__set_tracepoint(p.prog)
if ret < 0 && errno != nil {
return fmt.Errorf("failed to set bpf program as tracepoint: %w", errno)
}
return nil
Expand All @@ -897,7 +907,7 @@ func (p *BPFProg) AttachTracepoint(category, name string) (*BPFLink, error) {
link, errno := C.bpf_program__attach_tracepoint(p.prog, tpCategory, tpName)
C.free(unsafe.Pointer(tpCategory))
C.free(unsafe.Pointer(tpName))
if errno != nil {
if link == nil && errno != nil {
return nil, fmt.Errorf("failed to attach tracepoint %s to program %s: %w", name, p.name, errno)
}

Expand All @@ -915,7 +925,7 @@ func (p *BPFProg) AttachRawTracepoint(tpEvent string) (*BPFLink, error) {
cs := C.CString(tpEvent)
link, errno := C.bpf_program__attach_raw_tracepoint(p.prog, cs)
C.free(unsafe.Pointer(cs))
if errno != nil {
if link == nil && errno != nil {
return nil, fmt.Errorf("failed to attach raw tracepoint %s to program %s: %w", tpEvent, p.name, errno)
}

Expand All @@ -931,7 +941,7 @@ func (p *BPFProg) AttachRawTracepoint(tpEvent string) (*BPFLink, error) {

func (p *BPFProg) AttachPerfEvent(fd int) (*BPFLink, error) {
link, errno := C.bpf_program__attach_perf_event(p.prog, C.int(fd))
if errno != nil {
if link == nil && errno != nil {
return nil, fmt.Errorf("failed to attach perf event to program %s: %w", p.name, errno)
}

Expand All @@ -956,7 +966,7 @@ func (p *BPFProg) AttachKretprobe(kp string) (*BPFLink, error) {

func (p *BPFProg) AttachLSM() (*BPFLink, error) {
link, errno := C.bpf_program__attach_lsm(p.prog)
if errno != nil {
if link == nil && errno != nil {
return nil, fmt.Errorf("failed to attach lsm to program %s: %w", p.name, errno)
}

Expand All @@ -974,7 +984,7 @@ func doAttachKprobe(prog *BPFProg, kp string, isKretprobe bool) (*BPFLink, error
cbool := C.bool(isKretprobe)
link, errno := C.bpf_program__attach_kprobe(prog.prog, cbool, cs)
C.free(unsafe.Pointer(cs))
if errno != nil {
if link == nil && errno != nil {
return nil, fmt.Errorf("failed to attach %s k(ret)probe to program %s: %w", kp, prog.name, errno)
}

Expand Down Expand Up @@ -1026,7 +1036,7 @@ func doAttachUprobe(prog *BPFProg, isUretprobe bool, pid int, path string, offse

link, errno := C.bpf_program__attach_uprobe(prog.prog, retCBool, pidCint, pathCString, offsetCsizet)
C.free(unsafe.Pointer(pathCString))
if errno != nil {
if link == nil && errno != nil {
return nil, fmt.Errorf("failed to attach u(ret)probe to program %s:%d with pid %d: %w", path, offset, pid, errno)
}

Expand Down Expand Up @@ -1057,7 +1067,7 @@ func doAttachKprobeLegacy(prog *BPFProg, kp string, isKretprobe bool) (*BPFLink,
cbool := C.bool(isKretprobe)
link, errno := C.attach_kprobe_legacy(prog.prog, cs, cbool)
C.free(unsafe.Pointer(cs))
if errno != nil {
if link == nil && errno != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is implemented in libbpfgo - it is ok to check for nil

return nil, fmt.Errorf("failed to attach %s k(ret)probe using legacy debugfs API: %w", kp, errno)
}

Expand Down Expand Up @@ -1094,7 +1104,7 @@ func (m *Module) InitRingBuf(mapName string, eventsChan chan []byte) (*RingBuffe
}

rb, errno := C.init_ring_buf(bpfMap.fd, C.uintptr_t(slot))
if errno != nil {
if rb == nil && errno != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is implemented in libbpfgo - it is ok to check for nil

return nil, fmt.Errorf("failed to initialize ring buffer: %w", errno)
}

Expand Down Expand Up @@ -1370,17 +1380,17 @@ func (hook *TcHook) SetParent(a int, b int) {
}

func (hook *TcHook) Create() error {
_, errno := C.bpf_tc_hook_create(hook.hook)
if errno != nil {
ret, errno := C.bpf_tc_hook_create(hook.hook)
if ret < 0 && errno != nil {
return fmt.Errorf("could not create tc hook: %w", errno)
}

return nil
}

func (hook *TcHook) Destroy() error {
_, errno := C.bpf_tc_hook_destroy(hook.hook)
if errno != nil {
ret, errno := C.bpf_tc_hook_destroy(hook.hook)
if ret < 0 && errno != nil {
return fmt.Errorf("could not destroy tc hook: %w", errno)
}

Expand All @@ -1389,8 +1399,8 @@ func (hook *TcHook) Destroy() error {

func (hook *TcHook) Attach(tcOpts *TcOpts) error {
opts := tcOptsToC(tcOpts)
_, errno := C.bpf_tc_attach(hook.hook, opts)
if errno != nil {
ret, errno := C.bpf_tc_attach(hook.hook, opts)
if ret < 0 && errno != nil {
return fmt.Errorf("could not attach tc hook: %w", errno)
}
tcOptsFromC(tcOpts, opts)
Expand All @@ -1400,8 +1410,8 @@ func (hook *TcHook) Attach(tcOpts *TcOpts) error {

func (hook *TcHook) Detach(tcOpts *TcOpts) error {
opts := tcOptsToC(tcOpts)
_, errno := C.bpf_tc_detach(hook.hook, opts)
if errno != nil {
ret, errno := C.bpf_tc_detach(hook.hook, opts)
if ret < 0 && errno != nil {
return fmt.Errorf("could not detach tc hook: %w", errno)
}
tcOptsFromC(tcOpts, opts)
Expand All @@ -1411,8 +1421,8 @@ func (hook *TcHook) Detach(tcOpts *TcOpts) error {

func (hook *TcHook) Query(tcOpts *TcOpts) error {
opts := tcOptsToC(tcOpts)
_, errno := C.bpf_tc_query(hook.hook, opts)
if errno != nil {
ret, errno := C.bpf_tc_query(hook.hook, opts)
if ret < 0 && errno != nil {
return fmt.Errorf("could not query tc hook: %w", errno)
}
tcOptsFromC(tcOpts, opts)
Expand Down