Re: [PATCH v2 bpf-next 5/7] bpf: support BPF ksym variables in kernel modules

[Yonghong Song <yhs@fb.com>]

On 1/8/21 2:09 PM, Andrii Nakryiko wrote:
> Add support for directly accessing kernel module variables from BPF programs
> using special ldimm64 instructions. This functionality builds upon vmlinux
> ksym support, but extends ldimm64 with src_reg=BPF_PSEUDO_BTF_ID to allow
> specifying kernel module BTF's FD in insn[1].imm field.
> 
> During BPF program load time, verifier will resolve FD to BTF object and will
> take reference on BTF object itself and, for module BTFs, corresponding module
> as well, to make sure it won't be unloaded from under running BPF program. The
> mechanism used is similar to how bpf_prog keeps track of used bpf_maps.
> 
> One interesting change is also in how per-CPU variable is determined. The
> logic is to find .data..percpu data section in provided BTF, but both vmlinux
> and module each have their own .data..percpu entries in BTF. So for module's
> case, the search for DATASEC record needs to look at only module's added BTF
> types. This is implemented with custom search function.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Ack with a minor nit below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   include/linux/bpf.h          |  10 +++
>   include/linux/bpf_verifier.h |   3 +
>   include/linux/btf.h          |   3 +
>   kernel/bpf/btf.c             |  31 +++++++-
>   kernel/bpf/core.c            |  23 ++++++
>   kernel/bpf/verifier.c        | 149 ++++++++++++++++++++++++++++-------
>   6 files changed, 189 insertions(+), 30 deletions(-)
> 
[...]
>   /* replace pseudo btf_id with kernel symbol address */
>   static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>   			       struct bpf_insn *insn,
> @@ -9710,48 +9735,57 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>   {
>   	const struct btf_var_secinfo *vsi;
>   	const struct btf_type *datasec;
> +	struct btf_mod_pair *btf_mod;
>   	const struct btf_type *t;
>   	const char *sym_name;
>   	bool percpu = false;
>   	u32 type, id = insn->imm;
> +	struct btf *btf;
>   	s32 datasec_id;
>   	u64 addr;
> -	int i;
> +	int i, btf_fd, err;
>   
> -	if (!btf_vmlinux) {
> -		verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
> -		return -EINVAL;
> -	}
> -
> -	if (insn[1].imm != 0) {
> -		verbose(env, "reserved field (insn[1].imm) is used in pseudo_btf_id ldimm64 insn.\n");
> -		return -EINVAL;
> +	btf_fd = insn[1].imm;
> +	if (btf_fd) {
> +		btf = btf_get_by_fd(btf_fd);
> +		if (IS_ERR(btf)) {
> +			verbose(env, "invalid module BTF object FD specified.\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (!btf_vmlinux) {
> +			verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
> +			return -EINVAL;
> +		}
> +		btf = btf_vmlinux;
> +		btf_get(btf);
>   	}
>   
> -	t = btf_type_by_id(btf_vmlinux, id);
> +	t = btf_type_by_id(btf, id);
>   	if (!t) {
>   		verbose(env, "ldimm64 insn specifies invalid btf_id %d.\n", id);
> -		return -ENOENT;
> +		err = -ENOENT;
> +		goto err_put;
>   	}
>   
>   	if (!btf_type_is_var(t)) {
> -		verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR.\n",
> -			id);
> -		return -EINVAL;
> +		verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR.\n", id);
> +		err = -EINVAL;
> +		goto err_put;
>   	}
>   
> -	sym_name = btf_name_by_offset(btf_vmlinux, t->name_off);
> +	sym_name = btf_name_by_offset(btf, t->name_off);
>   	addr = kallsyms_lookup_name(sym_name);
>   	if (!addr) {
>   		verbose(env, "ldimm64 failed to find the address for kernel symbol '%s'.\n",
>   			sym_name);
> -		return -ENOENT;
> +		err = -ENOENT;
> +		goto err_put;
>   	}
>   
> -	datasec_id = btf_find_by_name_kind(btf_vmlinux, ".data..percpu",
> -					   BTF_KIND_DATASEC);
> +	datasec_id = find_btf_percpu_datasec(btf);
>   	if (datasec_id > 0) {
> -		datasec = btf_type_by_id(btf_vmlinux, datasec_id);
> +		datasec = btf_type_by_id(btf, datasec_id);
>   		for_each_vsi(i, datasec, vsi) {
>   			if (vsi->type == id) {
>   				percpu = true;
> @@ -9764,10 +9798,10 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>   	insn[1].imm = addr >> 32;
>   
>   	type = t->type;
> -	t = btf_type_skip_modifiers(btf_vmlinux, type, NULL);
> +	t = btf_type_skip_modifiers(btf, type, NULL);
>   	if (percpu) {
>   		aux->btf_var.reg_type = PTR_TO_PERCPU_BTF_ID;
> -		aux->btf_var.btf = btf_vmlinux;
> +		aux->btf_var.btf = btf;
>   		aux->btf_var.btf_id = type;
>   	} else if (!btf_type_is_struct(t)) {
>   		const struct btf_type *ret;
> @@ -9775,21 +9809,54 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>   		u32 tsize;
>   
>   		/* resolve the type size of ksym. */
> -		ret = btf_resolve_size(btf_vmlinux, t, &tsize);
> +		ret = btf_resolve_size(btf, t, &tsize);
>   		if (IS_ERR(ret)) {
> -			tname = btf_name_by_offset(btf_vmlinux, t->name_off);
> +			tname = btf_name_by_offset(btf, t->name_off);
>   			verbose(env, "ldimm64 unable to resolve the size of type '%s': %ld\n",
>   				tname, PTR_ERR(ret));
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto err_put;
>   		}
>   		aux->btf_var.reg_type = PTR_TO_MEM;
>   		aux->btf_var.mem_size = tsize;
>   	} else {
>   		aux->btf_var.reg_type = PTR_TO_BTF_ID;
> -		aux->btf_var.btf = btf_vmlinux;
> +		aux->btf_var.btf = btf;
>   		aux->btf_var.btf_id = type;
>   	}
> +
> +	/* check whether we recorded this BTF (and maybe module) already */
> +	for (i = 0; i < env->used_btf_cnt; i++) {
> +		if (env->used_btfs[i].btf == btf) {
> +			btf_put(btf);
> +			return 0;

An alternative way is to change the above code as
			err = 0;
			goto err_put;

> +		}
> +	}
> +
> +	if (env->used_btf_cnt >= MAX_USED_BTFS) {
> +		err = -E2BIG;
> +		goto err_put;
> +	}
> +
> +	btf_mod = &env->used_btfs[env->used_btf_cnt];
> +	btf_mod->btf = btf;
> +	btf_mod->module = NULL;
> +
> +	/* if we reference variables from kernel module, bump its refcount */
> +	if (btf_is_module(btf)) {
> +		btf_mod->module = btf_try_get_module(btf);
> +		if (!btf_mod->module) {
> +			err = -ENXIO;
> +			goto err_put;
> +		}
> +	}
> +
> +	env->used_btf_cnt++;
> +
>   	return 0;
> +err_put:
> +	btf_put(btf);
> +	return err;
>   }
>   
[...]