Standardize error checking across libbpfgo #100

grantseltzer · 2021-12-16T19:37:45Z

This handles correcting how we check errors in libbpfgo
in accordance with how libbpf recommends handling errors
leading into libbpf.

Particularly this follows the following rules:

APIs that return an error code (int) should have
error codes checked directly. The error codes
correspond with error codes in the syscall package.

For example:
```
errCodeInt := C.libbpf_api_function()
if errCodeInt != 0 {
  log.Errorf("uh oh: %s\n", syscall.Errno(errCodeInt))
}
```
APIs that return a pointer should be checked for
NULL which indicates error. The error code is
stored in errno. We can get the value of errno
using the second return.

For example:
```
ptr, errno := C.libbpf_api_function()
if ptr == nil {
  log.Errorf("uh oh: %s\n", errno)
}
```
We can also check if errno corresponds with a specific
error (it implements the standard error interface).

For example:
```
ptr, errno := C.libbpf_api_function()
if ptr == nil {
  if errno.Is(syscall.ENODENT) {
     // handle accordingly
  } else {
    log.Errorf("uh oh: %s\n", errno)
  }
}
```

Signed-off-by: grantseltzer grantseltzer@gmail.com

rafaeldtinoco · 2021-12-17T18:07:17Z

libbpfgo.go

-		return fmt.Errorf("failed to load BPF object")
+	cErr := C.bpf_object__load(m.obj)
+	if cErr != 0 {
+		return fmt.Errorf("failed to load BPF object", syscall.Errno(int(-cErr)))


Is this right ? Follow my thoughts:

You call bpf_object__load(NULL)... then you will face:

if (!attr) return libbpf_err(-EINVAL);

and libbpf_err(-EINVAL)::

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

So it will return a negative number, but errno will still be positive on the error return. Why do you need to set it to negative again ? And also it is supposed to be an unsigned integer:

type Errno uintptr

and all general errnos are positive as well:

// Errors const ( E2BIG = Errno(0x7) EACCES = Errno(0xd) EADDRINUSE = Errno(0x62) EADDRNOTAVAIL = Errno(0x63) EADV = Errno(0x44) EAFNOSUPPORT = Errno(0x61) EAGAIN = Errno(0xb) EALREADY = Errno(0x72) EBADE = Errno(0x34) EBADF = Errno(0x9) EBADFD = Errno(0x4d) EBADMSG = Errno(0x4a) EBADR = Errno(0x35) EBADRQC = Errno(0x38) EBADSLT = Errno(0x39) EBFONT = Errno(0x3b) EBUSY = Errno(0x10) ECANCELED = Errno(0x7d) ECHILD = Errno(0xa) ECHRNG = Errno(0x2c) ECOMM = Errno(0x46) ECONNABORTED = Errno(0x67) ECONNREFUSED = Errno(0x6f) ECONNRESET = Errno(0x68) EDEADLK = Errno(0x23) ...

Also, shouldn't you have a %s to stringify the errno ? Like shown at:

func (e Errno) Error() string { if 0 <= int(e) && int(e) < len(errors) { s := errors[e] if s != "" { return s } } return "errno " + itoa.Itoa(int(e)) }

Was that your intent here ?

There are other places doing the same thing. I'll clarify this first before moving on the review (as I'd like to understand the reasoning of that).

Ok you are correct, I didn't mean to have negatives there, that was an inconsistency and mistake :-)

Also yes, %s should be used since the error interface implements stringer. Just missing the %s here.

grantseltzer · 2021-12-17T19:20:45Z

Relevant mailing list question: https://lore.kernel.org/bpf/CAO658oXGs3R=mQZo=aOEvRgG6O-obFbwR54V+QSn36uecF0+=w@mail.gmail.com/

derekparker · 2021-12-22T17:17:58Z

Hey @grantseltzer I think there's some inconsistencies with the error handling in libbpf (at least that's what I'm observing).

If you check the other PR I submitted (#101) you can see errC == -1 however errno == ENOENT (which is 2, not 1). libbpf seems to use ENOENT to indicate reading from an empty map, so it's valuable to have that information from the callers perspective to know it wasn't a "real" error, just an empty map with nothing to be read (at least that's how I understand it).

derekparker · 2021-12-22T17:37:08Z

Also I believe some of Andrii's comments to be mistaken in his review on your documentation PR: https://patchwork.kernel.org/project/netdevbpf/patch/20211220054048.54845-1-grantseltzer@gmail.com/.

He claims that certain buffers for input buffers, such as the keys buffer argument to lookup_and_delete_batch or even just delete_batch but that's not the case. Your original documentation is correct in that the buffer is filled in by the call to the bpf batch operation, not used as an input set of keys (if that makes sense). This can be verified by looking at the kernel tests themselves: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c

grantseltzer · 2021-12-23T21:39:22Z

@derekparker Are you saying that keys is an output parameter for bpf_map_delete_batch()? I don't think that's the case. I do think it's the case for bpf_map_lookup_and_delete_batch() though.

derekparker · 2022-01-01T20:13:52Z

@derekparker Are you saying that keys is an output parameter for bpf_map_delete_batch()? I don't think that's the case. I do think it's the case for bpf_map_lookup_and_delete_batch() though.

Ack my mistake I think you're right.

derekparker · 2022-01-03T16:20:02Z

@grantseltzer in my opinion, based on what I've seen during implementation (return value differing from errno in some cases) I think what this library should do is return the errno error wrapped in another error, so that callers can use patterns such as:

err := libbpfgo.SomeFunction()
if errors.Is(err, os.ErrNotExist) {
        // ...
}

That would mean in the implementation we would use a pattern of:

ret, errno := C.bpf_do_some_stuff()
if errno != syscall.Errno(0) {
        return fmt.Errorf("unable to do some bpf stuff: %w")
}

Note the %w to explicitly wrap the error instead of shadowing it.

Thoughts? I could test this change locally against my codebase but I'm pretty sure this would be sufficient and would standardize errors.

grantseltzer · 2022-01-03T17:36:31Z

@derekparker I think that's a good idea, makes a lot of sense. I believe the inconsistencies between returned error and errno values is going to be solved by the time libbpf 1.0 is released, but i'm not entirely sure. Will verify and report back, getting back into the swing of things after vacation at the moment! I hope you had a nice holiday!

derekparker · 2022-01-03T17:46:38Z

@derekparker I think that's a good idea, makes a lot of sense. I believe the inconsistencies between returned error and errno values is going to be solved by the time libbpf 1.0 is released, but i'm not entirely sure. Will verify and report back, getting back into the swing of things after vacation at the moment! I hope you had a nice holiday!

Ack, thanks for the response! And likewise, still getting my head back in the game, haha. Hope you had a nice holiday as well!

derekparker · 2022-01-07T19:14:13Z

libbpfgo.go

@@ -672,7 +661,7 @@ func (b *BPFMap) UpdateBatch(keys, values unsafe.Pointer, count uint32) error {
 	}
 	errC := C.bpf_map_update_batch(b.fd, keys, values, &countC, bpfMapBatchOptsToC(&opts))
 	if errC != 0 {
-		return fmt.Errorf("failed to update map %s: %v", b.name, errC)
+		return fmt.Errorf("failed to update map %s: %w", b.name, syscall.Errno(int(errC)))


I think we should wrap the errno value directly, due to inconsistencies with the returned value and errno in libbpf.

Fair enough! I investigated and it looks like not every API function is directly moved over to directly returning error codes yet. Errno is better to rely on for now.

derekparker · 2022-01-07T19:14:54Z

Only one comment otherwise this LGTM. I just think we should explicitly check errno and wrap that to not have to deal with inconsistencies between ret value and errno value.

This handles correcting how we check errors in libbpfgo in accordance with how libbpf recommends handling errors leading into libbpf. Particularly this follows the following rules: - APIs that return an error code (int) should have error codes checked directly. The error codes correspond with error codes in the syscall package. For example: ``` errCodeInt := C.libbpf_api_function() if errCodeInt != 0 { log.Errorf("uh oh: %s\n", syscall.Errno(errCodeInt)) } ``` - APIs that return a pointer should be checked for NULL which indicates error. The error code is stored in errno. We can get the value of errno using the second return. For example: ``` ptr, errno := C.libbpf_api_function() if ptr == nil { log.Errorf("uh oh: %s\n", errno) } ``` We can also check if errno corresponds with a specific error (it implements the standard error interface). For example: ``` ptr, errno := C.libbpf_api_function() if ptr == nil { if errno.Is(syscall.ENODENT) { // handle accordingly } else { log.Errorf("uh oh: %s\n", errno) } } ``` Signed-off-by: grantseltzer <grantseltzer@gmail.com>

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

derekparker

LGTM

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

rafaeldtinoco

LGTM

yanivagman · 2022-01-11T14:44:20Z

libbpfgo.go

-	if ret < 0 {
-		return syscall.Errno(-ret)
+	if ret != 0 {
+		return syscall.Errno(ret)


syscall.Errno expect a positive value to get the correct error - are you sure it is correct to remove the negative sign? (also for all of the above)

Errors are returned negative. See https://github.com/libbpf/libbpf/blob/e99f34e14447866bcbc38944387531c36f6c85fe/src/libbpf_internal.h#L454

And https://github.com/libbpf/libbpf/blob/21f90f61b0849ae654b7c78ba9ce34bfb74ce6f2/src/netlink.c#L756

The discussion about this was made at:

#100 (comment)

I wonder if I missed something there.

You call bpf_object__load(NULL)... then you will face:

if (!attr) return libbpf_err(-EINVAL);

and libbpf_err(-EINVAL)::

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

So it will return a negative number, but errno will still be positive on the error return. Why do you need to set it to negative again ?

Like you wrote here, the return is a negative number, meaning ret is negative. To have a positive value, we should syscall.Errno(-ret)

yanivagman · 2022-01-11T15:11:29Z

libbpfgo.go

-	if C.IS_ERR_OR_NULL(unsafe.Pointer(obj)) {
-		return nil, errptrError(unsafe.Pointer(obj), "failed to open BPF object %s", args.BPFObjPath)
+	obj, errno := C.bpf_object__open_file(bpfFile, &opts)
+	if obj == nil {


This will only be null if LIBBPF_STRICT_CLEAN_PTRS is set.
https://github.com/libbpf/libbpf/blob/e99f34e14447866bcbc38944387531c36f6c85fe/src/libbpf_internal.h#L476
Is that the case?

This reverts commit 088c639.

grantseltzer requested review from yanivagman and rafaeldtinoco December 17, 2021 13:25

rafaeldtinoco reviewed Dec 17, 2021

View reviewed changes

grantseltzer mentioned this pull request Dec 22, 2021

libbpfgo: Improve error message in batch operations #101

Closed

derekparker mentioned this pull request Jan 3, 2022

Use batch operations parca-dev/parca-agent#74

Closed

derekparker reviewed Jan 7, 2022

View reviewed changes

grantseltzer added 4 commits January 10, 2022 14:30

fix -errno usage, add missing string format verb

4b716fe

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

Use %w verb for error wrapping

d353c16

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

Check errno instead of directly returned error for now

ca51024

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

grantseltzer force-pushed the standardize-error-handling branch from fca71b7 to ca51024 Compare January 10, 2022 19:30

derekparker previously approved these changes Jan 10, 2022

View reviewed changes

Fix missing C package qualifiers and incorrect usage of errors.Is

65aa9a9

Signed-off-by: grantseltzer <grantseltzer@gmail.com>

grantseltzer dismissed derekparker’s stale review via 65aa9a9 January 10, 2022 21:53

rafaeldtinoco approved these changes Jan 10, 2022

View reviewed changes

grantseltzer merged commit 088c639 into aquasecurity:main Jan 10, 2022

yanivagman reviewed Jan 11, 2022

View reviewed changes

This was referenced Jan 20, 2022

Downgrade libbpf-go parca-dev/parca-agent#221

Merged

Map iterator returns "no such file or directory" unexpectedly #114

Closed

rafaeldtinoco added a commit that referenced this pull request Jan 21, 2022

Revert "Standardize error checking across libbpfgo (#100)"

0942b81

This reverts commit 088c639.

javierhonduco mentioned this pull request Apr 28, 2022

Error handling improvements #159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize error checking across libbpfgo #100

Standardize error checking across libbpfgo #100

grantseltzer commented Dec 16, 2021

rafaeldtinoco Dec 17, 2021

rafaeldtinoco Dec 17, 2021

rafaeldtinoco Dec 17, 2021

grantseltzer Dec 17, 2021

grantseltzer commented Dec 17, 2021

derekparker commented Dec 22, 2021

derekparker commented Dec 22, 2021

grantseltzer commented Dec 23, 2021

derekparker commented Jan 1, 2022

derekparker commented Jan 3, 2022

grantseltzer commented Jan 3, 2022

derekparker commented Jan 3, 2022 •

edited

derekparker Jan 7, 2022

grantseltzer Jan 10, 2022

derekparker commented Jan 7, 2022

derekparker left a comment

rafaeldtinoco left a comment

yanivagman Jan 11, 2022

yanivagman Jan 11, 2022

rafaeldtinoco Jan 11, 2022

yanivagman Jan 13, 2022 •

edited

yanivagman Jan 11, 2022

Standardize error checking across libbpfgo #100

Standardize error checking across libbpfgo #100

Conversation

grantseltzer commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantseltzer commented Dec 17, 2021

derekparker commented Dec 22, 2021

derekparker commented Dec 22, 2021

grantseltzer commented Dec 23, 2021

derekparker commented Jan 1, 2022

derekparker commented Jan 3, 2022

grantseltzer commented Jan 3, 2022

derekparker commented Jan 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekparker commented Jan 7, 2022

derekparker left a comment

Choose a reason for hiding this comment

rafaeldtinoco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanivagman Jan 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekparker commented Jan 3, 2022 •

edited

yanivagman Jan 13, 2022 •

edited