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

On Jan 8, 2020, at 12:20 PM, Alexei Starovoitov [off-list ref] wrote:

On Wed, Jan 08, 2020 at 07:10:59PM +0000, Song Liu wrote:

quoted

quoted

On Jan 7, 2020, at 11:25 PM, Alexei Starovoitov [off-list ref] wrote:

New llvm and old llvm with libbpf help produce BTF that distinguish global and
static functions. Unlike arguments of static function the arguments of global
functions cannot be removed or optimized away by llvm. The compiler has to use
exactly the arguments specified in a function prototype. The argument type
information allows the verifier validate each global function independently.
For now only supported argument types are pointer to context and scalars. In
the future pointers to structures, sizes, pointer to packet data can be
supported as well. Consider the following example:

[...]

quoted

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>
---
include/linux/bpf.h          |   7 +-
include/linux/bpf_verifier.h |   7 +-
include/uapi/linux/btf.h     |   6 +
kernel/bpf/btf.c             | 147 +++++++++++++++++-----
kernel/bpf/verifier.c        | 228 +++++++++++++++++++++++++++--------
5 files changed, 317 insertions(+), 78 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b14e51d56a82..ceb5b6c13abc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -558,6 +558,7 @@ static inline void bpf_dispatcher_change_prog(struct bpf_dispatcher *d,

#endif

struct bpf_func_info_aux {
+	u32 linkage;

How about we use u16 or even u8 for linkage? We are using BTF_INFO_VLEN() which 
is 16-bit long. Maybe we should save some bits for future extensions?

sure. u16 is fine.
Will also introduce btf_func_kind() helper to avoid misleading BTF_INFO_VLEN macro.

quoted

quoted

-int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog)
+/* Compare BTF of a function with given bpf_reg_state */
+int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
+			     struct bpf_reg_state *reg)

I think we need more comments for the retval of btf_check_func_arg_match().

sure.

quoted

quoted

{
-	struct bpf_verifier_state *st = env->cur_state;
-	struct bpf_func_state *func = st->frame[st->curframe];
-	struct bpf_reg_state *reg = func->regs;
	struct bpf_verifier_log *log = &env->log;
	struct bpf_prog *prog = env->prog;
	struct btf *btf = prog->aux->btf;

[...]

quoted

+
+/* Convert BTF of a function into bpf_reg_state if possible */
+int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
+			  struct bpf_reg_state *reg)
+{
+	struct bpf_verifier_log *log = &env->log;
+	struct bpf_prog *prog = env->prog;
+	struct btf *btf = prog->aux->btf;
+	const struct btf_param *args;
+	const struct btf_type *t;
+	u32 i, nargs, btf_id;
+	const char *tname;
+
+	if (!prog->aux->func_info ||
+	    prog->aux->func_info_aux[subprog].linkage != BTF_FUNC_GLOBAL) {
+		bpf_log(log, "Verifier bug\n");

IIUC, this should never happen? Maybe we need more details in the log, and 
maybe also WARN_ONCE()?

Should never happen and I think it's pretty clear from the diff, since
this function is called after == FUNC_GLOBAL check in the caller.
I didn't add WARN to avoid wasting .text even more here.
Single 'if' above already feels a bit overly defensive.
It's not like other cases in the verifier where we have WARN_ONCE.
Those are for complex things. Here it's one callsite and trivial control flow.

Agreed. Current check is good enough. 

quoted

quoted

+	if (prog->aux->func_info_aux[subprog].unreliable) {
+		bpf_log(log, "Verifier bug in function %s()\n", tname);
+		return -EFAULT;

Why -EFAULT instead of -EINVAL? I think we treat them the same? 

EFAULT is a verifier bug like in all other places in the verifier
where EFAULT is returned. EINVAL is normal error.

Thanks for the explanation. 

Song