RE: [PATCH v5 bpf-next 05/14] libbpf: add kernel module BTF support for CO-RE relocations

[John Fastabend <john.fastabend@gmail.com>]

Andrii Nakryiko wrote:
> Teach libbpf to search for candidate types for CO-RE relocations across kernel
> modules BTFs, in addition to vmlinux BTF. If at least one candidate type is
> found in vmlinux BTF, kernel module BTFs are not iterated. If vmlinux BTF has
> no matching candidates, then find all kernel module BTFs and search for all
> matching candidates across all of them.
> 
> Kernel's support for module BTFs are inferred from the support for BTF name
> pointer in BPF UAPI.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
  
small typo and missing close maybe? Otherwise,

Acked-by: John Fastabend <john.fastabend@gmail.com>

> +static int load_module_btfs(struct bpf_object *obj)
> +{
> +	struct bpf_btf_info info;
> +	struct module_btf *mod_btf;
> +	struct btf *btf;
> +	char name[64];
> +	__u32 id = 0, len;
> +	int err, fd;
> +
> +	if (obj->btf_modules_loaded)
> +		return 0;
> +
> +	/* don't do this again, even if we find no module BTFs */

I wonder a bit if we might load modules at some point and want
to then run this, but I wouldn't worry about it. Its likely
easy enough to just reload or delay load until module is in place.

> +	obj->btf_modules_loaded = true;
> +
> +	/* kernel too old to support module BTFs */
> +	if (!kernel_supports(FEAT_MODULE_BTF))
> +		return 0;
> +
> +	while (true) {
> +		err = bpf_btf_get_next_id(id, &id);
> +		if (err && errno == ENOENT)
> +			return 0;
> +		if (err) {
> +			err = -errno;
> +			pr_warn("failed to iterate BTF objects: %d\n", err);
> +			return err;
> +		}
> +
> +		fd = bpf_btf_get_fd_by_id(id);
> +		if (fd < 0) {
> +			if (errno == ENOENT)
> +				continue; /* expected race: BTF was unloaded */
> +			err = -errno;
> +			pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
> +			return err;
> +		}
> +
> +		len = sizeof(info);
> +		memset(&info, 0, sizeof(info));
> +		info.name = ptr_to_u64(name);
> +		info.name_len = sizeof(name);
> +
> +		err = bpf_obj_get_info_by_fd(fd, &info, &len);
> +		if (err) {

do we also need to close(fd) here? If not then we don't need the close in xsk.c
I guess.

> +			err = -errno;
> +			pr_warn("failed to get BTF object #%d info: %d\n", id, err);
> +			return err;
> +		}
> +
> +		/* ignore non-module BTFs */
> +		if (!info.kernel_btf || strcmp(name, "vmlinux") == 0) {
> +			close(fd);
> +			continue;
> +		}
> +
> +		btf = btf_get_from_fd(fd, obj->btf_vmlinux);
> +		close(fd);
> +		if (IS_ERR(btf)) {
> +			pr_warn("failed to load module [%s]'s BTF object #%d: %ld\n",
> +				name, id, PTR_ERR(btf));
> +			return PTR_ERR(btf);
> +		}
> +
> +		err = btf_ensure_mem((void **)&obj->btf_modules, &obj->btf_module_cap,
> +				     sizeof(*obj->btf_modules), obj->btf_module_cnt + 1);
> +		if (err)
> +			return err;
> +
> +		mod_btf = &obj->btf_modules[obj->btf_module_cnt++];
> +
> +		mod_btf->btf = btf;
> +		mod_btf->id = id;
> +		mod_btf->name = strdup(name);
> +		if (!mod_btf->name)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct core_cand_list *
>  bpf_core_find_cands(struct bpf_object *obj, const struct btf *local_btf, __u32 local_type_id)
>  {
>  	struct core_cand local_cand = {};
>  	struct core_cand_list *cands;
> +	const struct btf *main_btf;
>  	size_t local_essent_len;
> -	int err;
> +	int err, i;
>  
>  	local_cand.btf = local_btf;
>  	local_cand.t = btf__type_by_id(local_btf, local_type_id);
> @@ -4697,15 +4824,38 @@ bpf_core_find_cands(struct bpf_object *obj, const struct btf *local_btf, __u32 l
>  		return ERR_PTR(-ENOMEM);
>  
>  	/* Attempt to find target candidates in vmlinux BTF first */
> -	err = bpf_core_add_cands(&local_cand, local_essent_len,
> -				 obj->btf_vmlinux_override ?: obj->btf_vmlinux,
> -				 "vmlinux", 1, cands);
> -	if (err) {
> -		bpf_core_free_cands(cands);
> -		return ERR_PTR(err);
> +	main_btf = obj->btf_vmlinux_override ?: obj->btf_vmlinux;
> +	err = bpf_core_add_cands(&local_cand, local_essent_len, main_btf, "vmlinux", 1, cands);
> +	if (err)
> +		goto err_out;
> +
> +	/* if vmlinux BTF has any candidate, don't got for module BTFs */

small typo: don't got for -> don't go for

> +	if (cands->len)
> +		return cands;
> +
> +	/* if vmlinux BTF was overridden, don't attempt to load module BTFs */
> +	if (obj->btf_vmlinux_override)
> +		return cands;
> +
> +	/* now look through module BTFs, trying to still find candidates */
> +	err = load_module_btfs(obj);
> +	if (err)
> +		goto err_out;
> +
> +	for (i = 0; i < obj->btf_module_cnt; i++) {
> +		err = bpf_core_add_cands(&local_cand, local_essent_len,
> +					 obj->btf_modules[i].btf,
> +					 obj->btf_modules[i].name,
> +					 btf__get_nr_types(obj->btf_vmlinux) + 1,
> +					 cands);
> +		if (err)
> +			goto err_out;
>  	}
>  
>  	return cands;
> +err_out:
> +	bpf_core_free_cands(cands);
> +	return ERR_PTR(err);
>  }
>