[PATCH bpf-next v2 2/2] bpf: Remove unnecessary dup/restore subprog_starts and prog clone

[Xu Kuohai]

From: Xu Kuohai <xukuohai@xxxxxxxxxx>

When JIT is required by the bpf program, the kernel rejects the program
on JIT failure rather than falling back to the interpreter. In this case,
the subprog_starts duplication/restore and bpf prog clone are unnecessary,
since they are only used to restore state for the interpreter. So remove
the duplication/restore for this case.

Signed-off-by: Xu Kuohai <xukuohai@xxxxxxxxxx>
---
 include/linux/filter.h |   9 ++--
 kernel/bpf/core.c      | 104 +++++++++++++++++++++++------------------
 kernel/bpf/fixups.c    |  37 +++++++--------
 3 files changed, 83 insertions(+), 67 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 5c072c1360eb..2d1aa062566c 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1356,7 +1356,8 @@ int bpf_jit_get_func_addr(const struct bpf_prog *prog,
 
 const char *bpf_jit_get_prog_name(struct bpf_prog *prog);
 
-struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog *prog);
+int bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog **pprog,
+			    bool clone_needed, bool *cloned);
 void bpf_jit_prog_release_other(struct bpf_prog *fp, struct bpf_prog *fp_other);
 
 static inline bool bpf_prog_need_blind(const struct bpf_prog *prog)
@@ -1508,9 +1509,11 @@ static inline bool bpf_prog_need_blind(const struct bpf_prog *prog)
 }
 
 static inline
-struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog *prog)
+int bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog **pprog,
+			    bool clone_needed, bool *cloned)
 {
-	return prog;
+	*cloned = false;
+	return 0;
 }
 
 static inline void bpf_jit_prog_release_other(struct bpf_prog *fp, struct bpf_prog *fp_other)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 505d134cd264..ab5da3ff941a 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1564,27 +1564,34 @@ void bpf_jit_prog_release_other(struct bpf_prog *fp, struct bpf_prog *fp_other)
  * Now this function is used only to blind the main prog and must be invoked only when
  * bpf_prog_need_blind() returns true.
  */
-struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog *prog)
+int bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog **pprog,
+			    bool clone_needed, bool *cloned)
 {
 	struct bpf_insn insn_buff[16], aux[2];
-	struct bpf_prog *clone, *tmp;
+	struct bpf_prog *prog, *orig_prog, *tmp;
 	int insn_delta, insn_cnt;
 	struct bpf_insn *insn;
 	int i, rewritten;
 
-	if (WARN_ON_ONCE(env && env->prog != prog))
-		return ERR_PTR(-EINVAL);
+	*cloned = false;
+	if (WARN_ON_ONCE(env && env->prog != *pprog))
+		return -EINVAL;
 
-	clone = bpf_prog_clone_create(prog, GFP_USER);
-	if (!clone)
-		return ERR_PTR(-ENOMEM);
+	prog = orig_prog = *pprog;
+	/* only clone the prog when we can fall back to the interpreter */
+	if (clone_needed) {
+		prog = bpf_prog_clone_create(orig_prog, GFP_USER);
+		if (!prog)
+			return -ENOMEM;
 
-	/* make sure bpf_patch_insn_data() patches the correct prog */
-	if (env)
-		env->prog = clone;
+		*cloned = true;
+		/* make sure bpf_patch_insn_data() patches the correct prog */
+		if (env)
+			env->prog = prog;
+	}
 
-	insn_cnt = clone->len;
-	insn = clone->insnsi;
+	insn_cnt = prog->len;
+	insn = prog->insnsi;
 
 	for (i = 0; i < insn_cnt; i++, insn++) {
 		if (bpf_pseudo_func(insn)) {
@@ -1605,42 +1612,49 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bp
 		    insn[1].code == 0)
 			memcpy(aux, insn, sizeof(aux));
 
-		rewritten = bpf_jit_blind_insn(insn, aux, insn_buff,
-						clone->aux->verifier_zext);
+		rewritten = bpf_jit_blind_insn(insn, aux, insn_buff, prog->aux->verifier_zext);
 		if (!rewritten)
 			continue;
 
 		if (env)
 			tmp = bpf_patch_insn_data(env, i, insn_buff, rewritten);
 		else
-			tmp = bpf_patch_insn_single(clone, i, insn_buff, rewritten);
+			tmp = bpf_patch_insn_single(prog, i, insn_buff, rewritten);
 
 		if (IS_ERR_OR_NULL(tmp)) {
-			if (env)
-				/* restore the original prog */
-				env->prog = prog;
-			/* Patching may have repointed aux->prog during
-			 * realloc from the original one, so we need to
-			 * fix it up here on error.
-			 */
-			bpf_jit_prog_release_other(prog, clone);
-			return IS_ERR(tmp) ? tmp : ERR_PTR(-ENOMEM);
+			if (*cloned) {
+				/* roll back to the original prog */
+				*pprog = orig_prog;
+				if (env)
+					env->prog = orig_prog;
+				/* Patching may have repointed aux->prog during
+				 * realloc from the original one, so we need to
+				 * fix it up here on error.
+				 */
+				bpf_jit_prog_release_other(orig_prog, prog);
+			} else {
+				/* just keep the latest successfully patched prog */
+				*pprog = prog;
+			}
+			return IS_ERR(tmp) ? PTR_ERR(tmp) : -ENOMEM;
 		}
 
-		clone = tmp;
+		prog = tmp;
 		insn_delta = rewritten - 1;
 
 		if (env)
-			env->prog = clone;
+			env->prog = prog;
 
 		/* Walk new program and skip insns we just inserted. */
-		insn = clone->insnsi + i + insn_delta;
+		insn = prog->insnsi + i + insn_delta;
 		insn_cnt += insn_delta;
 		i        += insn_delta;
 	}
 
-	clone->blinded = 1;
-	return clone;
+	prog->blinded = 1;
+	*pprog = prog;
+
+	return 0;
 }
 
 bool bpf_insn_is_indirect_target(const struct bpf_verifier_env *env, const struct bpf_prog *prog,
@@ -2630,33 +2644,33 @@ static bool bpf_prog_select_interpreter(struct bpf_prog *fp)
 	return select_interpreter;
 }
 
-static struct bpf_prog *bpf_prog_jit_compile(struct bpf_verifier_env *env, struct bpf_prog *prog)
+static struct bpf_prog *bpf_prog_jit_compile(struct bpf_verifier_env *env, struct bpf_prog *prog,
+					     bool jit_needed)
 {
 #ifdef CONFIG_BPF_JIT
+	int err;
+	bool cloned = false;
 	struct bpf_prog *orig_prog;
 
 	if (!bpf_prog_need_blind(prog))
 		return bpf_int_jit_compile(env, prog);
 
 	orig_prog = prog;
-	prog = bpf_jit_blind_constants(env, prog);
-	/*
-	 * If blinding was requested and we failed during blinding, we must fall
-	 * back to the interpreter.
-	 */
-	if (IS_ERR(prog))
-		goto out_restore;
+	err = bpf_jit_blind_constants(env, &prog, !jit_needed, &cloned);
+	if (err)
+		goto out;
 
 	prog = bpf_int_jit_compile(env, prog);
-	if (prog->jited) {
-		bpf_jit_prog_release_other(prog, orig_prog);
-		return prog;
+	if (cloned) {
+		if (prog->jited) {
+			bpf_jit_prog_release_other(prog, orig_prog);
+		} else {
+			bpf_jit_prog_release_other(orig_prog, prog);
+			prog = orig_prog;
+		}
 	}
 
-	bpf_jit_prog_release_other(orig_prog, prog);
-
-out_restore:
-	prog = orig_prog;
+out:
 #endif
 	return prog;
 }
@@ -2686,7 +2700,7 @@ struct bpf_prog *__bpf_prog_select_runtime(struct bpf_verifier_env *env, struct
 		if (*err)
 			return fp;
 
-		fp = bpf_prog_jit_compile(env, fp);
+		fp = bpf_prog_jit_compile(env, fp, jit_needed);
 		bpf_prog_jit_attempt_done(fp);
 		if (!fp->jited && jit_needed) {
 			*err = -ENOTSUPP;
diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index 2cdad2395b71..6da0f8d0b761 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -996,6 +996,8 @@ static u32 *bpf_dup_subprog_starts(struct bpf_verifier_env *env)
 
 static void bpf_restore_subprog_starts(struct bpf_verifier_env *env, u32 *orig_starts)
 {
+	if (!orig_starts)
+		return;
 	for (int i = 0; i < env->subprog_cnt; i++)
 		env->subprog_info[i].start = orig_starts[i];
 	/* restore the start of fake 'exit' subprog as well */
@@ -1273,35 +1275,33 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 int bpf_jit_subprogs(struct bpf_verifier_env *env)
 {
 	int err, i;
-	bool blinded = false;
 	struct bpf_insn *insn;
 	struct bpf_prog *prog, *orig_prog;
-	u32 *orig_subprog_starts;
+	u32 *orig_subprog_starts = NULL;
+	bool cloned = false;
 
 	if (env->subprog_cnt <= 1)
 		return 0;
 
 	prog = orig_prog = env->prog;
 	if (bpf_prog_need_blind(prog)) {
-		orig_subprog_starts = bpf_dup_subprog_starts(env);
-		if (!orig_subprog_starts) {
-			err = -ENOMEM;
-			goto out_cleanup;
+		if (!prog->jit_required) {
+			orig_subprog_starts = bpf_dup_subprog_starts(env);
+			if (!orig_subprog_starts) {
+				err = -ENOMEM;
+				goto out_cleanup;
+			}
 		}
-		prog = bpf_jit_blind_constants(env, prog);
-		if (IS_ERR(prog)) {
-			err = -ENOMEM;
-			prog = orig_prog;
+		err = bpf_jit_blind_constants(env, &prog, !prog->jit_required, &cloned);
+		if (err)
 			goto out_restore;
-		}
-		blinded = true;
 	}
 
 	err = jit_subprogs(env);
 	if (err)
 		goto out_jit_err;
 
-	if (blinded) {
+	if (cloned) {
 		bpf_jit_prog_release_other(prog, orig_prog);
 		kvfree(orig_subprog_starts);
 	}
@@ -1309,13 +1309,13 @@ int bpf_jit_subprogs(struct bpf_verifier_env *env)
 	return 0;
 
 out_jit_err:
-	if (blinded) {
+	if (cloned) {
 		bpf_jit_prog_release_other(orig_prog, prog);
 		/* roll back to the clean original prog */
 		prog = env->prog = orig_prog;
 		goto out_restore;
 	} else {
-		if (err != -EFAULT) {
+		if (err != -EFAULT && !prog->jit_required) {
 			/*
 			 * We will fall back to interpreter mode when err is not -EFAULT, before
 			 * that, insn->off and insn->imm should be restored to their original
@@ -1344,8 +1344,7 @@ int bpf_jit_subprogs(struct bpf_verifier_env *env)
 int bpf_fixup_call_args(struct bpf_verifier_env *env)
 {
 #ifndef CONFIG_BPF_JIT_ALWAYS_ON
-	struct bpf_prog *prog = env->prog;
-	struct bpf_insn *insn = prog->insnsi;
+	struct bpf_insn *insn;
 	int depth;
 #endif
 	int i, err = 0;
@@ -1371,7 +1370,7 @@ int bpf_fixup_call_args(struct bpf_verifier_env *env)
 			return err;
 	}
 #ifndef CONFIG_BPF_JIT_ALWAYS_ON
-	if (prog->jit_required) {
+	if (env->prog->jit_required) {
 		verbose(env, "program requires BPF JIT compiler but it is not available\n");
 		return -EINVAL;
 	}
@@ -1388,7 +1387,7 @@ int bpf_fixup_call_args(struct bpf_verifier_env *env)
 		verbose(env, "tail_calls are not allowed in non-JITed programs with bpf-to-bpf calls\n");
 		return -EINVAL;
 	}
-	for (i = 0; i < prog->len; i++, insn++) {
+	for (i = 0, insn = env->prog->insnsi; i < env->prog->len; i++, insn++) {
 		if (bpf_pseudo_func(insn)) {
 			/* When JIT fails the progs with callback calls
 			 * have to be rejected, since interpreter doesn't support them yet.
-- 
2.47.3