INTERNAL REVIEW: Btfgen v2

[mauriciovasquezbernal]

This is a PR intended for provide internal review of some patches. Please ignore it.

[rafaeldtinoco]

Please take a look at bpf_object__relocate_core(). It has:

	if (targ_btf_path) {
		obj->btf_vmlinux_override = btf__parse(targ_btf_path, NULL);
		err = libbpf_get_error(obj->btf_vmlinux_override);
		if (err) {
			pr_warn("failed to parse target BTF: %d\n", err);
			return err;
		}
	}

this targ_btf_path comes from function argument and it can be (from bpf_object__load_xattr()):

• struct bpf_object_open_opts {} -> char *btf_custom_path OR
• struct bpf_object_load_attr {} -> char *target_btf_path

You're checking for btf_custom_path and btf_custom existence simultaneously (and returning -EINVAL) during bpf object open time.

Shouldn't you also check if target_btf_path (from struct bpf_object_load_attr {}) wasn't set before load time (because of logic in open path) ? Couldn't it re-define btf_vmlinux_override from the snippet above ?

I think you can check if btf_vmlinux_override is NULL and, if it is, it means that it wasn't set during opening time. Is that right ? Or you can check for user_provided_btf_vmlinux here as well.

[mauriciovasquezbernal]

Shouldn't you also check if target_btf_path (from struct bpf_object_load_attr {}) wasn't set before load time (because of logic in open path) ? Couldn't it re-define btf_vmlinux_override from the snippet above ?

Yes, I missed that check. Added a check avoid redefining it when already provided by the user.

[rafaeldtinoco]

I have proposed an update to btfgen:

https://github.com/kinvolk/btfgen/pull/12/commits

using this new patches... I have also applied the 4 patches into vanilla libbpf tree:

kinvolk/libbpf#24

this way we can test this latest version.

[mauriciovasquezbernal]

For the reference this PR was sent upstream as https://lore.kernel.org/bpf/20211116164208.164245-1-mauricio@kinvolk.io/T/