Add workaround to CGO bug

[grantseltzer]

This fixes a bug we're seeing in Tracee where field offsets are mangled over c/go threshold. The bug behavior is such that the kconfig field overwrites the btf_custom_path field.

It started occurring in libbpf commit d8454ba8 and I believe the origin is adding long :0 which i'm not entirely sure the purpose of (something to do with aligning to certain size boundary within the struct).

This has no consequences on the logic of the code so should be a fine workaround until we figure out a solution upstream.

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

[geyslan]

It started occurring in libbpf commit d8454ba8 and I believe the origin is adding long :0 which i'm not entirely sure the purpose of (something to do with aligning to certain size boundary within the struct).

@grantseltzer that's interesting indeed. It's a zeroed bitfield.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf

I wonder if this solution could being changing the writing time which would avoid overwriting fields.

libbpf field order:

	size_t sz;	
	const char *object_name;
...
	long :0;
	const char *kconfig;
	const char *btf_custom_path;

Old writing order:

	opts.object_name = bpfName
	opts.sz = C.sizeof_struct_bpf_object_open_opts
	opts.btf_custom_path = btfFile
	opts.kconfig = kConfigFile

Proposed writing order:

	opts.kconfig = kConfigFile
	opts.object_name = bpfName
	opts.sz = C.sizeof_struct_bpf_object_open_opts
	opts.btf_custom_path = btfFile

[rafaeldtinoco]

It is a cache line alignment / padding optimization using an empty bitfield (AND VERY LIKELY may vary among different compilers and versions).

It is an automatic way to add padding to the next cacheline boundary WITHOUT having to specify a padding variable (and knowing its size for the alignment).

[rafaeldtinoco]

It is likely that the pointer given by CGO to C is unaligned to what libbpf expects because CGO didn't do the optimization when compiling. You can pick different compilers for CGO afaik, meaning that the issue might still be there in some cases (even with this proposed change).

Maybe we can simply calculate the offset correctly and always align with WORD size... we can have our own structure instead of allowing go to generate what it thinks is the structure:

And then we can add the intermediate (before btf_custom_path) padding "by hand".

[rafaeldtinoco]

Alright, after trying to manipulate a packed structure (with padding) using CGO I saw CGO simply ignores packing and brakes all padding (probably the origin of all this) so… Instead, we can use a wrapper struct and feed the calls with C only code (having the correct padding). Example:

diff --git a/libbpfgo.go b/libbpfgo.go
index 9d4fc39..692ab3d 100644
--- a/libbpfgo.go
+++ b/libbpfgo.go
@@ -113,6 +113,51 @@ int bpf_prog_detach_cgroup_legacy(
 
        return syscall(__NR_bpf, BPF_PROG_DETACH, &attr, sizeof(attr));
 }
+
+struct packed_bpf_object_open_opts {
+       size_t sz;
+       char *object_name;
+       unsigned char pad01[16];
+       char *kconfig;
+       char *btf_custom_path;
+       unsigned char pad02[24];
+} __attribute__ ((__packed__));
+
+struct custom_bpf_object_open_opts {
+       char *object_name;
+       char *kconfig;
+       char *btf_custom_path;
+};
+
+struct bpf_object *
+custom_bpf_object__open_file(
+       char *path,
+       struct custom_bpf_object_open_opts *opts)
+{
+       struct packed_bpf_object_open_opts packed = {0};
+
+       packed.kconfig = opts->kconfig;
+       packed.btf_custom_path = opts->btf_custom_path;
+       packed.sz = sizeof(struct packed_bpf_object_open_opts);
+
+       return bpf_object__open_file(path, (struct bpf_object_open_opts *) &packed);
+}
+
+struct bpf_object *
+custom_bpf_object__open_mem(
+       const void *obj_buf,
+       size_t obj_buf_sz,
+       struct custom_bpf_object_open_opts *opts)
+{
+       struct packed_bpf_object_open_opts packed = {0};
+
+       packed.object_name = opts->object_name;
+       packed.kconfig = opts->kconfig;
+       packed.btf_custom_path = opts->btf_custom_path;
+       packed.sz = sizeof(struct packed_bpf_object_open_opts);
+
+       return bpf_object__open_mem(obj_buf, obj_buf_sz, (struct bpf_object_open_opts *) &packed);
+}
 */
 import "C"
 
@@ -409,8 +454,7 @@ func SetStrictMode(mode LibbpfStrictMode) {
 func NewModuleFromFileArgs(args NewModuleArgs) (*Module, error) {
        C.set_print_fn()

-       opts := C.struct_bpf_object_open_opts{}
-       opts.sz = C.sizeof_struct_bpf_object_open_opts
+       opts := C.struct_custom_bpf_object_open_opts{}

        bpfFile := C.CString(args.BPFObjPath)
        defer C.free(unsafe.Pointer(bpfFile))
@@ -429,7 +473,7 @@ func NewModuleFromFileArgs(args NewModuleArgs) (*Module, error) {
                defer C.free(unsafe.Pointer(kConfigFile))
        }

-       obj, errno := C.bpf_object__open_file(bpfFile, &opts)
+       obj, errno := C.custom_bpf_object__open_file(bpfFile, &opts)
        if obj == nil {
                return nil, fmt.Errorf("failed to open BPF object at path %s: %w", args.BPFObjPath, errno)
        }
@@ -458,9 +502,8 @@ func NewModuleFromBufferArgs(args NewModuleArgs) (*Module, error) {
        bpfBuff := unsafe.Pointer(C.CBytes(args.BPFObjBuff))
        bpfBuffSize := C.size_t(len(args.BPFObjBuff))

-       opts := C.struct_bpf_object_open_opts{}
+       opts := C.struct_custom_bpf_object_open_opts{}
        opts.object_name = bpfName
-       opts.sz = C.sizeof_struct_bpf_object_open_opts
        opts.btf_custom_path = btfFile // instruct libbpf to use user provided kernel BTF file

        if len(args.KConfigFilePath) > 2 {
@@ -469,7 +512,7 @@ func NewModuleFromBufferArgs(args NewModuleArgs) (*Module, error) {
                defer C.free(unsafe.Pointer(kConfigFile))
        }

-       obj, errno := C.bpf_object__open_mem(bpfBuff, bpfBuffSize, &opts)
+       obj, errno := C.custom_bpf_object__open_mem(bpfBuff, bpfBuffSize, &opts)
        if obj == nil {
                return nil, fmt.Errorf("failed to open BPF object %s: %w", args.BPFObjName, errno)
        }

You can try changing the "pad01" and "pad02" sizes, and you will see the pointer being broken. @grantseltzer if you have a reproducer, could you test this ? I don't have, so it's all in theory for now...

[rafaeldtinoco]

I even suspect that, following my patch, we can even get rid of:

struct packed_bpf_object_open_opts {
       size_t sz;
       char *object_name;
       unsigned char pad01[16];
       char *kconfig;
       char *btf_custom_path;
       unsigned char pad02[24];
} __attribute__ ((__packed__));

Structure and use the regular bpf_object_open_opts struct declared in the C function only, instead of using it through CGO (then the padding/optimizations won't be ignored and CGO will use a wrapper struct for its logic).

[rafaeldtinoco]

I believe the correct workaround is something else.

We should wrap the struct with optimizations in a C function and deal with the needed structure without using CGO (so golang does not have to deal with padding/optimizations).

[geyslan]

It is a cache line alignment / padding optimization using an empty bitfield (AND VERY LIKELY may vary among different compilers and versions).

For sure, it's implementation-defined.

It is an automatic way to add padding to the next cacheline boundary WITHOUT having to specify a padding variable (and knowing its size for the alignment).

I would call it a not recommended HACKY way if portability is a goal.

Section 6.7.2.1:

An implementation may allocate any addressable storage unit large enough to hold a bit-field. If enough space remains, a bit-field that immediately follows another bit-field in a structure shall be packed into adjacent bits of the same unit. If insufficient space remains, whether a bit-field that does not fit is put into the next unit or overlaps adjacent units is implementation-defined. The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined. The alignment of the addressable storage unit is unspecified.

Section 6.7.2.1:

Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared.

I know that this one is not the same case but just making note: https://wiki.sei.cmu.edu/confluence/display/c/EXP11-C.+Do+not+make+assumptions+regarding+the+layout+of+structures+with+bit-fields

We should wrap the struct with optimizations in a C function and deal with the needed structure without using CGO (so golang does not have to deal with padding/optimizations).

And not forgetting to deal with different compilers...

[geyslan]

Beware of these other bit-fields (the last two in special - perf_buffer_raw_opts):

grep -n -r ":0;" ./code/libbpfgo/libbpf/src
./code/libbpfgo/libbpf/src/btf.h:228:	size_t :0;
./code/libbpfgo/libbpf/src/btf.h:269:	size_t :0;
./code/libbpfgo/libbpf/src/btf.h:287:	size_t :0;
./code/libbpfgo/libbpf/src/bpf.h:278:	size_t :0;
./code/libbpfgo/libbpf/src/bpf.h:325:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:121:	long :0;
./code/libbpfgo/libbpf/src/libbpf.h:171:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:427:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:452:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:469:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:527:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:576:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:940:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:947:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:958:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:988:	size_t :0;
./code/libbpfgo/libbpf/src/libbpf.h:999:	size_t :0;

./code/libbpfgo/libbpf/src/libbpf.h:1077:	long :0;
./code/libbpfgo/libbpf/src/libbpf.h:1078:	long :0;

struct perf_buffer_raw_opts {
	size_t sz;
	long :0;
	long :0;
	/* if cpu_cnt == 0, open all on all possible CPUs (up to the number of
	 * max_entries of given PERF_EVENT_ARRAY map)
	 */
	int cpu_cnt;
	/* if cpu_cnt > 0, cpus is an array of CPUs to open ring buffers on */
	int *cpus;
	/* if cpu_cnt > 0, map_keys specify map keys to set per-CPU FDs for */
	int *map_keys;
};

[rafaeldtinoco]

Beware of these other bit-fields (the last two in special - perf_buffer_raw_opts):

If someone can confirm that wrapping the structure through a function (like I proposed) deals with the issue (which I dont see a reason why it wouldn't), then we will have to make sure, like you probably thought so, we do for all needed cases indeed.

[yanivagman]

I believe the correct workaround is something else.

We should wrap the struct with optimizations in a C function and deal with the needed structure without using CGO (so golang does not have to deal with padding/optimizations).

I agree. With this approach, we can be sure that the workaround works as expected.
I believe there is indeed a CGO related problem here.

[geyslan]

Another way could be create getters and setters:

golang/go#43261 (comment)

[grantseltzer]

I've made changes accordingly and it looks like it fixes the issue, or at the very least avoids it.

[geyslan]

I suppose that fprintf() in rare cases can change errno, what could make C.open_bpf_object() return a wrong one. WDYT?

[grantseltzer]

Are you sure about this? Wouldn't it be passed by value?

[geyslan]

I'm supposing based on these:

https://github.com/bminor/glibc/blob/master/stdio-common/vfprintf-internal.c#L681-L683
https://github.com/bminor/glibc/blob/master/stdio-common/vfprintf-internal.c#L714
https://github.com/bminor/glibc/blob/master/stdio-common/vfprintf-internal.c#L887
https://github.com/bminor/glibc/blob/master/stdio-common/vfprintf-internal.c#L914
https://github.com/bminor/glibc/blob/master/stdio-common/vfprintf-internal.c#L937
https://github.com/bminor/glibc/blob/master/stdio-common/vfprintf-internal.c#L960
https://github.com/bminor/glibc/blob/master/stdio-common/vfprintf-internal.c#L1075

[geyslan]

The thing is that strerror() has this in its manual:

       POSIX.1-2001 and POSIX.1-2008 require that a successful  call  to  str‐
       error()  or  strerror_l()  shall  leave errno unchanged, and note that,
       since no function return value is reserved to indicate an error, an ap‐
       plication  that  wishes  to check for errors should initialize errno to
       zero before the call, and then check errno after the call.

But none are mentioned about errno in fprintf manual. And besides saving current errno in save_errno I didn't find it returning it as an error. It returns done -1. I could be wrong for sure, just skimming on this matter.

[geyslan]

Try this:

#include <stdio.h>
#include <errno.h>
#include <string.h>

int main(void) {
    printf("errno: %d - %s\n", errno, strerror(errno));
    fprintf(stderr, "TESTING: %'.5000000000f'\n", 1234567.89);
    printf("errno: %d - %s\n", errno, strerror(errno));

    return 0;
}

❯ ./test            
errno: 0 - Success
TESTING: errno: 75 - Value too large for defined data type

So, based on this I assume that printf family functions can taint errno. And of course, your current code is error free, but future changes can cause errors to arise. The same for init_ring_buf and init_perf_buf.

[grantseltzer]

Very interesting!! So is the fix for this just this?

	struct bpf_object *obj = bpf_object__open_mem(obj_buf, obj_buf_size, &opts);
	if (obj == NULL) {
			int saved_errno = errno;
			fprintf(stderr, "Failed to open bpf object: %s\n", strerror(saved_errno));
			return NULL;
	}

[geyslan]

Very interesting!! So is the fix for this just this?

I would:

   if (obj == NULL) {
       int saved_errno = errno;
       fprintf(stderr, "Failed to open bpf object: %s\n", strerror(errno));
       errno = saved_errno;
       return NULL;
   }

So we keep the main (right) errno returning to go.

[geyslan]

Just an addendum: https://man7.org/linux/man-pages/man3/errno.3.html#NOTES

[grantseltzer]

Done, looks like the pattern is done in a couple other places too, i'll fix them in a follow up PR.

Handled