[PATCH bpf-next 1/2] bpf: Eliminate dup/restore of insn_aux_data

[Xu Kuohai]

From: Xu Kuohai <xukuohai@xxxxxxxxxx>

The dup/restore of insn_aux_data was introduced to resolve the
inconsistency between insnsi and insn_aux_data arrays, which occurs
on the failure path where insnsi was rolled back to the original
state before constants blinding, while insn_aux_data was not.

After JIT failure, there is only one user, bpf_clear_insn_aux_data(),
that requires insnsi and insn_aux_data to be synchronized. It accesses
both insnsi and insn_aux_data using the same array size and index.

However, the access to insnsi in bpf_clear_insn_aux_data() is not
necessary. It is checked to skip the second slot of an ldimm64 instruction,
whose jt is never set and can be absorbed into the jt check itself.

So remove the access to insnsi from bpf_clear_insn_aux_data(), and add a
specific length field for insn_aux_data to allow it to have a different
length from the insnsi array. Then remove dup/restore of insn_aux_data.

Signed-off-by: Xu Kuohai <xukuohai@xxxxxxxxxx>
---
 include/linux/bpf_verifier.h |  1 +
 include/linux/filter.h       | 13 -------------
 kernel/bpf/core.c            | 16 ----------------
 kernel/bpf/fixups.c          | 36 ++----------------------------------
 kernel/bpf/verifier.c        |  4 ++--
 5 files changed, 5 insertions(+), 65 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 317e99b9acc0..c98ec9de5ba6 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -948,6 +948,7 @@ struct bpf_verifier_env {
 	bool seen_direct_write;
 	bool seen_exception;
 	bool signature;
+	u32 insn_aux_data_len;
 	struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */
 	const struct bpf_line_info *prev_linfo;
 	struct bpf_verifier_log log;
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 14acb2455746..25148865f9c7 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1211,25 +1211,12 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 #ifdef CONFIG_BPF_SYSCALL
 struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 off,
 				     const struct bpf_insn *patch, u32 len);
-struct bpf_insn_aux_data *bpf_dup_insn_aux_data(struct bpf_verifier_env *env);
-void bpf_restore_insn_aux_data(struct bpf_verifier_env *env,
-			       struct bpf_insn_aux_data *orig_insn_aux);
 #else
 static inline struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 off,
 						   const struct bpf_insn *patch, u32 len)
 {
 	return ERR_PTR(-ENOTSUPP);
 }
-
-static inline struct bpf_insn_aux_data *bpf_dup_insn_aux_data(struct bpf_verifier_env *env)
-{
-	return NULL;
-}
-
-static inline void bpf_restore_insn_aux_data(struct bpf_verifier_env *env,
-					     struct bpf_insn_aux_data *orig_insn_aux)
-{
-}
 #endif /* CONFIG_BPF_SYSCALL */
 
 int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 47fe047ad30b..b7f17f89d992 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2622,22 +2622,10 @@ static struct bpf_prog *bpf_prog_jit_compile(struct bpf_verifier_env *env, struc
 {
 #ifdef CONFIG_BPF_JIT
 	struct bpf_prog *orig_prog;
-	struct bpf_insn_aux_data *orig_insn_aux;
 
 	if (!bpf_prog_need_blind(prog))
 		return bpf_int_jit_compile(env, prog);
 
-	if (env) {
-		/*
-		 * If env is not NULL, we are called from the end of bpf_check(), at this
-		 * point, only insn_aux_data is used after failure, so it should be restored
-		 * on failure.
-		 */
-		orig_insn_aux = bpf_dup_insn_aux_data(env);
-		if (!orig_insn_aux)
-			return prog;
-	}
-
 	orig_prog = prog;
 	prog = bpf_jit_blind_constants(env, prog);
 	/*
@@ -2650,8 +2638,6 @@ static struct bpf_prog *bpf_prog_jit_compile(struct bpf_verifier_env *env, struc
 	prog = bpf_int_jit_compile(env, prog);
 	if (prog->jited) {
 		bpf_jit_prog_release_other(prog, orig_prog);
-		if (env)
-			vfree(orig_insn_aux);
 		return prog;
 	}
 
@@ -2659,8 +2645,6 @@ static struct bpf_prog *bpf_prog_jit_compile(struct bpf_verifier_env *env, struc
 
 out_restore:
 	prog = orig_prog;
-	if (env)
-		bpf_restore_insn_aux_data(env, orig_insn_aux);
 #endif
 	return prog;
 }
diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index d3be972714b2..37f22eb41854 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -174,6 +174,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
 	if (cnt == 1)
 		return;
 	prog_len = new_prog->len;
+	env->insn_aux_data_len = prog_len;
 
 	memmove(data + off + cnt - 1, data + off,
 		sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
@@ -440,7 +441,6 @@ static int bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
 void bpf_clear_insn_aux_data(struct bpf_verifier_env *env, int start, int len)
 {
 	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
-	struct bpf_insn *insns = env->prog->insnsi;
 	int end = start + len;
 	int i;
 
@@ -449,9 +449,6 @@ void bpf_clear_insn_aux_data(struct bpf_verifier_env *env, int start, int len)
 			kvfree(aux_data[i].jt);
 			aux_data[i].jt = NULL;
 		}
-
-		if (bpf_is_ldimm64(&insns[i]))
-			i++;
 	}
 }
 
@@ -464,7 +461,6 @@ static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
 	if (bpf_prog_is_offloaded(env->prog->aux))
 		bpf_prog_offload_remove_insns(env, off, cnt);
 
-	/* Should be called before bpf_remove_insns, as it uses prog->insnsi */
 	bpf_clear_insn_aux_data(env, off, cnt);
 
 	err = bpf_remove_insns(env->prog, off, cnt);
@@ -483,6 +479,7 @@ static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
 
 	memmove(aux_data + off,	aux_data + off + cnt,
 		sizeof(*aux_data) * (orig_prog_len - off - cnt));
+	env->insn_aux_data_len -= cnt;
 
 	return 0;
 }
@@ -1005,26 +1002,6 @@ static void bpf_restore_subprog_starts(struct bpf_verifier_env *env, u32 *orig_s
 	env->subprog_info[env->subprog_cnt].start = env->prog->len;
 }
 
-struct bpf_insn_aux_data *bpf_dup_insn_aux_data(struct bpf_verifier_env *env)
-{
-	size_t size;
-	void *new_aux;
-
-	size = array_size(sizeof(struct bpf_insn_aux_data), env->prog->len);
-	new_aux = __vmalloc(size, GFP_KERNEL_ACCOUNT);
-	if (new_aux)
-		memcpy(new_aux, env->insn_aux_data, size);
-	return new_aux;
-}
-
-void bpf_restore_insn_aux_data(struct bpf_verifier_env *env,
-			       struct bpf_insn_aux_data *orig_insn_aux)
-{
-	/* the expanded elements are zero-filled, so no special handling is required */
-	vfree(env->insn_aux_data);
-	env->insn_aux_data = orig_insn_aux;
-}
-
 static int jit_subprogs(struct bpf_verifier_env *env)
 {
 	struct bpf_prog *prog = env->prog, **func, *tmp;
@@ -1299,7 +1276,6 @@ int bpf_jit_subprogs(struct bpf_verifier_env *env)
 	bool blinded = false;
 	struct bpf_insn *insn;
 	struct bpf_prog *prog, *orig_prog;
-	struct bpf_insn_aux_data *orig_insn_aux;
 	u32 *orig_subprog_starts;
 
 	if (env->subprog_cnt <= 1)
@@ -1307,14 +1283,8 @@ int bpf_jit_subprogs(struct bpf_verifier_env *env)
 
 	prog = orig_prog = env->prog;
 	if (bpf_prog_need_blind(prog)) {
-		orig_insn_aux = bpf_dup_insn_aux_data(env);
-		if (!orig_insn_aux) {
-			err = -ENOMEM;
-			goto out_cleanup;
-		}
 		orig_subprog_starts = bpf_dup_subprog_starts(env);
 		if (!orig_subprog_starts) {
-			vfree(orig_insn_aux);
 			err = -ENOMEM;
 			goto out_cleanup;
 		}
@@ -1334,7 +1304,6 @@ int bpf_jit_subprogs(struct bpf_verifier_env *env)
 	if (blinded) {
 		bpf_jit_prog_release_other(prog, orig_prog);
 		kvfree(orig_subprog_starts);
-		vfree(orig_insn_aux);
 	}
 
 	return 0;
@@ -1364,7 +1333,6 @@ int bpf_jit_subprogs(struct bpf_verifier_env *env)
 
 out_restore:
 	bpf_restore_subprog_starts(env, orig_subprog_starts);
-	bpf_restore_insn_aux_data(env, orig_insn_aux);
 	kvfree(orig_subprog_starts);
 out_cleanup:
 	/* cleanup main prog to be interpreted */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 03e2202cca13..85b05529f107 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20108,7 +20108,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
 	if (!is_priv)
 		mutex_lock(&bpf_verifier_lock);
 
-	len = env->prog->len;
+	len = env->insn_aux_data_len = env->prog->len;
 	env->insn_aux_data =
 		__vmalloc(array_size(sizeof(struct bpf_insn_aux_data), len),
 			  GFP_KERNEL_ACCOUNT | __GFP_ZERO);
@@ -20354,7 +20354,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
 	release_btfs(env);
 err_free_env:
 	if (env->insn_aux_data)
-		bpf_clear_insn_aux_data(env, 0, env->prog->len);
+		bpf_clear_insn_aux_data(env, 0, env->insn_aux_data_len);
 	vfree(env->insn_aux_data);
 	kvfree(env->fd_array);
 	bpf_stack_liveness_free(env);
-- 
2.43.0