Re: [RFC bpf-next 2/3] bpf: add support for module helpers in verifier

[Usama Arif <usama.arif@xxxxxxxxxxxxx>]

On 22/01/2022 03:56, Kumar Kartikeya Dwivedi wrote:
> On Sat, Jan 22, 2022 at 09:01:33AM IST, Kumar Kartikeya Dwivedi wrote:
> > On Sat, Jan 22, 2022 at 01:09:55AM IST, Usama Arif wrote:
> > > After the kernel module registers the helper, its BTF id
> > > and func_proto are available during verification. During
> > > verification, it is checked to see if insn->imm is available
> > > in the list of module helper btf ids. If it is,
> > > check_helper_call is called, otherwise check_kfunc_call.
> > > The module helper function proto is obtained in check_helper_call
> > > via get_mod_helper_proto function.
> > >
> > > Signed-off-by: Usama Arif <usama.arif@xxxxxxxxxxxxx>
> > > ---
> > >   kernel/bpf/verifier.c | 50 +++++++++++++++++++++++++++++++++----------
> > >   1 file changed, 39 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 8c5a46d41f28..bf7605664b95 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -6532,19 +6532,39 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > >   	int insn_idx = *insn_idx_p;
> > >   	bool changes_data;
> > >   	int i, err, func_id;
> > > +	const struct btf_type *func;
> > > +	const char *func_name;
> > > +	struct btf *desc_btf;
> > >
> > >   	/* find function prototype */
> > >   	func_id = insn->imm;
> > > -	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
> > > -		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
> > > -			func_id);
> > > -		return -EINVAL;
> > > -	}
> > >
> > >   	if (env->ops->get_func_proto)
> > >   		fn = env->ops->get_func_proto(func_id, env->prog);
> > > -	if (!fn) {
> > > -		verbose(env, "unknown func %s#%d\n", func_id_name(func_id),
> > > +
> > > +	if (func_id >= __BPF_FUNC_MAX_ID) {
> > > +		desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
> >
> > I am not sure this is right, even if we reached this point. add_kfunc_call would
> > not be called for a helper call, which means the kfunc_btf_tab will not be
> > populated. I think this code is not reachable from your test, which is why you
> > didn't see this. More below.
> >
> > > +		if (IS_ERR(desc_btf))
> > > +			return PTR_ERR(desc_btf);
> > > +
> > > +		fn = get_mod_helper_proto(desc_btf, func_id);
> > > +		if (!fn) {
> > > +			func = btf_type_by_id(desc_btf, func_id);
> > > +			func_name = btf_name_by_offset(desc_btf, func->name_off);
> > > +			verbose(env, "unknown module helper func %s#%d\n", func_name,
> > > +				func_id);
> > > +			return -EACCES;
> > > +		}
> > > +	} else if (func_id >= 0) {
> > > +		if (env->ops->get_func_proto)
> > > +			fn = env->ops->get_func_proto(func_id, env->prog);
> > > +		if (!fn) {
> > > +			verbose(env, "unknown in-kernel helper func %s#%d\n", func_id_name(func_id),
> > > +				func_id);
> > > +			return -EINVAL;
> > > +		}
> > > +	} else {
> > > +		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
> > >   			func_id);
> > >   		return -EINVAL;
> > >   	}
> > > @@ -11351,6 +11371,7 @@ static int do_check(struct bpf_verifier_env *env)
> > >   	int insn_cnt = env->prog->len;
> > >   	bool do_print_state = false;
> > >   	int prev_insn_idx = -1;
> > > +	struct btf *desc_btf;
> > >
> > >   	for (;;) {
> > >   		struct bpf_insn *insn;
> > > @@ -11579,10 +11600,17 @@ static int do_check(struct bpf_verifier_env *env)
> > >   				}
> > >   				if (insn->src_reg == BPF_PSEUDO_CALL)
> > >   					err = check_func_call(env, insn, &env->insn_idx);
> > > -				else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
> > > -					err = check_kfunc_call(env, insn, &env->insn_idx);
> > > -				else
> > > -					err = check_helper_call(env, insn, &env->insn_idx);
> > > +				else {
> > > +					desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
> > > +					if (IS_ERR(desc_btf))
> > > +						return PTR_ERR(desc_btf);
> > > +
> >
> > I didn't get this part at all.
> >
> > At this point src_reg can be BPF_PSEUDO_KFUNC_CALL, or 0 (for helper call). If
> > it is a helper call, then find_kfunc_desc_btf using insn->imm and insn->off
> > would be a bug.
> >
> > > +					if (insn->src_reg == BPF_K ||
> >
> > [...]
>
> Ah, I think I see what you are doing: BPF_K is zero, so either when it is a
> helper call or it is a module helper (which will be a kfunc), you call
> check_helper_call. get_mod_helper_proto would return true in that case.
>
> But if it is an in-kernel helper, calling find_kfunc_desc_btf would still be a
> bug, since imm encodes func_id.
>
> It's also a bit confusing that check_helper_call is called for a kfunc.

Hi, Thanks for the reviews!

The patchset does require refractoring in relation to kfunc to make it 
mergable as mentioned in cover letter, but it is a working prototype for 
all situations in-kernel/module helper/kfunc.

There are 3 situations:
- if its an in-kernel helper call, insn->src_reg is BPF_K and 
check_helper_call will be called. get_mod_helper_proto will return NULL 
in this case but that doesnt matter as its an OR statement.
- if its a "module helper call" (essentially a kfunc with 
bpf_func_proto) and it has been registered, get_mod_helper_proto will 
return the function proto and check_helper_call will be called. 
insn->src_reg will be BPF_PSEUDO_KFUNC_CALL (requires refractoring) but 
that wont matter as its an OR statement.
- if its a kfunc call, insn->src_reg is BPF_PSEUDO_KFUNC_CALL and 
get_mod_helper_proto will return NULL, so check_kfunc_call will be called.

So all the cases will be covered. I didnt include the refractor as its 
quite big and just wanted to check if something like this will be 
considered by the bpf community before progressing further.

> > > +					   get_mod_helper_proto(desc_btf, insn->imm))
> > > +						err = check_helper_call(env, insn, &env->insn_idx);
> > > +					else
> > > +						err = check_kfunc_call(env, insn, &env->insn_idx);
> > > +				}
> > >   				if (err)
> > >   					return err;
> > >   			} else if (opcode == BPF_JA) {
> > > --
> > > 2.25.1