Re: [PATCH v2 bpf-next 3/7] bpf: Introduce function-by-function verification

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Song Liu <songliubraving@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	David Miller <davem@davemloft.net>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 3/7] bpf: Introduce function-by-function verification
Date: Thu, 9 Jan 2020 14:17:40 -0800	[thread overview]
Message-ID: <20200109221739.a7wuiqe37rqameqh@ast-mbp> (raw)
In-Reply-To: <B7A2A8DD-B070-4F80-A9A0-6570260D4346@fb.com>

On Thu, Jan 09, 2020 at 06:09:08PM +0000, Song Liu wrote:
> 
> 
> > On Jan 8, 2020, at 10:37 PM, Alexei Starovoitov <ast@kernel.org> wrote:
> 
> [...]
> 
> > 
> > Note that the stack limit of 512 still applies to the call chain regardless whether
> > functions were static or global. The nested level of 8 also still applies. The
> > same recursion prevention checks are in place as well.
> > 
> > The type information and static/global kind is preserved after the verification
> > hence in the above example global function f2() and f3() can be replaced later
> > by equivalent functions with the same types that are loaded and verified later
> > without affecting safety of this main() program. Such replacement (re-linking)
> > of global functions is a subject of future patches.
> > 
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> 
> With one nit below. 
> 
> [...]
> 
> > +
> > +static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > +{
> > +	struct bpf_verifier_state *state;
> > +	struct bpf_reg_state *regs;
> > +	int ret, i;
> > +
> > +	env->prev_linfo = NULL;
> > +	env->pass_cnt++;
> > +
> > +	state = kzalloc(sizeof(struct bpf_verifier_state), GFP_KERNEL);
> > +	if (!state)
> > +		return -ENOMEM;
> > +	state->curframe = 0;
> > +	state->speculative = false;
> > +	state->branches = 1;
> > +	state->frame[0] = kzalloc(sizeof(struct bpf_func_state), GFP_KERNEL);
> > +	if (!state->frame[0]) {
> > +		kfree(state);
> > +		return -ENOMEM;
> > +	}
> > +	env->cur_state = state;
> > +	init_func_state(env, state->frame[0],
> > +			BPF_MAIN_FUNC /* callsite */,
> > +			0 /* frameno */,
> > +			subprog);
> > +
> > +	regs = state->frame[state->curframe]->regs;
> > +	if (subprog) {
> > +		ret = btf_prepare_func_args(env, subprog, regs);
> > +		if (ret)
> > +			goto out;
> > +		for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> > +			if (regs[i].type == PTR_TO_CTX)
> > +				mark_reg_known_zero(env, regs, i);
> > +			else if (regs[i].type == SCALAR_VALUE)
> > +				mark_reg_unknown(env, regs, i);
> > +		}
> > +	} else {
> > +		/* 1st arg to a function */
> > +		regs[BPF_REG_1].type = PTR_TO_CTX;
> > +		mark_reg_known_zero(env, regs, BPF_REG_1);
> > +		ret = btf_check_func_arg_match(env, subprog, regs);
> > +		if (ret == -EFAULT)
> > +			/* unlikely verifier bug. abort.
> > +			 * ret == 0 and ret < 0 are sadly acceptable for
> > +			 * main() function due to backward compatibility.
> > +			 * Like socket filter program may be written as:
> > +			 * int bpf_prog(struct pt_regs *ctx)
> > +			 * and never dereference that ctx in the program.
> > +			 * 'struct pt_regs' is a type mismatch for socket
> > +			 * filter that should be using 'struct __sk_buff'.
> > +			 */
> > +			goto out;
> > +	}
> > +
> > +	ret = do_check(env);
> > +out:
> > +	if (env->cur_state) {
> 
> I think env->cur_state will never be NULL here. This check is necessary 
> before this patch (when we allocate cur_state in do_check()). 

yeah. good catch. 'if' can be dropped. I'll follow up with a clean up patch or
will fold it if respin is necessary for other reasons.